Some random fuzz test fixes

PATCH In review Qu Wenruo 4 participants 13 messages Jul 5, 2018 – Aug 2, 2018 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2018-07-05 07:37 UTC #0 lore raw

cover Some random fuzz test fixes

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(-)

Signature · 1 line

2.18.0

Qu Wenruo

author

2018-07-05 07:37 UTC #1 lore raw

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. 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 <[email protected]>

---
 check/main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

check/main.c +8 ~~-18~~

index 8db300abb825..c8c347236543 100644
@@ -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);

Signature · 1 line

2.18.0

Qu Wenruo

author

2018-07-05 07:37 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 transaction.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

transaction.c +13 -6

index 9619265ef6e8..b82d346b52c8 100644
@@ -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)

Signature · 1 line

2.18.0

Qu Wenruo

author

2018-07-05 07:37 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 check/main.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

check/main.c +17 -2

index c8c347236543..2b5abb2d025b 100644
@@ -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 ||

Signature · 1 line

2.18.0

Qu Wenruo

author

2018-07-05 07:37 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 check/main.c | 1 -
 1 file changed, 1 deletion(-)

check/main.c +0 -1

index 2b5abb2d025b..08476968aaba 100644
@@ -2764,7 +2764,6 @@ static int check_inode_recs(struct btrfs_root *root,
 				free_inode_rec(rec);
 				continue;
 			}
-			ret = 0;
 		}
 
 		if (!(repair && ret == 0))

Signature · 1 line

2.18.0

Gu, Jinxiang

Reviewed-by on v1 1/4

2018-07-05 08:50 UTC #2 lore raw

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

In reply to Qu Wenruo

Quoted 9 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Qu Wenruo
Sent: Thursday, July 05, 2018 3:37 PM
To: [email protected]
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

Quoted 82 lines

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 <[email protected]>

---
 check/main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

check/main.c +7 ~~-17~~

index 8db300abb825..c8c347236543 100644
@@ -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");
 		/*

check/main.c int cmd_check(int argc, char **argv)

Quoted 9 lines

check/main.c +1 -2

@@ -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 <[email protected]>

Quoted 7 lines

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

�翳簕.n�+壏煯壄�+%娝�遍荻w�簕.n�+壏{饼籯~镶炟^n噐■�侂h櫒璀�&｛鄗夸z罐楘+€�+zf＂穐殘啳嗃i��飦�戧鐉_璁鎗:+v墾�)撸鴐

Gu, Jinxiang

Reviewed-by on v1 2/4

2018-07-05 09:33 UTC #7 lore raw

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

In reply to Qu Wenruo

Quoted 65 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Qu Wenruo
Sent: Thursday, July 05, 2018 3:37 PM
To: [email protected]
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 <[email protected]>

---
 transaction.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

transaction.c +11 -5

index 9619265ef6e8..b82d346b52c8 100644
@@ -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);

transaction.c commit_tree:

Quoted 10 lines

transaction.c +2 -2

@@ -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 <[email protected]>

Quoted 7 lines

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

�翳簕.n�+壏煯壄�+%娝�遍荻w�簕.n�+壏{饼籯~镶炟^n噐■�侂h櫒璀�&｛鄗夸z罐楘+€�+zf＂穐殘啳嗃i��飦�戧鐉_璁鎗:+v墾�)撸鴐

Qu Wenruo

2018-07-05 09:41 UTC #3 lore raw

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

In reply to Gu, Jinxiang

Quoted 8 lines · Jinxiang

Quoted 8 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Qu Wenruo
Sent: Thursday, July 05, 2018 3:37 PM
To: [email protected]
Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

Quoted 4 lines

Quoted 1 line
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

Quoted 108 lines

Quoted 83 lines

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 <[email protected]>

---
 check/main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

check/main.c +7 ~~-17~~

index 8db300abb825..c8c347236543 100644
@@ -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");
 		/*

check/main.c int cmd_check(int argc, char **argv)

Quoted 9 lines

check/main.c +1 -2

@@ -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 <[email protected]>

Quoted 7 lines

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

N嫥叉靣笡y氊b瞂千v豝�)藓{.n�+壏{眓谶�)韰骅w*jg�秹殠娸/侁鋤罐枈�2娹櫒璀�&�)摺玜囤瓽珴閔�鎗:+v墾妛鑶佶

Gu, Jinxiang

2018-07-06 01:53 UTC #4 lore raw

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

Quoted 19 lines

-----Original Message-----
From: Qu Wenruo [mailto:[email protected]]
Sent: Thursday, July 05, 2018 5:41 PM
To: Gu, Jinxiang/顾金香 <[email protected]>; Qu Wenruo <[email protected]>; [email protected]
Subject: Re: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks
Quoted 8 lines · Jinxiang
Quoted 8 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Qu Wenruo
Sent: Thursday, July 05, 2018 3:37 PM
To: [email protected]
Subject: [PATCH 1/4] btrfs-progs: check: Remove the ability to rebuild root overwritting existing tree blocks

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
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)

Quoted 110 lines

Thanks,
Qu

Quoted 106 lines

Quoted 83 lines

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 <[email protected]>

---
 check/main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

check/main.c +7 ~~-17~~

index 8db300abb825..c8c347236543 100644
@@ -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");
 		/*

check/main.c int cmd_check(int argc, char **argv)

Quoted 9 lines

check/main.c +1 -2

@@ -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 <[email protected]>

Quoted 7 lines

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

Quoted 3 lines

Quoted 1 line
N嫥叉靣笡y氊b瞂千v豝�)藓{.n�+壏{眓谶�)韰骅w*jg�秹殠娸/侁鋤罐枈�2娹櫒璀�&�)摺玜囤瓽珴閔�鎗:+v墾
妛鑶佶
Quoted 1 line

��{.n�+��+%��ݶ��w��{.n�+��{��k~��^n�r��z��h��&��z��z�ޗ�+��+zf��h��~��i��z_��j:+v��)ߣ�m

Gu, Jinxiang

Reviewed-by on v1 3/4

2018-07-06 03:22 UTC #9 lore raw

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

In reply to Qu Wenruo

Quoted 67 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Qu Wenruo
Sent: Thursday, July 05, 2018 3:38 PM
To: [email protected]
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 <[email protected]>
---
 check/main.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)
check/main.c +11 -2
index c8c347236543..2b5abb2d025b 100644
@@ -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

check/main.c again:

Quoted 10 lines

check/main.c +6 -1

+				 * 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 <[email protected]>

Quoted 7 lines

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

�翳簕.n�+壏煯壄�+%娝�遍荻w�簕.n�+壏{饼籯~镶炟^n噐■�侂h櫒璀�&｛鄗夸z罐楘+€�+zf＂穐殘啳嗃i��飦�戧鐉_璁鎗:+v墾�)撸鴐

Gu, Jinxiang

Reviewed-by on v1 4/4

2018-07-06 04:57 UTC #11 lore raw

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

In reply to Qu Wenruo

Quoted 24 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Qu Wenruo
Sent: Thursday, July 05, 2018 3:38 PM
To: [email protected]
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 <[email protected]>
---
 check/main.c | 1 -
 1 file changed, 1 deletion(-)
check/main.c +0 -0
index 2b5abb2d025b..08476968aaba 100644

check/main.c static int check_inode_recs(struct btrfs_root *root,

Quoted 8 lines

check/main.c +0 -2

@@ -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 <[email protected]>

Quoted 7 lines

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

�翳簕.n�+壏煯壄�+%娝�遍荻w�簕.n�+壏{饼籯~镶炟^n噐■�侂h櫒璀�&｛鄗夸z罐楘+€�+zf＂穐殘啳嗃i��飦�戧鐉_璁鎗:+v墾�)撸鴐

David Sterba

2018-08-02 21:28 UTC #5 lore raw

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

In reply to Gu, Jinxiang

Quoted 4 lines · Jinxiang

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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 .

David Sterba

2018-08-02 21:55 UTC #12 lore raw

In reply to Qu Wenruo

Quoted 8 lines · 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.

Great, that counts too.

Quoted 9 lines

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.