| Message ID | 20200414040030.1802884-4-ira.weiny@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable ext4 support for per-file/directory DAX operations | expand |
On Mon 13-04-20 21:00:25, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Encryption and DAX are incompatible. Changing the DAX mode due to a > change in Encryption mode is wrong without a corresponding > address_space_operations update. > > Make the 2 options mutually exclusive by returning an error if DAX was > set first. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > fs/ext4/super.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 0c7c4adb664e..b14863058115 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1325,7 +1325,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > if (inode->i_ino == EXT4_ROOT_INO) > return -EPERM; > > - if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode))) > + if (WARN_ON_ONCE(IS_DAX(inode))) Also here I don't think WARN_ON_ONCE() is warranted once we allow per-inode setting of DAX. It will then become a regular error condition... Honza > return -EINVAL; > > res = ext4_convert_inline_data(inode); > @@ -1349,10 +1349,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT); > ext4_clear_inode_state(inode, > EXT4_STATE_MAY_INLINE_DATA); > - /* > - * Update inode->i_flags - S_ENCRYPTED will be enabled, > - * S_DAX may be disabled > - */ > ext4_set_inode_flags(inode); > } > return res; > @@ -1376,10 +1372,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > ctx, len, 0); > if (!res) { > ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT); > - /* > - * Update inode->i_flags - S_ENCRYPTED will be enabled, > - * S_DAX may be disabled > - */ > ext4_set_inode_flags(inode); > res = ext4_mark_inode_dirty(handle, inode); > if (res) > -- > 2.25.1 >
On Mon, Apr 13, 2020 at 09:00:25PM -0700, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Encryption and DAX are incompatible. Changing the DAX mode due to a > change in Encryption mode is wrong without a corresponding > address_space_operations update. > > Make the 2 options mutually exclusive by returning an error if DAX was > set first. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> The encryption flag is inherited from the containing directory, and directories can't have the DAX flag set, so anything we do in ext4_set_context() will be safety belt / sanity checking in nature. But we *do* need to figure out what we do with mount -o dax=always when the file system might have encrypted files. My previous comments about the verity flag and dax flag applies here. Also note that encrypted files are read/write so we must never allow the combination of ENCRPYT_FL and DAX_FL. So that may be something where we should teach __ext4_iget() to check for this, and declare the file system as corrupted if it sees this combination. (For VERITY_FL && DAX_FL that is a combo that we might want to support in the future, so that's probably a case where arguably, we should just ignore the DAX_FL for now.) - Ted
On Wed, Apr 15, 2020 at 9:03 AM Theodore Y. Ts'o <tytso@mit.edu> wrote: > > On Mon, Apr 13, 2020 at 09:00:25PM -0700, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Encryption and DAX are incompatible. Changing the DAX mode due to a > > change in Encryption mode is wrong without a corresponding > > address_space_operations update. > > > > Make the 2 options mutually exclusive by returning an error if DAX was > > set first. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > The encryption flag is inherited from the containing directory, and > directories can't have the DAX flag set, so anything we do in > ext4_set_context() will be safety belt / sanity checking in nature. > > But we *do* need to figure out what we do with mount -o dax=always > when the file system might have encrypted files. My previous comments > about the verity flag and dax flag applies here. > > Also note that encrypted files are read/write so we must never allow > the combination of ENCRPYT_FL and DAX_FL. So that may be something > where we should teach __ext4_iget() to check for this, and declare the > file system as corrupted if it sees this combination. (For VERITY_FL > && DAX_FL that is a combo that we might want to support in the future, > so that's probably a case where arguably, we should just ignore the > DAX_FL for now.) We also have a pending consideration for what MKTME (inline memory encryption with programmable hardware keys) means for file-encryption + dax. Certainly kernel based software encryption is incompatible with dax, but one of the hallway track discussions I wanted to have at LSF is whether fscrypt is the right interface for managing inline memory encryption. For now, disallowing ENCRPYT_FL + DAX_FL seems ok if ENCRPYT_FL always means software encryption, but this is something to circle back to once we get MKTME implemented for volume encryption and start to look at finer grained (per-directory key) encryption.
On Wed, Apr 15, 2020 at 12:03:07PM -0400, Theodore Y. Ts'o wrote: > On Mon, Apr 13, 2020 at 09:00:25PM -0700, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Encryption and DAX are incompatible. Changing the DAX mode due to a > > change in Encryption mode is wrong without a corresponding > > address_space_operations update. > > > > Make the 2 options mutually exclusive by returning an error if DAX was > > set first. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > The encryption flag is inherited from the containing directory, and > directories can't have the DAX flag set, But they can have FS_XFLAG_DAX set. > so anything we do in > ext4_set_context() will be safety belt / sanity checking in nature. > > But we *do* need to figure out what we do with mount -o dax=always > when the file system might have encrypted files. My previous comments > about the verity flag and dax flag applies here. :-( agreed. FWIW without these patches an inode which has encrypt or verity set is already turning off DAX... So we already have a '-o dax' flag which is not "always". :-( Unfortunately the 'always' designation kind of breaks semantically but it is equal to the current mount option. > > Also note that encrypted files are read/write so we must never allow > the combination of ENCRPYT_FL and DAX_FL. So that may be something > where we should teach __ext4_iget() to check for this, and declare the > file system as corrupted if it sees this combination. ok... > (For VERITY_FL > && DAX_FL that is a combo that we might want to support in the future, > so that's probably a case where arguably, we should just ignore the > DAX_FL for now.) ok... Ira
On Wed, Apr 15, 2020 at 02:02:41PM +0200, Jan Kara wrote: > On Mon 13-04-20 21:00:25, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Encryption and DAX are incompatible. Changing the DAX mode due to a > > change in Encryption mode is wrong without a corresponding > > address_space_operations update. > > > > Make the 2 options mutually exclusive by returning an error if DAX was > > set first. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > fs/ext4/super.c | 10 +--------- > > 1 file changed, 1 insertion(+), 9 deletions(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index 0c7c4adb664e..b14863058115 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -1325,7 +1325,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > > if (inode->i_ino == EXT4_ROOT_INO) > > return -EPERM; > > > > - if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode))) > > + if (WARN_ON_ONCE(IS_DAX(inode))) > > Also here I don't think WARN_ON_ONCE() is warranted once we allow per-inode > setting of DAX. It will then become a regular error condition... Removed. Ira > > Honza > > > return -EINVAL; > > > > res = ext4_convert_inline_data(inode); > > @@ -1349,10 +1349,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > > ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT); > > ext4_clear_inode_state(inode, > > EXT4_STATE_MAY_INLINE_DATA); > > - /* > > - * Update inode->i_flags - S_ENCRYPTED will be enabled, > > - * S_DAX may be disabled > > - */ > > ext4_set_inode_flags(inode); > > } > > return res; > > @@ -1376,10 +1372,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > > ctx, len, 0); > > if (!res) { > > ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT); > > - /* > > - * Update inode->i_flags - S_ENCRYPTED will be enabled, > > - * S_DAX may be disabled > > - */ > > ext4_set_inode_flags(inode); > > res = ext4_mark_inode_dirty(handle, inode); > > if (res) > > -- > > 2.25.1 > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Wed, Apr 15, 2020 at 12:54:34PM -0700, 'Ira Weiny' wrote: > On Wed, Apr 15, 2020 at 12:03:07PM -0400, Theodore Y. Ts'o wrote: > > On Mon, Apr 13, 2020 at 09:00:25PM -0700, ira.weiny@intel.com wrote: [snip] > > > > Also note that encrypted files are read/write so we must never allow > > the combination of ENCRPYT_FL and DAX_FL. So that may be something > > where we should teach __ext4_iget() to check for this, and declare the > > file system as corrupted if it sees this combination. > > ok... After thinking about this... Do we really want to declare the FS corrupted? If so, I think we need to return errors when such a configuration is attempted. If in the future we have an encrypted mode which can co-exist with DAX (such as Dan mentioned) we can change this. FWIW I think we should return errors when such a configuration is attempted but _not_ declare the FS corrupted. That allows users to enable this configuration later if we can figure out how to support it. > > > (For VERITY_FL > > && DAX_FL that is a combo that we might want to support in the future, > > so that's probably a case where arguably, we should just ignore the > > DAX_FL for now.) > > ok... I think this should work the same. It looks like VERITY_FL and ENCRYPT_FL are _not_ user modifiable? Is that correct? You said that ENCRPYT_FL is set from the parent directory? But I'm not seeing where that occurs? Similarly I don't see where VERITY_FL is being set either? :-/ I think to make this work correctly we should restrict setting those flags if DAX_FL is set and vice versa. But I'm not finding where to do that. :-/ Ira > > Ira >
On Tue, Apr 21, 2020 at 11:41:43AM -0700, Ira Weiny wrote: > On Wed, Apr 15, 2020 at 12:54:34PM -0700, 'Ira Weiny' wrote: > > On Wed, Apr 15, 2020 at 12:03:07PM -0400, Theodore Y. Ts'o wrote: > > > On Mon, Apr 13, 2020 at 09:00:25PM -0700, ira.weiny@intel.com wrote: > > [snip] > > > > > > > Also note that encrypted files are read/write so we must never allow > > > the combination of ENCRPYT_FL and DAX_FL. So that may be something > > > where we should teach __ext4_iget() to check for this, and declare the > > > file system as corrupted if it sees this combination. > > > > ok... > > After thinking about this... > > Do we really want to declare the FS corrupted? Seeing as we're defining the dax inode flag to be advisory (since its value is ignored if the fs isn't on pmem, or the administrator overrode with dax=never mount option), I don't see why that's filesystem corruption. I can see a case for returning errors if you're trying to change ENCRYPT or VERITY on a file that's has S_DAX set. We can't encrypt or set verity data for a file that could be changed behind our backs, so the kernel cannot satisfy /that/ request. As for changing FS_DAX_FL on an encrypted/verity'd file, the API says that it might not have an immedate effect on S_DAX and that programs have to check S_DAX after changing FS_DAX_FL. It'll never result in S_DAX being set, but the current spec never guarantees that. ;) (If FS_DAX_FL were *mandatory* then yes that would be corruption.) --D > If so, I think we need to return errors when such a configuration is attempted. > If in the future we have an encrypted mode which can co-exist with DAX (such as > Dan mentioned) we can change this. > > FWIW I think we should return errors when such a configuration is attempted but > _not_ declare the FS corrupted. That allows users to enable this configuration > later if we can figure out how to support it. > > > > > > (For VERITY_FL > > > && DAX_FL that is a combo that we might want to support in the future, > > > so that's probably a case where arguably, we should just ignore the > > > DAX_FL for now.) > > > > ok... > > I think this should work the same. > > It looks like VERITY_FL and ENCRYPT_FL are _not_ user modifiable? Is that > correct? > > You said that ENCRPYT_FL is set from the parent directory? But I'm not seeing > where that occurs? > > Similarly I don't see where VERITY_FL is being set either? :-/ > > I think to make this work correctly we should restrict setting those flags if > DAX_FL is set and vice versa. But I'm not finding where to do that. :-/ > > Ira > > > > > Ira > >
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 0c7c4adb664e..b14863058115 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1325,7 +1325,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, if (inode->i_ino == EXT4_ROOT_INO) return -EPERM; - if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode))) + if (WARN_ON_ONCE(IS_DAX(inode))) return -EINVAL; res = ext4_convert_inline_data(inode); @@ -1349,10 +1349,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT); ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA); - /* - * Update inode->i_flags - S_ENCRYPTED will be enabled, - * S_DAX may be disabled - */ ext4_set_inode_flags(inode); } return res; @@ -1376,10 +1372,6 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, ctx, len, 0); if (!res) { ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT); - /* - * Update inode->i_flags - S_ENCRYPTED will be enabled, - * S_DAX may be disabled - */ ext4_set_inode_flags(inode); res = ext4_mark_inode_dirty(handle, inode); if (res)