| Message ID | 20200513054324.2138483-4-ira.weiny@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/9] fs/ext4: Narrow scope of DAX check in setflags | expand |
On Tue, May 12, 2020 at 10:43:18PM -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. > > Furthermore, clarify the documentation of the exclusivity and how that > will work. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes: > remove WARN_ON_ONCE > Add documentation to the encrypt doc WRT DAX > --- > Documentation/filesystems/fscrypt.rst | 4 +++- > fs/ext4/super.c | 10 +--------- > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst > index aa072112cfff..1475b8d52fef 100644 > --- a/Documentation/filesystems/fscrypt.rst > +++ b/Documentation/filesystems/fscrypt.rst > @@ -1038,7 +1038,9 @@ astute users may notice some differences in behavior: > - The ext4 filesystem does not support data journaling with encrypted > regular files. It will fall back to ordered data mode instead. > > -- DAX (Direct Access) is not supported on encrypted files. > +- DAX (Direct Access) is not supported on encrypted files. Attempts to enable > + DAX on an encrypted file will fail. Mount options will _not_ enable DAX on > + encrypted files. > > - The st_size of an encrypted symlink will not necessarily give the > length of the symlink target as required by POSIX. It will actually > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index bf5fcb477f66..9873ab27e3fa 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1320,7 +1320,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 (IS_DAX(inode)) > return -EINVAL; > > res = ext4_convert_inline_data(inode); > @@ -1344,10 +1344,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; > @@ -1371,10 +1367,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) I'm confused by the ext4_set_context() change. ext4_set_context() is only called when FS_IOC_SET_ENCRYPTION_POLICY sets an encryption policy on an empty directory, *or* when a new inode (regular, dir, or symlink) is created in an encrypted directory (thus inheriting encryption from its parent). So when is it reachable when IS_DAX()? Is the issue that the DAX flag can now be set on directories? The commit message doesn't seem to be talking about directories. Is the behavior we want is that on an (empty) directory with the DAX flag set, FS_IOC_SET_ENCRYPTION_POLICY should fail with EINVAL? I don't see why the i_size_read(inode) check is there though, so I think you're at least right to remove that. - Eric
On Fri, May 15, 2020 at 07:02:53PM -0700, Eric Biggers wrote: > On Tue, May 12, 2020 at 10:43:18PM -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. > > > > Furthermore, clarify the documentation of the exclusivity and how that > > will work. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > --- > > Changes: > > remove WARN_ON_ONCE > > Add documentation to the encrypt doc WRT DAX > > --- > > Documentation/filesystems/fscrypt.rst | 4 +++- > > fs/ext4/super.c | 10 +--------- > > 2 files changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst > > index aa072112cfff..1475b8d52fef 100644 > > --- a/Documentation/filesystems/fscrypt.rst > > +++ b/Documentation/filesystems/fscrypt.rst > > @@ -1038,7 +1038,9 @@ astute users may notice some differences in behavior: > > - The ext4 filesystem does not support data journaling with encrypted > > regular files. It will fall back to ordered data mode instead. > > > > -- DAX (Direct Access) is not supported on encrypted files. > > +- DAX (Direct Access) is not supported on encrypted files. Attempts to enable > > + DAX on an encrypted file will fail. Mount options will _not_ enable DAX on > > + encrypted files. > > > > - The st_size of an encrypted symlink will not necessarily give the > > length of the symlink target as required by POSIX. It will actually > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index bf5fcb477f66..9873ab27e3fa 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -1320,7 +1320,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 (IS_DAX(inode)) > > return -EINVAL; > > > > res = ext4_convert_inline_data(inode); > > @@ -1344,10 +1344,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; > > @@ -1371,10 +1367,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) > > I'm confused by the ext4_set_context() change. > > ext4_set_context() is only called when FS_IOC_SET_ENCRYPTION_POLICY sets an > encryption policy on an empty directory, *or* when a new inode (regular, dir, or > symlink) is created in an encrypted directory (thus inheriting encryption from > its parent). I don't see the check which prevents FS_IOC_SET_ENCRYPTION_POLICY on a file? On inode creation, encryption will always usurp S_DAX... > > So when is it reachable when IS_DAX()? Is the issue that the DAX flag can now > be set on directories? The commit message doesn't seem to be talking about > directories. Is the behavior we want is that on an (empty) directory with the > DAX flag set, FS_IOC_SET_ENCRYPTION_POLICY should fail with EINVAL? We would want that but AFIAK S_DAX is never set on directories. Perhaps this is another place where S_DAX needs to be changed to the new inode flag? However, this would not be appropriate at this point in the series. At this point in the series S_DAX is still set based on the mount option and I'm 99% sure that only happens on regular files, not directories. So I'm confused now. This is, AFAICS, not going to affect correctness. It will only be confusing because the user will be able to set both DAX and encryption on the directory but files there will only see encryption being used... :-( Assuming you are correct about this call path only being valid on directories. It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in "fs/ext4: Introduce DAX inode flag"? Then at that point we can prevent DAX and encryption on a directory. ... and at this point IS_DAX() could be removed at this point in the series??? > > I don't see why the i_size_read(inode) check is there though, so I think you're > at least right to remove that. Agreed. Ira > > - Eric
On Sun, May 17, 2020 at 10:03:15PM -0700, Ira Weiny wrote: > On Fri, May 15, 2020 at 07:02:53PM -0700, Eric Biggers wrote: > > On Tue, May 12, 2020 at 10:43:18PM -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. > > > > > > Furthermore, clarify the documentation of the exclusivity and how that > > > will work. > > > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > --- > > > Changes: > > > remove WARN_ON_ONCE > > > Add documentation to the encrypt doc WRT DAX > > > --- > > > Documentation/filesystems/fscrypt.rst | 4 +++- > > > fs/ext4/super.c | 10 +--------- > > > 2 files changed, 4 insertions(+), 10 deletions(-) > > > > > > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst > > > index aa072112cfff..1475b8d52fef 100644 > > > --- a/Documentation/filesystems/fscrypt.rst > > > +++ b/Documentation/filesystems/fscrypt.rst > > > @@ -1038,7 +1038,9 @@ astute users may notice some differences in behavior: > > > - The ext4 filesystem does not support data journaling with encrypted > > > regular files. It will fall back to ordered data mode instead. > > > > > > -- DAX (Direct Access) is not supported on encrypted files. > > > +- DAX (Direct Access) is not supported on encrypted files. Attempts to enable > > > + DAX on an encrypted file will fail. Mount options will _not_ enable DAX on > > > + encrypted files. > > > > > > - The st_size of an encrypted symlink will not necessarily give the > > > length of the symlink target as required by POSIX. It will actually > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > > index bf5fcb477f66..9873ab27e3fa 100644 > > > --- a/fs/ext4/super.c > > > +++ b/fs/ext4/super.c > > > @@ -1320,7 +1320,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 (IS_DAX(inode)) > > > return -EINVAL; > > > > > > res = ext4_convert_inline_data(inode); > > > @@ -1344,10 +1344,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; > > > @@ -1371,10 +1367,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) > > > > I'm confused by the ext4_set_context() change. > > > > ext4_set_context() is only called when FS_IOC_SET_ENCRYPTION_POLICY sets an > > encryption policy on an empty directory, *or* when a new inode (regular, dir, or > > symlink) is created in an encrypted directory (thus inheriting encryption from > > its parent). > > I don't see the check which prevents FS_IOC_SET_ENCRYPTION_POLICY on a file? It's in fscrypt_ioctl_set_policy(). > > On inode creation, encryption will always usurp S_DAX... > > > > > So when is it reachable when IS_DAX()? Is the issue that the DAX flag can now > > be set on directories? The commit message doesn't seem to be talking about > > directories. Is the behavior we want is that on an (empty) directory with the > > DAX flag set, FS_IOC_SET_ENCRYPTION_POLICY should fail with EINVAL? > > We would want that but AFIAK S_DAX is never set on directories. Perhaps this > is another place where S_DAX needs to be changed to the new inode flag? > However, this would not be appropriate at this point in the series. At this > point in the series S_DAX is still set based on the mount option and I'm 99% > sure that only happens on regular files, not directories. So I'm confused now. S_DAX is only set by ext4_set_inode_flags() which only sets it on regular files. > > This is, AFAICS, not going to affect correctness. It will only be confusing > because the user will be able to set both DAX and encryption on the directory > but files there will only see encryption being used... :-( > > Assuming you are correct about this call path only being valid on directories. > It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in > "fs/ext4: Introduce DAX inode flag"? Then at that point we can prevent DAX and > encryption on a directory. ... and at this point IS_DAX() could be removed at > this point in the series??? I haven't read the whole series, but if you are indeed trying to prevent a directory with EXT4_DAX_FL from being encrypted, then it does look like you'd need to check EXT4_DAX_FL, not S_DAX. The other question is what should happen when a file is created in an encrypted directory when the filesystem is mounted with -o dax. Actually, I think I missed something there. Currently (based on reading the code) the DAX flag will get set first, and then ext4_set_context() will see IS_DAX() && i_size == 0 and clear the DAX flag when setting the encrypt flag. So, the i_size == 0 check is actually needed. Your patch (AFAICS) just makes creating an encrypted file fail when '-o dax'. Is that intended? If not, maybe you should change it to check S_NEW instead of i_size == 0 to make it clearer? - Eric
On Mon, May 18, 2020 at 09:24:47AM -0700, Eric Biggers wrote: > On Sun, May 17, 2020 at 10:03:15PM -0700, Ira Weiny wrote: > > On Fri, May 15, 2020 at 07:02:53PM -0700, Eric Biggers wrote: > > > On Tue, May 12, 2020 at 10:43:18PM -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. > > > > > > > > Furthermore, clarify the documentation of the exclusivity and how that > > > > will work. > > > > > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > > > --- > > > > Changes: > > > > remove WARN_ON_ONCE > > > > Add documentation to the encrypt doc WRT DAX > > > > --- > > > > Documentation/filesystems/fscrypt.rst | 4 +++- > > > > fs/ext4/super.c | 10 +--------- > > > > 2 files changed, 4 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst > > > > index aa072112cfff..1475b8d52fef 100644 > > > > --- a/Documentation/filesystems/fscrypt.rst > > > > +++ b/Documentation/filesystems/fscrypt.rst > > > > @@ -1038,7 +1038,9 @@ astute users may notice some differences in behavior: > > > > - The ext4 filesystem does not support data journaling with encrypted > > > > regular files. It will fall back to ordered data mode instead. > > > > > > > > -- DAX (Direct Access) is not supported on encrypted files. > > > > +- DAX (Direct Access) is not supported on encrypted files. Attempts to enable > > > > + DAX on an encrypted file will fail. Mount options will _not_ enable DAX on > > > > + encrypted files. > > > > > > > > - The st_size of an encrypted symlink will not necessarily give the > > > > length of the symlink target as required by POSIX. It will actually > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > > > index bf5fcb477f66..9873ab27e3fa 100644 > > > > --- a/fs/ext4/super.c > > > > +++ b/fs/ext4/super.c > > > > @@ -1320,7 +1320,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 (IS_DAX(inode)) > > > > return -EINVAL; > > > > > > > > res = ext4_convert_inline_data(inode); > > > > @@ -1344,10 +1344,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; > > > > @@ -1371,10 +1367,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) > > > > > > I'm confused by the ext4_set_context() change. > > > > > > ext4_set_context() is only called when FS_IOC_SET_ENCRYPTION_POLICY sets an > > > encryption policy on an empty directory, *or* when a new inode (regular, dir, or > > > symlink) is created in an encrypted directory (thus inheriting encryption from > > > its parent). > > > > I don't see the check which prevents FS_IOC_SET_ENCRYPTION_POLICY on a file? > > It's in fscrypt_ioctl_set_policy(). I see... > > > > > On inode creation, encryption will always usurp S_DAX... > > > > > > > > So when is it reachable when IS_DAX()? Is the issue that the DAX flag can now > > > be set on directories? The commit message doesn't seem to be talking about > > > directories. Is the behavior we want is that on an (empty) directory with the > > > DAX flag set, FS_IOC_SET_ENCRYPTION_POLICY should fail with EINVAL? > > > > We would want that but AFIAK S_DAX is never set on directories. Perhaps this > > is another place where S_DAX needs to be changed to the new inode flag? > > However, this would not be appropriate at this point in the series. At this > > point in the series S_DAX is still set based on the mount option and I'm 99% > > sure that only happens on regular files, not directories. So I'm confused now. > > S_DAX is only set by ext4_set_inode_flags() which only sets it on regular files. Exactly... > > > > > This is, AFAICS, not going to affect correctness. It will only be confusing > > because the user will be able to set both DAX and encryption on the directory > > but files there will only see encryption being used... :-( > > > > Assuming you are correct about this call path only being valid on directories. > > It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in > > "fs/ext4: Introduce DAX inode flag"? Then at that point we can prevent DAX and > > encryption on a directory. ... and at this point IS_DAX() could be removed at > > this point in the series??? > > I haven't read the whole series, but if you are indeed trying to prevent a > directory with EXT4_DAX_FL from being encrypted, then it does look like you'd > need to check EXT4_DAX_FL, not S_DAX. Yep. > > The other question is what should happen when a file is created in an encrypted > directory when the filesystem is mounted with -o dax. Actually, I think I > missed something there. Currently (based on reading the code) the DAX flag will > get set first, and then ext4_set_context() See this is where I am confused. Above you said that ext4_set_context() is only called on a directory. And I agree with you now having seen the check in fscrypt_ioctl_set_policy(). So what is the call path you are speaking of here? > will see IS_DAX() && i_size == 0 and > clear the DAX flag when setting the encrypt flag. So, the i_size == 0 check is > actually needed. Your patch (AFAICS) just makes creating an encrypted file fail > when '-o dax'. Is that intended? Yes that is what I intended for this patch. At this point in the series the file system is either all DAX (-o dax) or not. I did not comprehend the directory vs regular file complexity with fscrypt. It seems this patch should be removing the IS_DAX() check completely but I'm still not sure if a regular file inode could be passed to ext4_set_context() and I think we need to protect if it has IS_DAX() set if it does... An alternate solution would be to drop this patch entirely and change the code later in the series once EXT4_DAX_FL is defined... But I'm not even clear where EXT4_ENCRYPT_FL is set... Ira > If not, maybe you should change it to check > S_NEW instead of i_size == 0 to make it clearer? > > - Eric
On Mon, May 18, 2020 at 12:23:57PM -0700, Ira Weiny wrote: > > > > The other question is what should happen when a file is created in an encrypted > > directory when the filesystem is mounted with -o dax. Actually, I think I > > missed something there. Currently (based on reading the code) the DAX flag will > > get set first, and then ext4_set_context() > > See this is where I am confused. Above you said that ext4_set_context() is only > called on a directory. And I agree with you now having seen the check in > fscrypt_ioctl_set_policy(). So what is the call path you are speaking of here? Here's what I actually said: ext4_set_context() is only called when FS_IOC_SET_ENCRYPTION_POLICY sets an encryption policy on an empty directory, *or* when a new inode (regular, dir, or symlink) is created in an encrypted directory (thus inheriting encryption from its parent). Just find the places where ->set_context() is called and follow them backwards. - Eric
On Mon, May 18, 2020 at 09:24:47AM -0700, Eric Biggers wrote: > On Sun, May 17, 2020 at 10:03:15PM -0700, Ira Weiny wrote: First off... OMG... I'm seeing some possible user pitfalls which are complicating things IMO. It probably does not matter because most users don't care and have either enabled DAX on _every_ mount or _not_ enabled DAX on _every_ mount. And have _not_ used verity nor encryption while using DAX. Verity is a bit easier because verity is not inherited and we only need to protect against setting it if DAX is on. However, it can be weird for the user thusly: 1) mount _without_ DAX 2) enable verity on individual inodes 3) unmount/mount _with_ DAX Now the verity files are not enabled for DAX without any indication... <sigh> This is still true with my patch. But at least it closes the hole of trying to change the DAX flag after the fact (because verity was set). Also both this check and the verity need to be maintained to keep the mount option working as it was before... For encryption it is more complicated because encryption can be set on directories and inherited so the IS_DAX() check does nothing while '-o dax' is used. Therefore users can: 1) mount _with_ DAX 2) enable encryption on a directory 3) files created in that directory will not have DAX set And I now understand why the WARN_ON() was there... To tell users about this craziness. ... > > This is, AFAICS, not going to affect correctness. It will only be confusing > > because the user will be able to set both DAX and encryption on the directory > > but files there will only see encryption being used... :-( > > > > Assuming you are correct about this call path only being valid on directories. > > It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in > > "fs/ext4: Introduce DAX inode flag"? Then at that point we can prevent DAX and > > encryption on a directory. ... and at this point IS_DAX() could be removed at > > this point in the series??? > > I haven't read the whole series, but if you are indeed trying to prevent a > directory with EXT4_DAX_FL from being encrypted, then it does look like you'd > need to check EXT4_DAX_FL, not S_DAX. > > The other question is what should happen when a file is created in an encrypted > directory when the filesystem is mounted with -o dax. Actually, I think I > missed something there. Currently (based on reading the code) the DAX flag will > get set first, and then ext4_set_context() will see IS_DAX() && i_size == 0 and > clear the DAX flag when setting the encrypt flag. I think you are correct. > > So, the i_size == 0 check is actually needed. > Your patch (AFAICS) just makes creating an encrypted file fail > when '-o dax'. Is that intended? Yes that is what I intended but it is more complicated I see now. The intent is that IS_DAX() should _never_ be true on an encrypted or verity file... even if -o dax is specified. Because IS_DAX() should be a result of the inode flags being checked. The order of the setting of those flags is a bit odd for the encrypted case. I don't really like that DAX is set then un-set. It is convoluted but I'm not clear right now how to fix it. > If not, maybe you should change it to check > S_NEW instead of i_size == 0 to make it clearer? The patch is completely unnecessary. It is much easier to make (EXT4_ENCRYPT_FL | EXT4_VERITY_FL) incompatible with EXT4_DAX_FL when it is introduced later in the series. Furthermore this mutual exclusion can be done on directories in the encrypt case. Which I think will be nicer for the user if they get an error when trying to set one when the other is set. Ira
On Tue 19-05-20 19:02:33, Ira Weiny wrote: > On Mon, May 18, 2020 at 09:24:47AM -0700, Eric Biggers wrote: > > On Sun, May 17, 2020 at 10:03:15PM -0700, Ira Weiny wrote: > > First off... OMG... > > I'm seeing some possible user pitfalls which are complicating things IMO. It > probably does not matter because most users don't care and have either enabled > DAX on _every_ mount or _not_ enabled DAX on _every_ mount. And have _not_ > used verity nor encryption while using DAX. > > Verity is a bit easier because verity is not inherited and we only need to > protect against setting it if DAX is on. > > However, it can be weird for the user thusly: > > 1) mount _without_ DAX > 2) enable verity on individual inodes > 3) unmount/mount _with_ DAX > > Now the verity files are not enabled for DAX without any indication... > <sigh> This is still true with my patch. But at least it closes the hole > of trying to change the DAX flag after the fact (because verity was set). > > Also both this check and the verity need to be maintained to keep the mount > option working as it was before... > > For encryption it is more complicated because encryption can be set on > directories and inherited so the IS_DAX() check does nothing while '-o > dax' is used. Therefore users can: > > 1) mount _with_ DAX > 2) enable encryption on a directory > 3) files created in that directory will not have DAX set > > And I now understand why the WARN_ON() was there... To tell users about this > craziness. Thanks for digging into this! I agree that just not setting S_DAX where other inode features disallow that is probably the best. > > > This is, AFAICS, not going to affect correctness. It will only be confusing > > > because the user will be able to set both DAX and encryption on the directory > > > but files there will only see encryption being used... :-( > > > > > > Assuming you are correct about this call path only being valid on directories. > > > It seems this IS_DAX() needs to be changed to check for EXT4_DAX_FL in > > > "fs/ext4: Introduce DAX inode flag"? Then at that point we can prevent DAX and > > > encryption on a directory. ... and at this point IS_DAX() could be removed at > > > this point in the series??? > > > > I haven't read the whole series, but if you are indeed trying to prevent a > > directory with EXT4_DAX_FL from being encrypted, then it does look like you'd > > need to check EXT4_DAX_FL, not S_DAX. > > > > The other question is what should happen when a file is created in an encrypted > > directory when the filesystem is mounted with -o dax. Actually, I think I > > missed something there. Currently (based on reading the code) the DAX flag will > > get set first, and then ext4_set_context() will see IS_DAX() && i_size == 0 and > > clear the DAX flag when setting the encrypt flag. > > I think you are correct. > > > > > So, the i_size == 0 check is actually needed. > > Your patch (AFAICS) just makes creating an encrypted file fail > > when '-o dax'. Is that intended? > > Yes that is what I intended but it is more complicated I see now. > > The intent is that IS_DAX() should _never_ be true on an encrypted or verity > file... even if -o dax is specified. Because IS_DAX() should be a result of > the inode flags being checked. The order of the setting of those flags is a > bit odd for the encrypted case. I don't really like that DAX is set then > un-set. It is convoluted but I'm not clear right now how to fix it. > > > If not, maybe you should change it to check > > S_NEW instead of i_size == 0 to make it clearer? > > The patch is completely unnecessary. > > It is much easier to make (EXT4_ENCRYPT_FL | EXT4_VERITY_FL) incompatible > with EXT4_DAX_FL when it is introduced later in the series. Furthermore > this mutual exclusion can be done on directories in the encrypt case. > Which I think will be nicer for the user if they get an error when trying > to set one when the other is set. Agreed. Honza
diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst index aa072112cfff..1475b8d52fef 100644 --- a/Documentation/filesystems/fscrypt.rst +++ b/Documentation/filesystems/fscrypt.rst @@ -1038,7 +1038,9 @@ astute users may notice some differences in behavior: - The ext4 filesystem does not support data journaling with encrypted regular files. It will fall back to ordered data mode instead. -- DAX (Direct Access) is not supported on encrypted files. +- DAX (Direct Access) is not supported on encrypted files. Attempts to enable + DAX on an encrypted file will fail. Mount options will _not_ enable DAX on + encrypted files. - The st_size of an encrypted symlink will not necessarily give the length of the symlink target as required by POSIX. It will actually diff --git a/fs/ext4/super.c b/fs/ext4/super.c index bf5fcb477f66..9873ab27e3fa 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1320,7 +1320,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 (IS_DAX(inode)) return -EINVAL; res = ext4_convert_inline_data(inode); @@ -1344,10 +1344,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; @@ -1371,10 +1367,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)