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

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 <[email protected]>
> Signed-off-by: Ojaswin Mujoo <[email protected]>
> ---

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 <[email protected]>

>   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