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 Signed-off-by: Ojaswin Mujoo --- 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