Re: [PATCH] ext4: turn off DAX on new files when encryption is set

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Jul 27, 2026 at 09:06:23PM +0200, Jan Kara wrote:
> On Thu 23-07-26 14:26:48, Ojaswin Mujoo wrote:
> > Currently, when setting the S_ENCRYPTED flag on a new regular inode in
> > a -o dax=always mounted FS, we seem to be erroneously retaining the S_DAX
> > flag. This is because the newly created inode ends up with the following:
> > 
> >   __ext4_new_inode()
> >     ext4_set_inode_flags(init=true) // sets S_DAX
> >       fscrypt_set_context()
> >         ext4_set_context()
> >           ext4_set_inode_flags(init=false) // sets S_ENCRYPTED but
> >                                               doesn't clears S_DAX
> >       ext4_set_aops
> >         inode->i_mapping->a_ops = &ext4_dax_aops;
> > 
> > Due to the S_DAX flag, the excrypted inode gets ext4_dax_aops and it
> > silently ends up bypassing encryption completely. This is reflected in
> > multiple xfstests failures like generic/548. To fix this, ensure we disable
> > S_DAX correctly when S_ENCRYPTED is being set on a newly created inode. It
> > is safe to change DAX state there because ext4_set_aops() later can
> > correctly detect S_DAX unset and assign the correct aops.
> > 
> > The fix was actually the intended behavior however it seemed to have
> > silently changed in 043546e46dc7.
> > 
> > Fixes: 043546e46dc7 ("fs/ext4: Only change S_DAX on inode load")
> > Reported-by: Disha Goel <disgoel@linux.ibm.com>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  fs/ext4/crypto.c | 15 +++++++++++----
> >  fs/ext4/inode.c  | 10 ++++++----
> >  2 files changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> > index f41f320f4437..2d409114e02e 100644
> > --- a/fs/ext4/crypto.c
> > +++ b/fs/ext4/crypto.c
> > @@ -134,6 +134,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> >  {
> >  	handle_t *handle = fs_data;
> >  	int res, res2, credits, retries = 0;
> > +	bool init = S_ISREG(inode->i_mode);
> 
> Hum, I have some comments: IMHO the change is confusing because the meaning
> of 'init' argument of ext4_set_inode_flags() doesn't mean the first setting
> of flags any longer. And the reliance on ext4_set_context() getting called
> in a safe place exactly for regular files is rather subtle as well.
> 
> Also does the change to ext4_set_inode_flags() really work? As far as I'm
> reading the ext4 crypto implementation, I don't see where we'd be setting
> EXT4_INODE_ENCRYPT for inodes with already enabled encryption (where we
> load the context from xattrs through ext4_get_context()) and thus DAX would
> still get enabled for such inodes? What am I missing?

Maybe __ext4_new_inode() should set the encryption flag on the inode
earlier?  Currently it decides whether the inode will be encrypted, then
calls ext4_set_inode_flags(), then calls fscrypt_set_context() (and thus
ext4_set_context()).  That last step sets the flag and calls
ext4_set_inode_flags() *again*.

I don't remember if there's a reason for that redundancy, but it might
just be for historical reasons.  ext4_set_context() is called in two
different cases: when a new inode is created and it's encrypted from the
beginning, and when FS_IOC_SET_ENCRYPTION_POLICY is executed on an empty
directory.  The latter does have to set the encrypt flag there.
 
- Eric