[PATCH] btrfs: do not allow compression on nocow files

[PATCH] btrfs: do not allow compression on nocow files

[Chung-Chiang Cheng]

Compression and nocow are mutually exclusive. 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
  $ lsattr bar
  ---------------C-- bar
  $ setfattr -n btrfs.compression -v zstd bar
  $ lsattr bar
  --------c------C-- bar

Reported-by: Jayce Lin <jaycelin@synology.com>
Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/btrfs/props.c | 20 ++++++++++++--------
 fs/btrfs/props.h |  3 ++-
 fs/btrfs/xattr.c |  2 +-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 1a6d2d5b4b33..fde2bf069b03 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -17,7 +17,7 @@ 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)(struct 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 +55,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(struct inode *inode, const char *name,
+			const char *value, size_t value_len)
 {
 	const struct prop_handler *handler;
 
@@ -69,7 +70,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,18 +253,21 @@ 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(struct inode *inode, const char *value, size_t len)
 {
 	if (!value)
 		return 0;
 
-	if (btrfs_compress_is_valid_type(value, len))
-		return 0;
-
 	if ((len == 2 && strncmp("no", value, 2) == 0) ||
 	    (len == 4 && strncmp("none", value, 4) == 0))
 		return 0;
 
+	if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW)
+		return -EINVAL;
+
+	if (btrfs_compress_is_valid_type(value, len))
+		return 0;
+
 	return -EINVAL;
 }
 
@@ -364,7 +368,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(inode, value, strlen(value));
 		if (ret)
 			continue;
 
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 40b2c65b518c..0504c03177e3 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(struct 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..9aceae07a02b 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(inode, name, value, size);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH] btrfs: do not allow compression on nocow files

[Filipe Manana]

On Sat, Apr 9, 2022 at 10:14 AM Chung-Chiang Cheng <cccheng@synology.com> wrote:
>
> Compression and nocow are mutually exclusive. 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
>   $ lsattr bar
>   ---------------C-- bar

The example should show how the NOCOW attribute ended up in the file.
Either the filesystem mounted with -o nodatacow or a chattr +C was
done on the file bar before the lsattr command.

>   $ setfattr -n btrfs.compression -v zstd bar
>   $ lsattr bar
>   --------c------C-- bar
>
> Reported-by: Jayce Lin <jaycelin@synology.com>
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>

The changelog should mention what happens if we end up both with
compression and nodatacow set on a file.
Will all future writes be compressed and nodatacow have no effect? Or
will compression have no effect and we nodatacow overrides it?
Or something else? Some corruption, some crash, etc?

> ---
>  fs/btrfs/props.c | 20 ++++++++++++--------
>  fs/btrfs/props.h |  3 ++-
>  fs/btrfs/xattr.c |  2 +-
>  3 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 1a6d2d5b4b33..fde2bf069b03 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -17,7 +17,7 @@ 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)(struct 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 +55,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(struct inode *inode, const char *name,

The inode can be made const.
The validation handler should never need to change anything in the inode.

Also, we can use struct btrfs_inode * instead of struct inode (preferred).

> +                       const char *value, size_t value_len)
>  {
>         const struct prop_handler *handler;
>
> @@ -69,7 +70,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,18 +253,21 @@ 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(struct inode *inode, const char *value, size_t len)

Same here, about const and using struct btrfs_inode * instead.

>  {
>         if (!value)
>                 return 0;
>
> -       if (btrfs_compress_is_valid_type(value, len))
> -               return 0;
> -
>         if ((len == 2 && strncmp("no", value, 2) == 0) ||
>             (len == 4 && strncmp("none", value, 4) == 0))
>                 return 0;
>
> +       if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW)

How can inode ever be NULL?
Unless I missed something, it can't be NULL.

We should also get a test case for fstests, to help prevent
regressions in the future.

Otherwise it looks fine.

Thanks.




> +               return -EINVAL;
> +
> +       if (btrfs_compress_is_valid_type(value, len))
> +               return 0;
> +
>         return -EINVAL;
>  }
>
> @@ -364,7 +368,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(inode, value, strlen(value));
>                 if (ret)
>                         continue;
>
> diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
> index 40b2c65b518c..0504c03177e3 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(struct 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..9aceae07a02b 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(inode, name, value, size);
>         if (ret)
>                 return ret;
>
> --
> 2.34.1
>

Re: [PATCH] btrfs: do not allow compression on nocow files

[David Sterba]

On Sat, Apr 09, 2022 at 12:34:32PM +0800, Chung-Chiang Cheng wrote:
> -static int prop_compression_validate(const char *value, size_t len)
> +static int prop_compression_validate(struct inode *inode, const char *value, size_t len)
>  {
>  	if (!value)
>  		return 0;
>  
> -	if (btrfs_compress_is_valid_type(value, len))
> -		return 0;
> -
>  	if ((len == 2 && strncmp("no", value, 2) == 0) ||
>  	    (len == 4 && strncmp("none", value, 4) == 0))
>  		return 0;
>  
> +	if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW)
> +		return -EINVAL;

I think the nodatacow check should be the first one, before the
validation of value for "no" or "none", it's logically the same as the
btrfs_compress_is_valid_type.

Also, should there be a check for NODATASUM? The checks in the property
should do the same as the normal compression code, see eg.
inode_can_compress. It's an internal helper so it would be cleaner to
have it exported and reuse here instead of duplicatin the conditions.

For a minimal fix I'd be OK with the code duplication as this would need
to go to stable, so "fixes before cleanups".

Re: [PATCH] btrfs: do not allow compression on nocow files

[Chung-Chiang Cheng]

> > +       if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW)
>
> How can inode ever be NULL?
> Unless I missed something, it can't be NULL.
>
> We should also get a test case for fstests, to help prevent
> regressions in the future.
>
> Otherwise it looks fine.

Thanks for your comments. I will send a v2 patch and a fstest item
later. And as David mentioned, inode_can_compress() can be reused to
reduce duplication.

Thanks.

Re: [PATCH] btrfs: do not allow compression on nocow files

[Chung-Chiang Cheng]

On Mon, Apr 11, 2022 at 10:27 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Sat, Apr 09, 2022 at 12:34:32PM +0800, Chung-Chiang Cheng wrote:
> > -static int prop_compression_validate(const char *value, size_t len)
> > +static int prop_compression_validate(struct inode *inode, const char *value, size_t len)
> >  {
> >       if (!value)
> >               return 0;
> >
> > -     if (btrfs_compress_is_valid_type(value, len))
> > -             return 0;
> > -
> >       if ((len == 2 && strncmp("no", value, 2) == 0) ||
> >           (len == 4 && strncmp("none", value, 4) == 0))
> >               return 0;
> >
> > +     if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW)
> > +             return -EINVAL;
>
> I think the nodatacow check should be the first one, before the
> validation of value for "no" or "none", it's logically the same as the
> btrfs_compress_is_valid_type.

if this check is located before the validation of value for no/none, the
following operation isn't allowed. is it ok? although it makes no sense,
it doesn't produce any invalid combination.

      $ touch bar
      $ chattr +C bar
      $ lsattr bar
      ---------------C-- bar
      $ setfattr -n btrfs.compression -v no bar
      setfattr: bar: Invalid argument

Thanks.