[PATCH 0/4] btrfs: fix property bugs

[PATCH 0/4] btrfs: fix property bugs

[Anand Jain]

1/4 Fixes a bug that we were applying an incomplete string zstd.
2/4 Fixes a bug that the compression property gets reset to NULL after a
    failed attempt to change the compression parameter on the object.
3/4 preparatory patch so that btrfs_set_prop_trans() can be merged with
    btrfs_set_prop() in 4/4.
4/4 Fixes a bug that we were incrementing the generation even when the
    property validation check failed.

3/4 and 4/4 are only added here.

Anand Jain (4):
  btrfs: fix zstd compression parameter
  btrfs: fix vanished compression property after failed set
  btrfs: open code btrfs_set_prop in inherit_prop
  btrfs: fix property validate fail should not increment generation

 fs/btrfs/ioctl.c | 10 +++---
 fs/btrfs/props.c | 94 ++++++++++++++++++++++++++++++--------------------------
 fs/btrfs/props.h |  4 +--
 fs/btrfs/xattr.c |  2 +-
 4 files changed, 58 insertions(+), 52 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] btrfs: fix zstd compression parameter

[Anand Jain]

We let to pass zstd compression parameter even if its not fully written.
For example:
  btrfs prop set /btrfs compression zst
  btrfs prop get /btrfs compression
     compression=zst

zlib and lzo are fine.

Fix it by using the expected number of char in strncmp().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/props.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f1a8be67f639..3cc007e3c7f8 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -304,7 +304,7 @@ static int prop_compression_apply(struct inode *inode, const char *value,
 		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
 	} else if (!strncmp("zlib", value, 4)) {
 		type = BTRFS_COMPRESS_ZLIB;
-	} else if (!strncmp("zstd", value, len)) {
+	} else if (!strncmp("zstd", value, 4)) {
 		type = BTRFS_COMPRESS_ZSTD;
 		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
 	} else {
-- 
1.8.3.1

[PATCH 2/4] btrfs: fix vanished compression property after failed set

[Anand Jain]

The compression property resets to NULL, instead of the old value if we
fail to set the new compression parameter.

btrfs prop get /btrfs compression
  compression=lzo
btrfs prop set /btrfs compression zli
  ERROR: failed to set compression for /btrfs: Invalid argument
btrfs prop get /btrfs compression

This is because the compression property ->validate() is successful for
'zli' as the strncmp() used the len passed from the userland.

Fix it by using the expected string length in strncmp().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/props.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 3cc007e3c7f8..72a06c4d3c70 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -275,11 +275,11 @@ static int prop_compression_validate(const char *value, size_t len)
 	if (!value)
 		return 0;
 
-	if (!strncmp("lzo", value, len))
+	if (!strncmp("lzo", value, 3))
 		return 0;
-	else if (!strncmp("zlib", value, len))
+	else if (!strncmp("zlib", value, 4))
 		return 0;
-	else if (!strncmp("zstd", value, len))
+	else if (!strncmp("zstd", value, 4))
 		return 0;
 
 	return -EINVAL;
-- 
1.8.3.1

Re: [PATCH 2/4] btrfs: fix vanished compression property after failed set

[Nikolay Borisov]

On 14.03.19 г. 7:05 ч., Anand Jain wrote:
> The compression property resets to NULL, instead of the old value if we
> fail to set the new compression parameter.
> 
> btrfs prop get /btrfs compression
>   compression=lzo
> btrfs prop set /btrfs compression zli
>   ERROR: failed to set compression for /btrfs: Invalid argument
> btrfs prop get /btrfs compression
> 
> This is because the compression property ->validate() is successful for
> 'zli' as the strncmp() used the len passed from the userland.
> 
> Fix it by using the expected string length in strncmp().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

The changelog could be a bit clearer. The failure is due to the fact
that ->validate succeeds but ->apply fails and the handling code calls

	btrfs_setxattr(trans, inode, handler->xattr_name,
              NULL, 0, flags);

However, if you make this patch be the first in the series you obsolete
the existing patch 1 and fix the issue in this one. I.e 2 birds with 1
stone.

Also I believe btrfs/048 could use some extension in the part where it
validates compression properties to test for the bugs discovered/fixed
in this series.

> ---
>  fs/btrfs/props.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 3cc007e3c7f8..72a06c4d3c70 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -275,11 +275,11 @@ static int prop_compression_validate(const char *value, size_t len)
>  	if (!value)
>  		return 0;
>  
> -	if (!strncmp("lzo", value, len))
> +	if (!strncmp("lzo", value, 3))
>  		return 0;
> -	else if (!strncmp("zlib", value, len))
> +	else if (!strncmp("zlib", value, 4))
>  		return 0;
> -	else if (!strncmp("zstd", value, len))
> +	else if (!strncmp("zstd", value, 4))
>  		return 0;
>  
>  	return -EINVAL;
> 

Re: [PATCH 2/4] btrfs: fix vanished compression property after failed set

[Anand Jain]

On 3/14/19 4:32 PM, Nikolay Borisov wrote:
> 
> 
> On 14.03.19 г. 7:05 ч., Anand Jain wrote:
>> The compression property resets to NULL, instead of the old value if we
>> fail to set the new compression parameter.
>>
>> btrfs prop get /btrfs compression
>>    compression=lzo
>> btrfs prop set /btrfs compression zli
>>    ERROR: failed to set compression for /btrfs: Invalid argument
>> btrfs prop get /btrfs compression
>>
>> This is because the compression property ->validate() is successful for
>> 'zli' as the strncmp() used the len passed from the userland.
>>
>> Fix it by using the expected string length in strncmp().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> The changelog could be a bit clearer. The failure is due to the fact
> that ->validate succeeds but ->apply fails and the handling code calls
> > 	btrfs_setxattr(trans, inode, handler->xattr_name,
>                NULL, 0, flags);
> 
> However, if you make this patch be the first in the series you obsolete
> the existing patch 1 and fix the issue in this one. I.e 2 birds with 1
> stone.

  Right. I wanted to show the bugs, its reason, and fix separately.

> Also I believe btrfs/048 could use some extension in the part where it
> validates compression properties to test for the bugs discovered/fixed
> in this series.

  Oh. Let me try. Thanks.

-Anand

>> ---
>>   fs/btrfs/props.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index 3cc007e3c7f8..72a06c4d3c70 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -275,11 +275,11 @@ static int prop_compression_validate(const char *value, size_t len)
>>   	if (!value)
>>   		return 0;
>>   
>> -	if (!strncmp("lzo", value, len))
>> +	if (!strncmp("lzo", value, 3))
>>   		return 0;
>> -	else if (!strncmp("zlib", value, len))
>> +	else if (!strncmp("zlib", value, 4))
>>   		return 0;
>> -	else if (!strncmp("zstd", value, len))
>> +	else if (!strncmp("zstd", value, 4))
>>   		return 0;
>>   
>>   	return -EINVAL;
>>

[PATCH 3/4] btrfs: open code btrfs_set_prop in inherit_prop

[Anand Jain]

When an inode inherits property from its parent, we call btrfs_set_prop().
btrfs_set_prop() does an elaborate checks, which is not required in the
context of inheriting a property. Instead just open-code only the required
items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
now the only user of btrfs_set_prop() is gone, (except for the wraper
function btrfs_set_prop_trans()).

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/props.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 72a06c4d3c70..fb84e76f3b1d 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		if (!value)
 			continue;
 
+		/* may be removed */
+		ret = h->validate(value, strlen(value));
+		if (ret)
+			continue;
+
 		num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
 		ret = btrfs_block_rsv_add(root, trans->block_rsv,
 					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
 		if (ret)
-			goto out;
-		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,
+			return ret;
+
+		ret = btrfs_setxattr(trans, inode, h->xattr_name, value,
 				     strlen(value), 0);
+		if (!ret) {
+			ret = h->apply(inode, value, strlen(value));
+			if (ret)
+				btrfs_setxattr(trans, inode, h->xattr_name,
+					       NULL, 0, 0);
+			else
+				set_bit(BTRFS_INODE_HAS_PROPS,
+					&BTRFS_I(inode)->runtime_flags);
+		}
+
 		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);
 		if (ret)
-			goto out;
+			return ret;
 	}
-	ret = 0;
-out:
-	return ret;
+
+	return 0;
 }
 
 int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
-- 
1.8.3.1

Re: [PATCH 3/4] btrfs: open code btrfs_set_prop in inherit_prop

[Nikolay Borisov]

On 14.03.19 г. 7:05 ч., Anand Jain wrote:
> When an inode inherits property from its parent, we call btrfs_set_prop().
> btrfs_set_prop() does an elaborate checks, which is not required in the
> context of inheriting a property. Instead just open-code only the required
> items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
> now the only user of btrfs_set_prop() is gone, (except for the wraper
> function btrfs_set_prop_trans()).

Um, no:

git grep -P '(?<!__)btrfs_set_prop' fs/btrfs/
fs/btrfs/ioctl.c:               ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
fs/btrfs/ioctl.c:               ret = btrfs_set_prop(inode, "btrfs.compression",
fs/btrfs/ioctl.c:               ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
fs/btrfs/props.c:int btrfs_set_prop(struct inode *inode,
fs/btrfs/props.h:int btrfs_set_prop(struct inode *inode,
fs/btrfs/xattr.c:       return btrfs_set_prop(inode, name, value, size, flags);


Rebase the patch on misc-next and remove the last sentence 
since it's factually wrong.

> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/props.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 72a06c4d3c70..fb84e76f3b1d 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>  		if (!value)
>  			continue;
>  
> +		/* may be removed */
> +		ret = h->validate(value, strlen(value));
> +		if (ret)
> +			continue;
> +
>  		num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
>  		ret = btrfs_block_rsv_add(root, trans->block_rsv,
>  					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
>  		if (ret)
> -			goto out;
> -		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,

What branch is this code based on, currently misc-next is using (and 
has been since 2014) __btrfs_set_prop yet you remove btrfs_set_prop?

> +			return ret;
> +
> +		ret = btrfs_setxattr(trans, inode, h->xattr_name, value,
>  				     strlen(value), 0);
> +		if (!ret) {
> +			ret = h->apply(inode, value, strlen(value));
> +			if (ret)
> +				btrfs_setxattr(trans, inode, h->xattr_name,
> +					       NULL, 0, 0);
> +			else
> +				set_bit(BTRFS_INODE_HAS_PROPS,
> +					&BTRFS_I(inode)->runtime_flags);
> +		}
> +
>  		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);
>  		if (ret)
> -			goto out;
> +			return ret;
>  	}
> -	ret = 0;
> -out:
> -	return ret;
> +
> +	return 0;
>  }
>  
>  int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
> 

Re: [PATCH 3/4] btrfs: open code btrfs_set_prop in inherit_prop

[Anand Jain]

On 3/14/19 4:57 PM, Nikolay Borisov wrote:
> 
> 
> On 14.03.19 г. 7:05 ч., Anand Jain wrote:
>> When an inode inherits property from its parent, we call btrfs_set_prop().
>> btrfs_set_prop() does an elaborate checks, which is not required in the
>> context of inheriting a property. Instead just open-code only the required
>> items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
>> now the only user of btrfs_set_prop() is gone, (except for the wraper
>> function btrfs_set_prop_trans()).
> 
> Um, no:
> 
> git grep -P '(?<!__)btrfs_set_prop' fs/btrfs/
> fs/btrfs/ioctl.c:               ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
> fs/btrfs/ioctl.c:               ret = btrfs_set_prop(inode, "btrfs.compression",
> fs/btrfs/ioctl.c:               ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
> fs/btrfs/props.c:int btrfs_set_prop(struct inode *inode,
> fs/btrfs/props.h:int btrfs_set_prop(struct inode *inode,
> fs/btrfs/xattr.c:       return btrfs_set_prop(inode, name, value, size, flags);
> 
> 
> Rebase the patch on misc-next and remove the last sentence
> since it's factually wrong.

  Oh. This is based on misc-5.2.
  Oops I should have mentioned.

Thanks. Anand

[PATCH 4/4] btrfs: fix property validate fail should not increment generation

[Anand Jain]

When the property fails to pass the prop_handlers::validate() check, the
thread should exit with no changes in the kernel, but as we are starting
the transaction too early, we have just updated the generation even if
there is no change.

For example:
btrfs prop get /btrfs compression
 compression=lzo
sync
btrfs in dump-super /dev/sdb | grep "^generation"
 generation		32

Try to set an incomplete compression type

btrfs prop set /btrfs compression zli
  ERROR: failed to set compression for /btrfs: Invalid argument
sync

Set failed but generation is incremented
btrfs in dump-super /dev/sdb | grep "^generation"
 generation		33  <--

Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which
provides flexibility to start the transaction after the
prop_handlers::validate() has been checked.

As of now if the prop_handlers::apply() fails then we still increment
the generation, but if there is strong prop_handlers::validate() then
the prop_handlers::apply() can never fail.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c | 10 ++++------
 fs/btrfs/props.c | 59 +++++++++++++++++++++++++-------------------------------
 fs/btrfs/props.h |  4 ++--
 fs/btrfs/xattr.c |  2 +-
 4 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 28ee9fe6edb4..0eeaf9a68082 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -284,8 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		binode->flags &= ~BTRFS_INODE_COMPRESS;
 		binode->flags |= BTRFS_INODE_NOCOMPRESS;
 
-		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
-					   0, 0);
+		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
 	} else if (fsflags & FS_COMPR_FL) {
@@ -303,14 +302,13 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		if (!comp || comp[0] == 0)
 			comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB);
 
-		ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp,
-					   strlen(comp), 0);
+		ret = btrfs_set_prop(inode, "btrfs.compression", comp,
+				     strlen(comp), 0);
 		if (ret)
 			goto out_drop;
 
 	} else {
-		ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL,
-					   0, 0);
+		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
 		binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index fb84e76f3b1d..99fa2459ba61 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -56,11 +56,12 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
 	return NULL;
 }
 
-static 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_set_prop(struct inode *inode, const char *name, const char *value,
+		   size_t value_len, int flags)
 {
+	struct btrfs_root *root = BTRFS_I(inode)->root;
 	const struct prop_handler *handler;
+	struct btrfs_trans_handle *trans;
 	int ret;
 
 	if (btrfs_root_readonly(BTRFS_I(inode)->root))
@@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		return -EINVAL;
 
 	if (value_len == 0) {
+		/* Its called to reset the property */
+		trans = btrfs_start_transaction(root, 2);
+		if (IS_ERR(trans))
+			return PTR_ERR(trans);
+
 		ret = btrfs_setxattr(trans, inode, handler->xattr_name,
 				       NULL, 0, flags);
-		if (ret)
-			return ret;
-
-		ret = handler->apply(inode, NULL, 0);
-		ASSERT(ret == 0);
-
-		return ret;
+		if (!ret) {
+			ret = handler->apply(inode, NULL, 0);
+			ASSERT(ret == 0);
+		}
+		goto out;
 	}
 
 	ret = handler->validate(value, value_len);
 	if (ret)
 		return ret;
+
+	trans = btrfs_start_transaction(root, 2);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
 	ret = btrfs_setxattr(trans, inode, handler->xattr_name,
 			       value, value_len, flags);
 	if (ret)
-		return ret;
+		goto out;
+
 	ret = handler->apply(inode, value, value_len);
 	if (ret) {
+		/* Apply failed. Reset the property. */
 		btrfs_setxattr(trans, inode, handler->xattr_name,
 				 NULL, 0, flags);
-		return ret;
-	}
-
-	set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
-
-	return 0;
-}
-
-int btrfs_set_prop_trans(struct inode *inode, const char *name,
-			 const char *value, size_t value_len, int flags)
-{
-	struct btrfs_root *root = BTRFS_I(inode)->root;
-	struct btrfs_trans_handle *trans;
-	int ret;
-
-	trans = btrfs_start_transaction(root, 2);
-	if (IS_ERR(trans))
-		return PTR_ERR(trans);
-
-	ret = btrfs_set_prop(trans, inode, name, value, value_len, flags);
-
-	if (!ret) {
+	} else {
+		set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags);
 		inode_inc_iversion(inode);
 		inode->i_ctime = current_time(inode);
 		set_bit(BTRFS_INODE_COPY_EVERYTHING,
@@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name,
 		ret = btrfs_update_inode(trans, root, inode);
 		BUG_ON(ret);
 	}
+
+out:
 	btrfs_end_transaction(trans);
 	return ret;
 }
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index b1a6b233b774..72bca072f9ba 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -10,8 +10,8 @@
 
 void __init btrfs_props_init(void);
 
-int btrfs_set_prop_trans(struct inode *inode, const char *name,
-			 const char *value, size_t value_len, int flags);
+int btrfs_set_prop(struct inode *inode, const char *name,
+		   const char *value, size_t value_len, int flags);
 
 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 58579a4d2f22..abc150a56f8b 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -371,7 +371,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
 					size_t size, int flags)
 {
 	name = xattr_full_name(handler, name);
-	return btrfs_set_prop_trans(inode, name, value, size, flags);
+	return btrfs_set_prop(inode, name, value, size, flags);
 }
 
 static const struct xattr_handler btrfs_security_xattr_handler = {
-- 
1.8.3.1