Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
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... > 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 -- Jan Kara SUSE Labs, CR
Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
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... 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
Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
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
Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
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.we...@intel.com wrote:
> > > > From: Ira Weiny
> > > >
> > > > 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
> > > >
> > > > ---
> > > > 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
Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
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.we...@intel.com wrote:
> > > From: Ira Weiny
> > >
> > > 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
> > >
> > > ---
> > > 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
Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
On Fri, May 15, 2020 at 07:02:53PM -0700, Eric Biggers wrote:
> On Tue, May 12, 2020 at 10:43:18PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny
> >
> > 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
> >
> > ---
> > 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 t
Re: [PATCH 3/9] fs/ext4: Disallow encryption if inode is DAX
On Tue, May 12, 2020 at 10:43:18PM -0700, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> 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
>
> ---
> 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
