[PATCH 0/4] Some random fuzz test fixes

[PATCH 0/4] Some random fuzz test fixes

[Qu Wenruo]

Can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/fuzzed_enhance

Some random bugs exposed by the ill-fated fuzz-test (003 never seems to
pass).

It will be a big work to make fuzz/003 to pass, but at least there are
some easy fixes.

Qu Wenruo (4):
  btrfs-progs: check: Remove the ability to rebuild root overwritting
    existing tree blocks
  btrfs-progs: transaction: Error out other than panic when committing
    transaction
  btrfs-progs: check/original: Avoid infinite loop when failed to repair
    inode
  btrfs-progs: check/original: Don't overwrite return value when we
    failed to repair

 check/main.c  | 46 +++++++++++++++++++++++++---------------------
 transaction.c | 19 +++++++++++++------
 2 files changed, 38 insertions(+), 27 deletions(-)

-- 
2.18.0

[PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

[Qu Wenruo]

We have function btrfs_fsck_reinit_root() to reinit csum or extent tree.
However this function allows us to let it overwrite existing tree blocks
using @overwrite parameter.

Such behavior is pretty dangerous while no caller is using this feature
explicitly.

So just remove @overwrite parameter and allow btrfs_fsck_reinit_root()
to error out when it fails to allocate tree block.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/check/main.c b/check/main.c
index 8db300abb825..c8c347236543 100644
--- a/check/main.c
+++ b/check/main.c
@@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info)
 }
 
 static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
-			   struct btrfs_root *root, int overwrite)
+				  struct btrfs_root *root)
 {
 	struct extent_buffer *c;
 	struct extent_buffer *old = root->node;
@@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
 
 	level = 0;
 
-	if (overwrite) {
-		c = old;
-		extent_buffer_get(c);
-		goto init;
-	}
 	c = btrfs_alloc_free_block(trans, root,
 				   root->fs_info->nodesize,
 				   root->root_key.objectid,
 				   &disk_key, level, 0, 0);
-	if (IS_ERR(c)) {
-		c = old;
-		extent_buffer_get(c);
-		overwrite = 1;
-	}
-init:
+	if (IS_ERR(c))
+		return PTR_ERR(c);
+
 	memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header));
 	btrfs_set_header_level(c, level);
 	btrfs_set_header_bytenr(c, c->start);
@@ -8422,9 +8414,7 @@ init:
 	/*
 	 * this case can happen in the following case:
 	 *
-	 * 1.overwrite previous root.
-	 *
-	 * 2.reinit reloc data root, this is because we skip pin
+	 * reinit reloc data root, this is because we skip pin
 	 * down reloc data tree before which means we can allocate
 	 * same block bytenr here.
 	 */
@@ -8609,7 +8599,7 @@ reinit_data_reloc:
 		goto out;
 	}
 	record_root_in_trans(trans, root);
-	ret = btrfs_fsck_reinit_root(trans, root, 0);
+	ret = btrfs_fsck_reinit_root(trans, root);
 	if (ret)
 		goto out;
 	ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID);
@@ -8675,7 +8665,7 @@ again:
 	}
 
 	/* Ok we can allocate now, reinit the extent root */
-	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0);
+	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root);
 	if (ret) {
 		fprintf(stderr, "extent root initialization failed\n");
 		/*
@@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv)
 
 		if (init_csum_tree) {
 			printf("Reinitialize checksum tree\n");
-			ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0);
+			ret = btrfs_fsck_reinit_root(trans, info->csum_root);
 			if (ret) {
 				error("checksum tree initialization failed: %d",
 						ret);
-- 
2.18.0

[PATCH 2/4] btrfs-progs: transaction: Error out other than panic when committing transaction

[Qu Wenruo]

There are cases that btrfs_commit_transaction() itself can fail, mostly
due to ENOSPC when allocating space.

Don't panic out in this case.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 transaction.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/transaction.c b/transaction.c
index 9619265ef6e8..b82d346b52c8 100644
--- a/transaction.c
+++ b/transaction.c
@@ -73,7 +73,8 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
 		ret = btrfs_update_root(trans, tree_root,
 					&root->root_key,
 					&root->root_item);
-		BUG_ON(ret);
+		if (ret < 0)
+			return ret;
 		btrfs_write_dirty_block_groups(trans, root);
 	}
 	return 0;
@@ -101,9 +102,11 @@ int commit_tree_roots(struct btrfs_trans_handle *trans,
 		next = fs_info->dirty_cowonly_roots.next;
 		list_del_init(next);
 		root = list_entry(next, struct btrfs_root, dirty_list);
-		update_cowonly_root(trans, root);
+		ret = update_cowonly_root(trans, root);
 		free_extent_buffer(root->commit_root);
 		root->commit_root = NULL;
+		if (ret < 0)
+			return ret;
 	}
 
 	return 0;
@@ -162,12 +165,15 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	root->root_item.level = btrfs_header_level(root->node);
 	ret = btrfs_update_root(trans, root->fs_info->tree_root,
 				&root->root_key, &root->root_item);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto out;
 commit_tree:
 	ret = commit_tree_roots(trans, fs_info);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto out;
 	ret = __commit_transaction(trans, root);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto out;
 	write_ctree_super(trans);
 	btrfs_finish_extent_commit(trans, fs_info->extent_root,
 			           &fs_info->pinned_extents);
@@ -176,7 +182,8 @@ commit_tree:
 	root->commit_root = NULL;
 	fs_info->running_transaction = NULL;
 	fs_info->last_trans_committed = transid;
-	return 0;
+out:
+	return ret;
 }
 
 void btrfs_abort_transaction(struct btrfs_trans_handle *trans, int error)
-- 
2.18.0

[PATCH 3/4] btrfs-progs: check/original: Avoid infinite loop when failed to repair inode

[Qu Wenruo]

Exposed by fuzz-tests/003-multi-check-unmounted/ on fuzzed image
bko-161811.raw.xz.

It's caused by the fact when check_fs_roots() finds tree root is
modified, it re-search tree root by goto again: tag.
However again: tag will also reset root objectid to 0.
If we failed to repair one fs root but still modified tree root, we will
go into such infinite loop.

Fix it by record which root we should skip for repair mode.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/check/main.c b/check/main.c
index c8c347236543..2b5abb2d025b 100644
--- a/check/main.c
+++ b/check/main.c
@@ -3380,6 +3380,7 @@ static int check_fs_roots(struct btrfs_fs_info *fs_info,
 	struct extent_buffer *leaf, *tree_node;
 	struct btrfs_root *tmp_root;
 	struct btrfs_root *tree_root = fs_info->tree_root;
+	u64 skip_root = 0;
 	int ret;
 	int err = 0;
 
@@ -3400,7 +3401,10 @@ static int check_fs_roots(struct btrfs_fs_info *fs_info,
 
 again:
 	key.offset = 0;
-	key.objectid = 0;
+	if (skip_root)
+		key.objectid = skip_root + 1;
+	else
+		key.objectid = 0;
 	key.type = BTRFS_ROOT_ITEM_KEY;
 	ret = btrfs_search_slot(NULL, tree_root, &key, &path, 0, 0);
 	if (ret < 0) {
@@ -3409,6 +3413,7 @@ again:
 	}
 	tree_node = tree_root->node;
 	while (1) {
+
 		if (tree_node != tree_root->node) {
 			free_root_recs_tree(root_cache);
 			btrfs_release_path(&path);
@@ -3445,8 +3450,18 @@ again:
 				btrfs_release_path(&path);
 				goto again;
 			}
-			if (ret)
+			if (ret) {
 				err = 1;
+
+				/*
+				 * We failed to repair this root but modified tree
+				 * root, after again: tag we will still hit this
+				 * root and fail to repair, must skip this root to
+				 * avoid infinite loop
+				 */
+				if (repair)
+					skip_root = key.objectid;
+			}
 			if (key.objectid == BTRFS_TREE_RELOC_OBJECTID)
 				btrfs_free_fs_root(tmp_root);
 		} else if (key.type == BTRFS_ROOT_REF_KEY ||
-- 
2.18.0

[PATCH 4/4] btrfs-progs: check/original: Don't overwrite return value when we failed to repair

[Qu Wenruo]

In check_inode_recs(), for repair mode we always reset @ret to 0.
It makes no sense and later we check@ret to determine if the repair is
successful.

Fix it by removing the offending overwrite.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/check/main.c b/check/main.c
index 2b5abb2d025b..08476968aaba 100644
--- a/check/main.c
+++ b/check/main.c
@@ -2764,7 +2764,6 @@ static int check_inode_recs(struct btrfs_root *root,
 				free_inode_rec(rec);
 				continue;
 			}
-			ret = 0;
 		}
 
 		if (!(repair && ret == 0))
-- 
2.18.0

RE: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3721 bytes --]



> -----Original Message-----
> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo
> Sent: Thursday, July 05, 2018 3:37 PM
> To: linux-btrfs@vger.kernel.org
> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks
> 
> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree.

Nit£º
or fs/file tree

> However this function allows us to let it overwrite existing tree blocks
> using @overwrite parameter.
> 
> Such behavior is pretty dangerous while no caller is using this feature
> explicitly.
> 
> So just remove @overwrite parameter and allow btrfs_fsck_reinit_root()
> to error out when it fails to allocate tree block.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/main.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 8db300abb825..c8c347236543 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info)
>  }
> 
>  static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
> -			   struct btrfs_root *root, int overwrite)
> +				  struct btrfs_root *root)
>  {
>  	struct extent_buffer *c;
>  	struct extent_buffer *old = root->node;
> @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
> 
>  	level = 0;
> 
> -	if (overwrite) {
> -		c = old;
> -		extent_buffer_get(c);
> -		goto init;
> -	}
>  	c = btrfs_alloc_free_block(trans, root,
>  				   root->fs_info->nodesize,
>  				   root->root_key.objectid,
>  				   &disk_key, level, 0, 0);
> -	if (IS_ERR(c)) {
> -		c = old;
> -		extent_buffer_get(c);
> -		overwrite = 1;
> -	}
> -init:
> +	if (IS_ERR(c))
> +		return PTR_ERR(c);
> +
>  	memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header));
>  	btrfs_set_header_level(c, level);
>  	btrfs_set_header_bytenr(c, c->start);
> @@ -8422,9 +8414,7 @@ init:
>  	/*
>  	 * this case can happen in the following case:
>  	 *
> -	 * 1.overwrite previous root.
> -	 *
> -	 * 2.reinit reloc data root, this is because we skip pin
> +	 * reinit reloc data root, this is because we skip pin
>  	 * down reloc data tree before which means we can allocate
>  	 * same block bytenr here.
>  	 */
> @@ -8609,7 +8599,7 @@ reinit_data_reloc:
>  		goto out;
>  	}
>  	record_root_in_trans(trans, root);
> -	ret = btrfs_fsck_reinit_root(trans, root, 0);
> +	ret = btrfs_fsck_reinit_root(trans, root);
>  	if (ret)
>  		goto out;
>  	ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID);
> @@ -8675,7 +8665,7 @@ again:
>  	}
> 
>  	/* Ok we can allocate now, reinit the extent root */
> -	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0);
> +	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root);
>  	if (ret) {
>  		fprintf(stderr, "extent root initialization failed\n");
>  		/*
> @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv)
> 
>  		if (init_csum_tree) {
>  			printf("Reinitialize checksum tree\n");
> -			ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0);
> +			ret = btrfs_fsck_reinit_root(trans, info->csum_root);
>  			if (ret) {
>  				error("checksum tree initialization failed: %d",
>  						ret);
> --

Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com>


> 2.18.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

RE: [PATCH 2/4] btrfs-progs: transaction: Error out other than panic when committing transaction

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2800 bytes --]



> -----Original Message-----
> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo
> Sent: Thursday, July 05, 2018 3:37 PM
> To: linux-btrfs@vger.kernel.org
> Subject: [PATCH 2/4] btrfs-progs: transaction: Error out other than panic when committing transaction
> 
> There are cases that btrfs_commit_transaction() itself can fail, mostly
> due to ENOSPC when allocating space.
> 
> Don't panic out in this case.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  transaction.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/transaction.c b/transaction.c
> index 9619265ef6e8..b82d346b52c8 100644
> --- a/transaction.c
> +++ b/transaction.c
> @@ -73,7 +73,8 @@ static int update_cowonly_root(struct btrfs_trans_handle *trans,
>  		ret = btrfs_update_root(trans, tree_root,
>  					&root->root_key,
>  					&root->root_item);
> -		BUG_ON(ret);
> +		if (ret < 0)
> +			return ret;
>  		btrfs_write_dirty_block_groups(trans, root);
>  	}
>  	return 0;
> @@ -101,9 +102,11 @@ int commit_tree_roots(struct btrfs_trans_handle *trans,
>  		next = fs_info->dirty_cowonly_roots.next;
>  		list_del_init(next);
>  		root = list_entry(next, struct btrfs_root, dirty_list);
> -		update_cowonly_root(trans, root);
> +		ret = update_cowonly_root(trans, root);
>  		free_extent_buffer(root->commit_root);
>  		root->commit_root = NULL;
> +		if (ret < 0)
> +			return ret;
>  	}
> 
>  	return 0;
> @@ -162,12 +165,15 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>  	root->root_item.level = btrfs_header_level(root->node);
>  	ret = btrfs_update_root(trans, root->fs_info->tree_root,
>  				&root->root_key, &root->root_item);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto out;
>  commit_tree:
>  	ret = commit_tree_roots(trans, fs_info);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto out;
>  	ret = __commit_transaction(trans, root);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto out;
>  	write_ctree_super(trans);
>  	btrfs_finish_extent_commit(trans, fs_info->extent_root,
>  			           &fs_info->pinned_extents);
> @@ -176,7 +182,8 @@ commit_tree:
>  	root->commit_root = NULL;
>  	fs_info->running_transaction = NULL;
>  	fs_info->last_trans_committed = transid;
> -	return 0;
> +out:
> +	return ret;
>  }
> 
>  void btrfs_abort_transaction(struct btrfs_trans_handle *trans, int error)
> --

Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com>

> 2.18.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4154 bytes --]



On 2018年07月05日 16:50, Gu, Jinxiang wrote:
> 
> 
>> -----Original Message-----
>> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo
>> Sent: Thursday, July 05, 2018 3:37 PM
>> To: linux-btrfs@vger.kernel.org
>> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks
>>
>> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree.
> 
> Nit：
> or fs/file tree

Nope, btrfs check is only capable of reinit csum or extent tree.

Thanks,
Qu

> 
>> However this function allows us to let it overwrite existing tree blocks
>> using @overwrite parameter.
>>
>> Such behavior is pretty dangerous while no caller is using this feature
>> explicitly.
>>
>> So just remove @overwrite parameter and allow btrfs_fsck_reinit_root()
>> to error out when it fails to allocate tree block.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  check/main.c | 26 ++++++++------------------
>>  1 file changed, 8 insertions(+), 18 deletions(-)
>>
>> diff --git a/check/main.c b/check/main.c
>> index 8db300abb825..c8c347236543 100644
>> --- a/check/main.c
>> +++ b/check/main.c
>> @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info)
>>  }
>>
>>  static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
>> -			   struct btrfs_root *root, int overwrite)
>> +				  struct btrfs_root *root)
>>  {
>>  	struct extent_buffer *c;
>>  	struct extent_buffer *old = root->node;
>> @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
>>
>>  	level = 0;
>>
>> -	if (overwrite) {
>> -		c = old;
>> -		extent_buffer_get(c);
>> -		goto init;
>> -	}
>>  	c = btrfs_alloc_free_block(trans, root,
>>  				   root->fs_info->nodesize,
>>  				   root->root_key.objectid,
>>  				   &disk_key, level, 0, 0);
>> -	if (IS_ERR(c)) {
>> -		c = old;
>> -		extent_buffer_get(c);
>> -		overwrite = 1;
>> -	}
>> -init:
>> +	if (IS_ERR(c))
>> +		return PTR_ERR(c);
>> +
>>  	memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header));
>>  	btrfs_set_header_level(c, level);
>>  	btrfs_set_header_bytenr(c, c->start);
>> @@ -8422,9 +8414,7 @@ init:
>>  	/*
>>  	 * this case can happen in the following case:
>>  	 *
>> -	 * 1.overwrite previous root.
>> -	 *
>> -	 * 2.reinit reloc data root, this is because we skip pin
>> +	 * reinit reloc data root, this is because we skip pin
>>  	 * down reloc data tree before which means we can allocate
>>  	 * same block bytenr here.
>>  	 */
>> @@ -8609,7 +8599,7 @@ reinit_data_reloc:
>>  		goto out;
>>  	}
>>  	record_root_in_trans(trans, root);
>> -	ret = btrfs_fsck_reinit_root(trans, root, 0);
>> +	ret = btrfs_fsck_reinit_root(trans, root);
>>  	if (ret)
>>  		goto out;
>>  	ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID);
>> @@ -8675,7 +8665,7 @@ again:
>>  	}
>>
>>  	/* Ok we can allocate now, reinit the extent root */
>> -	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0);
>> +	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root);
>>  	if (ret) {
>>  		fprintf(stderr, "extent root initialization failed\n");
>>  		/*
>> @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv)
>>
>>  		if (init_csum_tree) {
>>  			printf("Reinitialize checksum tree\n");
>> -			ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0);
>> +			ret = btrfs_fsck_reinit_root(trans, info->csum_root);
>>  			if (ret) {
>>  				error("checksum tree initialization failed: %d",
>>  						ret);
>> --
> 
> Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> 
> 
>> 2.18.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> 
> N嫥叉靣笡y氊b瞂千v豝�)藓{.n�+壏{眓谶�)韰骅w*\x1fjg�\x1e秹殠娸/侁鋤罐枈�2娹櫒璀�&�)摺玜囤\x7f\x1e瓽珴閔�\x0f鎗:+v墾妛鑶佶
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4986 bytes --]



> -----Original Message-----
> From: Qu Wenruo [mailto:quwenruo.btrfs@gmx.com]
> Sent: Thursday, July 05, 2018 5:41 PM
> To: Gu, Jinxiang/顾 金香 <gujx@cn.fujitsu.com>; Qu Wenruo <wqu@suse.com>; linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks
> 
> 
> 
> On 2018年07月05日 16:50, Gu, Jinxiang wrote:
> >
> >
> >> -----Original Message-----
> >> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo
> >> Sent: Thursday, July 05, 2018 3:37 PM
> >> To: linux-btrfs@vger.kernel.org
> >> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks
> >>
> >> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree.
> >
> > Nit：
> > or fs/file tree
> 
> Nope, btrfs check is only capable of reinit csum or extent tree.
Sorry, not fs/file tree. But DATA RELOC TREE.
Call stack is reset_balance -> btrfs_fsck_reinit_root, and the root is get by
btrfs_read_fs_root(fs_info, &key), which key is
(BTRFS_DATA_RELOC_TREE_OBJECTID, BTRFS_ROOT_ITEM_KEY , (u64)-1)

> 
> Thanks,
> Qu
> 
> >
> >> However this function allows us to let it overwrite existing tree blocks
> >> using @overwrite parameter.
> >>
> >> Such behavior is pretty dangerous while no caller is using this feature
> >> explicitly.
> >>
> >> So just remove @overwrite parameter and allow btrfs_fsck_reinit_root()
> >> to error out when it fails to allocate tree block.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  check/main.c | 26 ++++++++------------------
> >>  1 file changed, 8 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/check/main.c b/check/main.c
> >> index 8db300abb825..c8c347236543 100644
> >> --- a/check/main.c
> >> +++ b/check/main.c
> >> @@ -8379,7 +8379,7 @@ static int do_check_chunks_and_extents(struct btrfs_fs_info *fs_info)
> >>  }
> >>
> >>  static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
> >> -			   struct btrfs_root *root, int overwrite)
> >> +				  struct btrfs_root *root)
> >>  {
> >>  	struct extent_buffer *c;
> >>  	struct extent_buffer *old = root->node;
> >> @@ -8389,21 +8389,13 @@ static int btrfs_fsck_reinit_root(struct btrfs_trans_handle *trans,
> >>
> >>  	level = 0;
> >>
> >> -	if (overwrite) {
> >> -		c = old;
> >> -		extent_buffer_get(c);
> >> -		goto init;
> >> -	}
> >>  	c = btrfs_alloc_free_block(trans, root,
> >>  				   root->fs_info->nodesize,
> >>  				   root->root_key.objectid,
> >>  				   &disk_key, level, 0, 0);
> >> -	if (IS_ERR(c)) {
> >> -		c = old;
> >> -		extent_buffer_get(c);
> >> -		overwrite = 1;
> >> -	}
> >> -init:
> >> +	if (IS_ERR(c))
> >> +		return PTR_ERR(c);
> >> +
> >>  	memset_extent_buffer(c, 0, 0, sizeof(struct btrfs_header));
> >>  	btrfs_set_header_level(c, level);
> >>  	btrfs_set_header_bytenr(c, c->start);
> >> @@ -8422,9 +8414,7 @@ init:
> >>  	/*
> >>  	 * this case can happen in the following case:
> >>  	 *
> >> -	 * 1.overwrite previous root.
> >> -	 *
> >> -	 * 2.reinit reloc data root, this is because we skip pin
> >> +	 * reinit reloc data root, this is because we skip pin
> >>  	 * down reloc data tree before which means we can allocate
> >>  	 * same block bytenr here.
> >>  	 */
> >> @@ -8609,7 +8599,7 @@ reinit_data_reloc:
> >>  		goto out;
> >>  	}
> >>  	record_root_in_trans(trans, root);
> >> -	ret = btrfs_fsck_reinit_root(trans, root, 0);
> >> +	ret = btrfs_fsck_reinit_root(trans, root);
> >>  	if (ret)
> >>  		goto out;
> >>  	ret = btrfs_make_root_dir(trans, root, BTRFS_FIRST_FREE_OBJECTID);
> >> @@ -8675,7 +8665,7 @@ again:
> >>  	}
> >>
> >>  	/* Ok we can allocate now, reinit the extent root */
> >> -	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root, 0);
> >> +	ret = btrfs_fsck_reinit_root(trans, fs_info->extent_root);
> >>  	if (ret) {
> >>  		fprintf(stderr, "extent root initialization failed\n");
> >>  		/*
> >> @@ -9764,7 +9754,7 @@ int cmd_check(int argc, char **argv)
> >>
> >>  		if (init_csum_tree) {
> >>  			printf("Reinitialize checksum tree\n");
> >> -			ret = btrfs_fsck_reinit_root(trans, info->csum_root, 0);
> >> +			ret = btrfs_fsck_reinit_root(trans, info->csum_root);
> >>  			if (ret) {
> >>  				error("checksum tree initialization failed: %d",
> >>  						ret);
> >> --
> >
> > Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com>
> >
> >
> >> 2.18.0
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
> >
> >
> > N嫥叉靣笡y氊b瞂千v豝�)藓{.n�+壏{眓谶�)韰骅w*jg�\x1e秹殠娸/侁鋤罐枈�2娹櫒璀�&�)摺玜囤\x7f\x1e瓽珴閔�\x0f鎗:+v墾
> 妛鑶佶
> >



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

RE: [PATCH 3/4] btrfs-progs: check/original: Avoid infinite loop when failed to repair inode

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2817 bytes --]



> -----Original Message-----
> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo
> Sent: Thursday, July 05, 2018 3:38 PM
> To: linux-btrfs@vger.kernel.org
> Subject: [PATCH 3/4] btrfs-progs: check/original: Avoid infinite loop when failed to repair inode
> 
> Exposed by fuzz-tests/003-multi-check-unmounted/ on fuzzed image
> bko-161811.raw.xz.
> 
> It's caused by the fact when check_fs_roots() finds tree root is
> modified, it re-search tree root by goto again: tag.
> However again: tag will also reset root objectid to 0.
> If we failed to repair one fs root but still modified tree root, we will
> go into such infinite loop.
> 
> Fix it by record which root we should skip for repair mode.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/main.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/check/main.c b/check/main.c
> index c8c347236543..2b5abb2d025b 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -3380,6 +3380,7 @@ static int check_fs_roots(struct btrfs_fs_info *fs_info,
>  	struct extent_buffer *leaf, *tree_node;
>  	struct btrfs_root *tmp_root;
>  	struct btrfs_root *tree_root = fs_info->tree_root;
> +	u64 skip_root = 0;
>  	int ret;
>  	int err = 0;
> 
> @@ -3400,7 +3401,10 @@ static int check_fs_roots(struct btrfs_fs_info *fs_info,
> 
>  again:
>  	key.offset = 0;
> -	key.objectid = 0;
> +	if (skip_root)
> +		key.objectid = skip_root + 1;
> +	else
> +		key.objectid = 0;
>  	key.type = BTRFS_ROOT_ITEM_KEY;
>  	ret = btrfs_search_slot(NULL, tree_root, &key, &path, 0, 0);
>  	if (ret < 0) {
> @@ -3409,6 +3413,7 @@ again:
>  	}
>  	tree_node = tree_root->node;
>  	while (1) {
> +
>  		if (tree_node != tree_root->node) {
>  			free_root_recs_tree(root_cache);
>  			btrfs_release_path(&path);
> @@ -3445,8 +3450,18 @@ again:
>  				btrfs_release_path(&path);
>  				goto again;
>  			}
> -			if (ret)
> +			if (ret) {
>  				err = 1;
> +
> +				/*
> +				 * We failed to repair this root but modified tree
> +				 * root, after again: tag we will still hit this
> +				 * root and fail to repair, must skip this root to
> +				 * avoid infinite loop
> +				 */
> +				if (repair)
> +					skip_root = key.objectid;
> +			}
>  			if (key.objectid == BTRFS_TREE_RELOC_OBJECTID)
>  				btrfs_free_fs_root(tmp_root);
>  		} else if (key.type == BTRFS_ROOT_REF_KEY ||
> --

Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com>


> 2.18.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

RE: [PATCH 4/4] btrfs-progs: check/original: Don't overwrite return value when we failed to repair

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1352 bytes --]



> -----Original Message-----
> From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs-owner@vger.kernel.org] On Behalf Of Qu Wenruo
> Sent: Thursday, July 05, 2018 3:38 PM
> To: linux-btrfs@vger.kernel.org
> Subject: [PATCH 4/4] btrfs-progs: check/original: Don't overwrite return value when we failed to repair
> 
> In check_inode_recs(), for repair mode we always reset @ret to 0.
> It makes no sense and later we check@ret to determine if the repair is
> successful.
> 
> Fix it by removing the offending overwrite.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 2b5abb2d025b..08476968aaba 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -2764,7 +2764,6 @@ static int check_inode_recs(struct btrfs_root *root,
>  				free_inode_rec(rec);
>  				continue;
>  			}
> -			ret = 0;
>  		}
> 
>  		if (!(repair && ret == 0))
> --

Reviewed-by: Gu Jinxiang <gujx@cn.fujitsu.com>


> 2.18.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

[David Sterba]

On Fri, Jul 06, 2018 at 01:53:24AM +0000, Gu, Jinxiang wrote:
> > >> Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks
> > >>
> > >> We have function btrfs_fsck_reinit_root() to reinit csum or extent tree.
> > >
> > > Nit：
> > > or fs/file tree
> > 
> > Nope, btrfs check is only capable of reinit csum or extent tree.
> Sorry, not fs/file tree. But DATA RELOC TREE.
> Call stack is reset_balance -> btrfs_fsck_reinit_root, and the root is get by
> btrfs_read_fs_root(fs_info, &key), which key is
> (BTRFS_DATA_RELOC_TREE_OBJECTID, BTRFS_ROOT_ITEM_KEY , (u64)-1)

So, it's correct that the data reloc tree is reinitialized, but this is
part of extent tree reset and there's no separate commandline option for
it. I understand the changelog as a reference to --init-csum-tree and
--init-extent-tree .

Re: [PATCH 0/4] Some random fuzz test fixes

[David Sterba]

On Thu, Jul 05, 2018 at 03:37:27PM +0800, Qu Wenruo wrote:
> Can be fetched from github:
> https://github.com/adam900710/btrfs-progs/tree/fuzzed_enhance
> 
> Some random bugs exposed by the ill-fated fuzz-test (003 never seems to
> pass).
> 
> It will be a big work to make fuzz/003 to pass, but at least there are
> some easy fixes.

Great, that counts too.

> Qu Wenruo (4):
>   btrfs-progs: check: Remove the ability to rebuild root overwritting
>     existing tree blocks
>   btrfs-progs: transaction: Error out other than panic when committing
>     transaction
>   btrfs-progs: check/original: Avoid infinite loop when failed to repair
>     inode
>   btrfs-progs: check/original: Don't overwrite return value when we
>     failed to repair

Thanks, all applied.