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

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

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 <[email protected]>
SUSE Labs, CR