* [PATCH] ext4: turn off DAX on new files when encryption is set
@ 2026-07-23 8:56 Ojaswin Mujoo
2026-07-27 11:59 ` Disha Goel
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ojaswin Mujoo @ 2026-07-23 8:56 UTC (permalink / raw)
To: linux-ext4, Theodore Ts'o
Cc: Ritesh Harjani, Zhang Yi, linux-kernel, Baokun Li, Jan Kara,
Disha Goel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: turn off DAX on new files when encryption is set 2026-07-23 8:56 [PATCH] ext4: turn off DAX on new files when encryption is set Ojaswin Mujoo @ 2026-07-27 11:59 ` Disha Goel 2026-07-27 19:01 ` Eric Biggers 2026-07-27 19:06 ` Jan Kara 2 siblings, 0 replies; 6+ messages in thread From: Disha Goel @ 2026-07-27 11:59 UTC (permalink / raw) To: Ojaswin Mujoo, linux-ext4, Theodore Ts'o Cc: Ritesh Harjani, Zhang Yi, linux-kernel, Baokun Li, Jan Kara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: turn off DAX on new files when encryption is set 2026-07-23 8:56 [PATCH] ext4: turn off DAX on new files when encryption is set Ojaswin Mujoo 2026-07-27 11:59 ` Disha Goel @ 2026-07-27 19:01 ` Eric Biggers 2026-07-27 19:06 ` Jan Kara 2 siblings, 0 replies; 6+ messages in thread From: Eric Biggers @ 2026-07-27 19:01 UTC (permalink / raw) To: Ojaswin Mujoo Cc: linux-ext4, Theodore Ts'o, Ritesh Harjani, Zhang Yi, linux-kernel, Baokun Li, Jan Kara, Disha Goel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: turn off DAX on new files when encryption is set 2026-07-23 8:56 [PATCH] ext4: turn off DAX on new files when encryption is set Ojaswin Mujoo 2026-07-27 11:59 ` Disha Goel 2026-07-27 19:01 ` Eric Biggers @ 2026-07-27 19:06 ` Jan Kara 2026-07-27 19:43 ` Eric Biggers 2 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2026-07-27 19:06 UTC (permalink / raw) To: Ojaswin Mujoo Cc: linux-ext4, Theodore Ts'o, Ritesh Harjani, Zhang Yi, linux-kernel, Baokun Li, Jan Kara, Disha Goel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: turn off DAX on new files when encryption is set 2026-07-27 19:06 ` Jan Kara @ 2026-07-27 19:43 ` Eric Biggers 2026-07-27 20:13 ` Eric Biggers 0 siblings, 1 reply; 6+ messages in thread From: Eric Biggers @ 2026-07-27 19:43 UTC (permalink / raw) To: Jan Kara Cc: Ojaswin Mujoo, linux-ext4, Theodore Ts'o, Ritesh Harjani, Zhang Yi, linux-kernel, Baokun Li, Disha Goel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: turn off DAX on new files when encryption is set 2026-07-27 19:43 ` Eric Biggers @ 2026-07-27 20:13 ` Eric Biggers 0 siblings, 0 replies; 6+ messages in thread From: Eric Biggers @ 2026-07-27 20:13 UTC (permalink / raw) To: Jan Kara Cc: Ojaswin Mujoo, linux-ext4, Theodore Ts'o, Ritesh Harjani, Zhang Yi, linux-kernel, Baokun Li, Disha Goel 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-27 20:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-23 8:56 [PATCH] ext4: turn off DAX on new files when encryption is set Ojaswin Mujoo 2026-07-27 11:59 ` Disha Goel 2026-07-27 19:01 ` Eric Biggers 2026-07-27 19:06 ` Jan Kara 2026-07-27 19:43 ` Eric Biggers 2026-07-27 20:13 ` Eric Biggers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.