[PATCH 1/5] btrfs-progs: Add location check when process share_data_ref item

[PATCH 1/5] btrfs-progs: Add location check when process share_data_ref item

[Gu Jinxiang]

The following test failed becasuse share_data_ref be added into
extent_cache when deal with root tree node.

$sudo TEST=003\* make test-fuzz
cmds-check.c:5660: check_owner_ref: BUG_ON `rec->is_root` triggered, value 1
/home/adam/btrfs/btrfs-progs/btrfs[0x46a43b]
/home/adam/btrfs/btrfs-progs/btrfs[0x46a529]
/home/adam/btrfs/btrfs-progs/btrfs[0x479e55]
/home/adam/btrfs/btrfs-progs/btrfs[0x47af81]
/home/adam/btrfs/btrfs-progs/btrfs[0x47f69e]
/home/adam/btrfs/btrfs-progs/btrfs[0x484680]
/home/adam/btrfs/btrfs-progs/btrfs[0x484cb9]
/home/adam/btrfs/btrfs-progs/btrfs[0x4884f9]
/home/adam/btrfs/btrfs-progs/btrfs(cmd_check+0xb53)[0x48b7c3]
/home/adam/btrfs/btrfs-progs/btrfs(main+0x127)[0x40b39d]
/lib64/libc.so.6(__libc_start_main+0xea)[0x7f9ac76db03a]
/home/adam/btrfs/btrfs-progs/btrfs(_start+0x2a)[0x40ac9a]
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --check-data-csum /home/adam/btrfs/btrfs-progs/tests/fuzz-tests/images/bko-156731.raw.restored
mayfail: returned code 134 (SIGABRT), not ignored
test failed for case 003-multi-check-unmounted

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---
 cmds-check.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 5c822b84..71b15de4 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8018,6 +8018,14 @@ static int run_next_block(struct btrfs_root *root,
 			}
 			if (key.type == BTRFS_SHARED_DATA_REF_KEY) {
 				struct btrfs_shared_data_ref *ref;
+				if (root->root_key.objectid !=
+						BTRFS_EXTENT_TREE_OBJECTID) {
+					error(
+				"invaild location of share_data_ref [%d %d] root %d",
+						key.objectid, key.offset,
+						root->root_key.objectid);
+					continue;
+				}
 				ref = btrfs_item_ptr(buf, i,
 						struct btrfs_shared_data_ref);
 				add_data_backref(extent_cache,
-- 
2.14.3

[PATCH 2/5] btrfs-progs: Add logic to judge the level between parent and child

[Gu Jinxiang]

The following test failed becasuse the level of child node is not
correct. In repair mode, when update parent's ptr in
btrfs_search_slot->btrfs_cow_block->__btrfs_cow_block,
it use path[level+1] to get the parent, and not judge whether
path[level+1] is NULL.

$sudo TEST=003\* make test-fuzz
failed (ignored, ret=139): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests/fuzz-tests/images/bko-161821.raw.restored
mayfail: returned code 139 (SEGFAULT), not ignored

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---
 cmds-check.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 71b15de4..ac0375e5 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -2085,6 +2085,7 @@ static void reada_walk_down(struct btrfs_root *root,
  *    in parent.
  * 2. block in parent node should match the child node/leaf.
  * 3. generation of parent node and child's header should be consistent.
+ * 4. level of parent node should not smaller than the child node/leaf.
  *
  * Or the child node/leaf pointed by the key in parent is not valid.
  *
@@ -2125,6 +2126,17 @@ static int check_child_node(struct extent_buffer *parent, int slot,
 			btrfs_header_generation(child),
 			btrfs_node_ptr_generation(parent, slot));
 	}
+	/* If level of child is not correct, it will lead to
+	 * Segmentation fault when repair parent.
+	 * Because when update parent's ptr in
+	 * btrfs_search_slot->btrfs_cow_block->__btrfs_cow_block,
+	 * it use path[level+1] to get the parent
+	 */
+	if (btrfs_header_level(parent) <= btrfs_header_level(child)) {
+		ret = -EFAULT;
+		fprintf(stderr, "Wrong level of child node/leaf, wanted: %d, have: %d\n",
+				btrfs_header_level(parent)-1, btrfs_header_level(child));
+	}
 	return ret;
 }
 
@@ -4067,6 +4079,12 @@ static int check_fs_root(struct btrfs_root *root,
 		wret = walk_down_tree(root, &path, wc, &level, &nrefs);
 		if (wret < 0)
 			ret = wret;
+		/* When return value is -EFAULT, it indicate that level
+		 * in path is incorrect. And it will lead to Segmentation fault
+		 * in repair mod.
+		 */
+		if (wret == -EFAULT && repair)
+			goto out;
 		if (wret != 0)
 			break;
 
@@ -4123,6 +4141,7 @@ skip_walking:
 	if (!ret)
 		ret = err;
 
+out:
 	free_corrupt_blocks_tree(&corrupt_blocks);
 	root->fs_info->corrupt_blocks = NULL;
 	free_orphan_data_extents(&root->orphan_data_extents);
-- 
2.14.3

[PATCH 3/5] btrfs-progs: return error to upper caller instead of BUG_ON

[Gu Jinxiang]

Return error to upper caller instead of BUG_ON.

The following test failed when trying to repair fs root,
because the bytenr of item below in fs tree is smaller than sectorsize.
key (256 INODE_ITEM 0) block 3835 (0) gen 6052837899185946625
It fails in function read_tree_block.

Here comes the call stack:
    at disk-io.c:324
    at ctree.c:652
    p=0x721ec0, ins_len=185, cow=1) at ctree.c:1173
    path=0x721ec0, cpu_key=0x7fffffffdc90, data_size=0x7fffffffdbcc, nr=1) at ctree.c:2485
    key=0x7fffffffdc90, data_size=160) at ctree.h:2603
    cpu_key=0x7fffffffdc90, data=0x7fffffffdce0, data_size=160) at ctree.c:2584
    inode_item=0x7fffffffdce0) at inode-item.c:155
    at utils.c:397
    at cmds-check.c:3925

Here comes the error message:
$ sudo TEST=003\* make test-fuzz
cmds-check.c:3938: check_inode_recs: BUG_ON `ret` triggered, value -5
/home/adam/btrfs/btrfs-progs/btrfs[0x46aba7]
/home/adam/btrfs/btrfs-progs/btrfs[0x46ac95]
/home/adam/btrfs/btrfs-progs/btrfs[0x4760ee]
/home/adam/btrfs/btrfs-progs/btrfs[0x477d01]
/home/adam/btrfs/btrfs-progs/btrfs[0x477ff1]
/home/adam/btrfs/btrfs-progs/btrfs[0x47cd3f]
/home/adam/btrfs/btrfs-progs/btrfs(cmd_check+0xd6b)[0x48fc86]
/home/adam/btrfs/btrfs-progs/btrfs(main+0x127)[0x40b49d]
/lib64/libc.so.6(__libc_start_main+0xea)[0x7fc7005a603a]
/home/adam/btrfs/btrfs-progs/btrfs(_start+0x2a)[0x40ad9a]
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests/fuzz-tests/images/bko-172811.raw.restored

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---
 cmds-check.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index ac0375e5..49b0792b 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -3489,9 +3489,10 @@ static int check_inode_recs(struct btrfs_root *root,
 				(unsigned long long)root->objectid);
 
 			ret = btrfs_make_root_dir(trans, root, root_dirid);
-			BUG_ON(ret);
 
 			btrfs_commit_transaction(trans, root);
+			if (ret)
+				return ret;
 			return -EAGAIN;
 		}
 
-- 
2.14.3

[PATCH 4/5] btrfs-progs: check null pointer before use it

[Gu Jinxiang]

The following test failed when trying to check tree below.
item 9 key (TREE_RELOC ROOT_ITEM 0) itemoff 1135 itemsize 439
Since it has a inconsistent level in root and root->node, Segment fault
accures when use btrfs_node_key after btrfs_search_slot.

So add null pointer check before use btrfs_node_key.

Here comes the error message:
$ sudo TEST=003\* make test-fuzz
failed (ignored, ret=139): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests/fuzz-tests/images/bko-172811.raw.restored
mayfail: returned code 139 (SEGFAULT), not ignored
test failed for case 003-multi-check-unmounted

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---
 cmds-check.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 49b0792b..3f4244a2 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -4070,6 +4070,8 @@ static int check_fs_root(struct btrfs_root *root,
 		wret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
 		if (wret < 0)
 			goto skip_walking;
+		if (!path.nodes[level])
+			goto skip_walking;
 		btrfs_node_key(path.nodes[level], &found_key,
 				path.slots[level]);
 		WARN_ON(memcmp(&found_key, &root_item->drop_progress,
-- 
2.14.3

[PATCH 5/5] btrfs-progs: return error to caller instead of BUG_ON

[Gu Jinxiang]

The following test fails when deal with corrupt block below in fs tree.
key (3742682168 UNKNOWN.0 51539611375) block 256 (0) gen 298824472660

And when try to repair this fs tree, it fails in repair_btree.
Because in repair_btree, it attempt to balance, but can not
find the following item in extent tree.
key (34084861431808 UNKNOWN.0 256) block 131072 (32) gen 36283884678912

Since from returned value, we can know the result of a procedure,
so using return error instead of BUG_ON.

Here comes the error message:
$ sudo TEST=003\* make test-fuzz
ctree.c:197: update_ref_for_cow: BUG_ON `ret` triggered, value -5
/home/adam/btrfs/btrfs-progs/btrfs[0x40b5d4]
/home/adam/btrfs/btrfs-progs/btrfs[0x40b6c2]
/home/adam/btrfs/btrfs-progs/btrfs[0x40c727]
/home/adam/btrfs/btrfs-progs/btrfs(__btrfs_cow_block+0x2cf)[0x40cda4]
/home/adam/btrfs/btrfs-progs/btrfs(btrfs_cow_block+0x105)[0x40d0fc]
/home/adam/btrfs/btrfs-progs/btrfs[0x40dd8a]
/home/adam/btrfs/btrfs-progs/btrfs(btrfs_search_slot+0x355)[0x40f14f]
/home/adam/btrfs/btrfs-progs/btrfs[0x477546]
/home/adam/btrfs/btrfs-progs/btrfs[0x477c19]
/home/adam/btrfs/btrfs-progs/btrfs[0x477ff1]
/home/adam/btrfs/btrfs-progs/btrfs[0x47cd3f]
/home/adam/btrfs/btrfs-progs/btrfs(cmd_check+0xd6b)[0x48fc86]
/home/adam/btrfs/btrfs-progs/btrfs(main+0x127)[0x40b49d]
/lib64/libc.so.6(__libc_start_main+0xea)[0x7fe14bf1803a]
/home/adam/btrfs/btrfs-progs/btrfs(_start+0x2a)[0x40ad9a]
failed (ignored, ret=134): /home/adam/btrfs/btrfs-progs/btrfs check --init-csum-tree /home/adam/btrfs/btrfs-progs/tests/fuzz-tests/images/bko-172811.raw.restored
mayfail: returned code 134 (SIGABRT), not ignored

Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com>
---
 ctree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ctree.c b/ctree.c
index 4fc33b14..f4cf006b 100644
--- a/ctree.c
+++ b/ctree.c
@@ -194,8 +194,10 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 		ret = btrfs_lookup_extent_info(trans, root, buf->start,
 					       btrfs_header_level(buf), 1,
 					       &refs, &flags);
-		BUG_ON(ret);
-		BUG_ON(refs == 0);
+		if (refs == 0)
+			ret = -EIO;
+		if (ret)
+			return ret;
 	} else {
 		refs = 1;
 		if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID ||
-- 
2.14.3

Re: [PATCH 1/5] btrfs-progs: Add location check when process share_data_ref item

[David Sterba]

On Fri, Nov 24, 2017 at 06:41:28PM +0800, Gu Jinxiang wrote:
> The following test failed becasuse share_data_ref be added into
> extent_cache when deal with root tree node.

The whole function run_next_block would need to be revisited with
respect to error handling and sanity checks. Adding them only when a
fuzzed image hits the particular code path is not IMHO the best
approach.

If there's some fuzzed test case, we should try to find all similar
missing checks and fix them before moving to another type. Addressing
only the failed tests gives a false sense of fixing, there are usally
more similar bugs.

Re: [PATCH 1/5] btrfs-progs: Add location check when process share_data_ref item

[Qu Wenruo]

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



On 2017年11月29日 03:02, David Sterba wrote:
> On Fri, Nov 24, 2017 at 06:41:28PM +0800, Gu Jinxiang wrote:
>> The following test failed becasuse share_data_ref be added into
>> extent_cache when deal with root tree node.
> 
> The whole function run_next_block would need to be revisited with
> respect to error handling and sanity checks. Adding them only when a
> fuzzed image hits the particular code path is not IMHO the best
> approach.

As suggested before, backporting tree-checker from kernel may be a good
solution for all later possible fuzzed problem.
(And can make btrfs-progs become a good testbed for tree-checker related
patches before merging into kernel)

For this particular case, planned key->type check against root should
handle it quite well.

Thanks,
Qu

> 
> If there's some fuzzed test case, we should try to find all similar
> missing checks and fix them before moving to another type. Addressing
> only the failed tests gives a false sense of fixing, there are usally
> more similar bugs.
> --
> 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
> 


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

Re: [PATCH 1/5] btrfs-progs: Add location check when process share_data_ref item

[David Sterba]

On Thu, Nov 30, 2017 at 02:55:49PM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年11月29日 03:02, David Sterba wrote:
> > On Fri, Nov 24, 2017 at 06:41:28PM +0800, Gu Jinxiang wrote:
> >> The following test failed becasuse share_data_ref be added into
> >> extent_cache when deal with root tree node.
> > 
> > The whole function run_next_block would need to be revisited with
> > respect to error handling and sanity checks. Adding them only when a
> > fuzzed image hits the particular code path is not IMHO the best
> > approach.
> 
> As suggested before, backporting tree-checker from kernel may be a good
> solution for all later possible fuzzed problem.
> (And can make btrfs-progs become a good testbed for tree-checker related
> patches before merging into kernel)

Agreed, that would be great.

> For this particular case, planned key->type check against root should
> handle it quite well.

Good, so I'd rather take the tree-checker route.