Re: [PATCH 2/2] btrfs: do not allow compression on nodatacow files

[Filipe Manana <fdmanana@kernel.org>]

On Fri, Apr 15, 2022 at 04:04:06PM +0800, Chung-Chiang Cheng wrote:
> Compression and nodatacow are mutually exclusive. A similar issue was
> fixed by commit f37c563bab429 ("btrfs: add missing check for nocow and
> compression inode flags"). Besides ioctl, there is another way to
> enable/disable/reset compression directly via xattr. The following
> steps will result in a invalid combination.
> 
>   $ touch bar
>   $ chattr +C bar
>   $ lsattr bar
>   ---------------C-- bar
>   $ setfattr -n btrfs.compression -v zstd bar
>   $ lsattr bar
>   --------c------C-- bar
> 
> To align with the logic in check_fsflags, nocompress will also be
> unacceptable after this patch, to prevent mix any compression-related
> options with nodatacow.
> 
>   $ touch bar
>   $ chattr +C bar
>   $ lsattr bar
>   ---------------C-- bar
>   $ setfattr -n btrfs.compression -v zstd bar
>   setfattr: bar: Invalid argument
>   $ setfattr -n btrfs.compression -v no bar
>   setfattr: bar: Invalid argument
> 
> Reported-by: Jayce Lin <jaycelin@synology.com>
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/props.c | 16 +++++++++++-----
>  fs/btrfs/props.h |  3 ++-
>  fs/btrfs/xattr.c |  2 +-
>  3 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 1a6d2d5b4b33..5a6f87744c28 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -17,7 +17,8 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
>  struct prop_handler {
>  	struct hlist_node node;
>  	const char *xattr_name;
> -	int (*validate)(const char *value, size_t len);
> +	int (*validate)(const struct btrfs_inode *inode, const char *value,
> +			size_t len);
>  	int (*apply)(struct inode *inode, const char *value, size_t len);
>  	const char *(*extract)(struct inode *inode);
>  	int inheritable;
> @@ -55,7 +56,8 @@ find_prop_handler(const char *name,
>  	return NULL;
>  }
>  
> -int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
> +int btrfs_validate_prop(const struct btrfs_inode *inode, const char *name,
> +			const char *value, size_t value_len)
>  {
>  	const struct prop_handler *handler;
>  
> @@ -69,7 +71,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
>  	if (value_len == 0)
>  		return 0;
>  
> -	return handler->validate(value, value_len);
> +	return handler->validate(inode, value, value_len);
>  }
>  
>  int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
> @@ -252,8 +254,12 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
>  	return ret;
>  }
>  
> -static int prop_compression_validate(const char *value, size_t len)
> +static int prop_compression_validate(const struct btrfs_inode *inode,
> +				     const char *value, size_t len)
>  {
> +	if (!btrfs_inode_can_compress(inode))
> +		return -EINVAL;
> +
>  	if (!value)
>  		return 0;
>  
> @@ -364,7 +370,7 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>  		 * This is not strictly necessary as the property should be
>  		 * valid, but in case it isn't, don't propagate it further.
>  		 */
> -		ret = h->validate(value, strlen(value));
> +		ret = h->validate(BTRFS_I(inode), value, strlen(value));
>  		if (ret)
>  			continue;
>  
> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
> index 40b2c65b518c..2b2ac15ab788 100644
> --- a/fs/btrfs/props.h
> +++ b/fs/btrfs/props.h
> @@ -13,7 +13,8 @@ void __init btrfs_props_init(void);
>  int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
>  		   const char *name, const char *value, size_t value_len,
>  		   int flags);
> -int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
> +int btrfs_validate_prop(const struct btrfs_inode *inode, const char *name,
> +			const char *value, size_t value_len);
>  
>  int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
>  
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index 99abf41b89b9..9632d0ff2038 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -403,7 +403,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  
>  	name = xattr_full_name(handler, name);
> -	ret = btrfs_validate_prop(name, value, size);
> +	ret = btrfs_validate_prop(BTRFS_I(inode), name, value, size);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.34.1
>