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

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

[Ojaswin Mujoo]

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);
 
 	/*
 	 * Encrypting the root directory is not allowed because e2fsck expects
@@ -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

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

[Disha Goel]

On 23/07/26 2:26 pm, 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>
> ---

Thanks for the fix patch Ojaswin. This fixes the issue I reported.

Tested on a -o dax=always ext4 filesystem on ppc64le with the following
xfstests configuration:

FSTYP=ext4
MOUNT_OPTIONS="-o block_validity,dax"
MKFS_OPTIONS="-b 65536"

All failing encryption tests in the generic group now pass.

Tested-by: Disha Goel <disgoel@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);
>   
>   	/*
>   	 * Encrypting the root directory is not allowed because e2fsck expects
> @@ -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;

-- 
Regards,
Disha

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

[Eric Biggers]

On Thu, Jul 23, 2026 at 02:26:48PM +0530, 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>

Acked-by: Eric Biggers <ebiggers@kernel.org>

I'm glad I wrote those tests like generic/548, which verify that files
are actually being encrypted on-disk...

Looking for other potential encryption bypasses in ext4, I think
ext4_iomap_swap_activate() needs a fix as well to reject encrypted files
(perhaps in the caller).  That would be a separate patch though.

- Eric

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

[Jan Kara]

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?

								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@suse.com>
SUSE Labs, CR

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

[Eric Biggers]

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

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

[Eric Biggers]

On Mon, Jul 27, 2026 at 07:43:50PM +0000, Eric Biggers 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@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

Here's an idea.  I haven't tested it though.

Note that this would align ext4 closer with f2fs, which already sets the
encrypt flag before it calls fscrypt_set_context().

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index f41f320f4437..c4bfe18fd527 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -144,7 +144,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 (WARN_ON_ONCE(IS_DAX(inode)))
 		return -EINVAL;
 
 	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))
@@ -163,6 +163,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	 */
 
 	if (handle) {
+		/*
+		 * ext4 should have set the encrypt flag on the inode already,
+		 * and it should not have enabled inline data.
+		 */
+		if (WARN_ON_ONCE(!IS_ENCRYPTED(inode)))
+			return -EINVAL;
+		if (WARN_ON_ONCE(ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)))
+			return -EINVAL;
 		/*
 		 * Since the inode is new it is ok to pass the
 		 * XATTR_CREATE flag. This is necessary to match the
@@ -170,21 +178,10 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 		 * function with the credits allocated for the new
 		 * inode.
 		 */
-		res = ext4_xattr_set_handle(handle, inode,
-					    EXT4_XATTR_INDEX_ENCRYPTION,
-					    EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
-					    ctx, len, XATTR_CREATE);
-		if (!res) {
-			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, false);
-		}
-		return res;
+		return ext4_xattr_set_handle(handle, inode,
+					     EXT4_XATTR_INDEX_ENCRYPTION,
+					     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
+					     ctx, len, XATTR_CREATE);
 	}
 
 	res = dquot_initialize(inode);
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index a40cb27f8116..3848244af77a 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -997,6 +997,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
 		if (err)
 			goto out;
+		if (encrypt)
+			i_flags |= EXT4_ENCRYPT_FL;
 	}
 
 	err = dquot_initialize(inode);
@@ -1306,6 +1308,7 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 	ei->i_extra_isize = sbi->s_want_extra_isize;
 	ei->i_inline_off = 0;
 	if (ext4_has_feature_inline_data(sb) &&
+	    !(ei->i_flags & EXT4_ENCRYPT_FL) &&
 	    (!(ei->i_flags & (EXT4_DAX_FL|EXT4_EA_INODE_FL)) || S_ISDIR(mode)))
 		ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
 	ret = inode;

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

[Jan Kara]

On Mon 27-07-26 20:13:39, Eric Biggers wrote:
> On Mon, Jul 27, 2026 at 07:43:50PM +0000, Eric Biggers 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@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
> 
> Here's an idea.  I haven't tested it though.
> 
> Note that this would align ext4 closer with f2fs, which already sets the
> encrypt flag before it calls fscrypt_set_context().

Thanks! This definitely looks much more obvious than Ojaswin's fix.
Ojaswin, does this fix work for you?

								Honza

> 
> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> index f41f320f4437..c4bfe18fd527 100644
> --- a/fs/ext4/crypto.c
> +++ b/fs/ext4/crypto.c
> @@ -144,7 +144,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 (WARN_ON_ONCE(IS_DAX(inode)))
>  		return -EINVAL;
>  
>  	if (ext4_test_inode_flag(inode, EXT4_INODE_DAX))
> @@ -163,6 +163,14 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
>  	 */
>  
>  	if (handle) {
> +		/*
> +		 * ext4 should have set the encrypt flag on the inode already,
> +		 * and it should not have enabled inline data.
> +		 */
> +		if (WARN_ON_ONCE(!IS_ENCRYPTED(inode)))
> +			return -EINVAL;
> +		if (WARN_ON_ONCE(ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)))
> +			return -EINVAL;
>  		/*
>  		 * Since the inode is new it is ok to pass the
>  		 * XATTR_CREATE flag. This is necessary to match the
> @@ -170,21 +178,10 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
>  		 * function with the credits allocated for the new
>  		 * inode.
>  		 */
> -		res = ext4_xattr_set_handle(handle, inode,
> -					    EXT4_XATTR_INDEX_ENCRYPTION,
> -					    EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> -					    ctx, len, XATTR_CREATE);
> -		if (!res) {
> -			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, false);
> -		}
> -		return res;
> +		return ext4_xattr_set_handle(handle, inode,
> +					     EXT4_XATTR_INDEX_ENCRYPTION,
> +					     EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> +					     ctx, len, XATTR_CREATE);
>  	}
>  
>  	res = dquot_initialize(inode);
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index a40cb27f8116..3848244af77a 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -997,6 +997,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>  		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
>  		if (err)
>  			goto out;
> +		if (encrypt)
> +			i_flags |= EXT4_ENCRYPT_FL;
>  	}
>  
>  	err = dquot_initialize(inode);
> @@ -1306,6 +1308,7 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>  	ei->i_extra_isize = sbi->s_want_extra_isize;
>  	ei->i_inline_off = 0;
>  	if (ext4_has_feature_inline_data(sb) &&
> +	    !(ei->i_flags & EXT4_ENCRYPT_FL) &&
>  	    (!(ei->i_flags & (EXT4_DAX_FL|EXT4_EA_INODE_FL)) || S_ISDIR(mode)))
>  		ext4_set_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
>  	ret = inode;
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR