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

[Jan Kara]

On Wed 29-07-26 00:47:03, Ojaswin Mujoo wrote:
> 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@xxxxxxxxxxxxx>
> > > Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> > > ---
> > >  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.
> 
> Hi Jan, thanks for the review. 
> Yes I agree the init naming and checking for ISREG is a bit hacky.
> > 
> > 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?
> 
> Hmm do you mean __ext4_iget() on an already encrypted inode? In that
> case raw_inode->i_flags with EXT4_ENCRYPT_FL set will get carried over
> to ei->i_flags and we will still get the right aops no? Or do you mean
> some other case?

Yes, I've realized that today when reading Eric's patch and checking how
f2fs works. But thanks for confirmation anyway :)

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR