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

[Ojaswin Mujoo]

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?

> 
> 								Honza
> 
> > @@ -179,10 +180,16 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> >  			ext4_clear_inode_state(inode,
> >  					EXT4_STATE_MAY_INLINE_DATA);
> >  			/*
> > -			 * Update inode->i_flags - S_ENCRYPTED will be enabled,
> > -			 * S_DAX may be disabled
> > +			 * Update inode->i_flags, S_ENCRYPTED will be enabled.
> > +			 * If this is a regular inode, then we must be coming
> > +			 * via __ext4_new_inode() as only new inodes can be
> > +			 * encrypted, so we must set the init flag so
> > +			 * S_DAX can be disabled. This is a bit fragile but
> > +			 * seems like the easiest way to make sure we don't let
> > +			 * the DAX flag linger when encryption is enabled as
> > +			 * that result in writes silently bypassing encryption.
> >  			 */
> > -			ext4_set_inode_flags(inode, false);
> > +			ext4_set_inode_flags(inode, init);
> >  		}
> >  		return res;
> >  	}
> > @@ -209,7 +216,7 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> >  		 * Update inode->i_flags - S_ENCRYPTED will be enabled,
> >  		 * S_DAX may be disabled
> >  		 */
> > -		ext4_set_inode_flags(inode, false);
> > +		ext4_set_inode_flags(inode, init);
> >  		res = ext4_mark_inode_dirty(handle, inode);
> >  		if (res)
> >  			EXT4_ERROR_INODE(inode, "Failed to mark inode dirty");
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index ce99807c5f5b..a57179655353 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -5113,8 +5113,6 @@ void ext4_set_inode_flags(struct inode *inode, bool init)
> >  	unsigned int flags = EXT4_I(inode)->i_flags;
> >  	unsigned int new_fl = 0;
> >  
> > -	WARN_ON_ONCE(IS_DAX(inode) && init);
> > -
> >  	if (flags & EXT4_SYNC_FL)
> >  		new_fl |= S_SYNC;
> >  	if (flags & EXT4_APPEND_FL)
> > @@ -5129,8 +5127,12 @@ void ext4_set_inode_flags(struct inode *inode, bool init)
> >  	/* Because of the way inode_set_flags() works we must preserve S_DAX
> >  	 * here if already set. */
> >  	new_fl |= (inode->i_flags & S_DAX);
> > -	if (init && ext4_should_enable_dax(inode))
> > -		new_fl |= S_DAX;
> > +	if (init) {
> > +		if (ext4_should_enable_dax(inode))
> > +			new_fl |= S_DAX;
> > +		else
> > +			new_fl &= ~S_DAX;
> > +	}
> >  
> >  	if (flags & EXT4_ENCRYPT_FL)
> >  		new_fl |= S_ENCRYPTED;
> > -- 
> > 2.53.0
> > 
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR