[PATCH 0/3] btrfs: tree-checker: Enhancement and fixes for new fuzzed image report

[PATCH 0/3] btrfs: tree-checker: Enhancement and fixes for new fuzzed image report

[Qu Wenruo]

This patchset can be fetched from:
https://github.com/adam900710/linux/tree/tree_checker_enhancement

The base branch write_time_tree_checker, the base commit is:
commit cb3ba69132a83d5e87f243dece5db09a022b0be7 (github/write_time_tree_checker, write_time_tree_checker)
Author: Qu Wenruo <wqu@suse.com>
Date:   Thu Jan 17 15:15:14 2019 +0800

    btrfs: Do mandatory tree block check before submitting bio


Yoon Jungyeon reported several fuzzed image screwing up btrfs.

3 of the reported images are mount panic and can be addressed by tree
checker enhancement and fix.

This patchset addresses those 3 mount panic by:
- Verify chunk items
  This merges the existing btrfs_check_chunk_valid() and apply it to all
  chunk items, both in super block syschunk array and all tree blocks.

- Verify dev item
  This could avoid NULL pointer workaround for seed dummy.

- Don't trigger comprehensive root owner check if tree root is not
  initialized yet

The remaining runtime problem needs extra inspection, and some isn't so
easy to fix by tree-checker alone.

Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>

Qu Wenruo (3):
  btrfs: tree-checker: Verify chunk items
  btrfs: tree-checker: Verify dev item
  btrfs: tree-checker: Fix NULL pointer access for corrupted chunk root

 fs/btrfs/tree-checker.c | 242 +++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/tree-checker.h |   3 +
 fs/btrfs/volumes.c      | 103 +----------------
 fs/btrfs/volumes.h      |   9 ++
 4 files changed, 254 insertions(+), 103 deletions(-)

-- 
2.21.0

[PATCH 1/3] btrfs: tree-checker: Verify chunk items

[Qu Wenruo]

We already have btrfs_check_chunk_valid() to verify each chunk before
tree-checker.

Merge that function into tree-checker, and update its error message to
be more readable.

Old error message would be something like:
  BTRFS error (device dm-3): invalid chunk num_stipres: 0

New error message would be:
  Btrfs critical (device dm-3): corrupt superblock syschunk array: chunk_start=2097152, invalid chunk num_stripes: 0
Or
  Btrfs critical (device dm-3): corrupt leaf: root=3 block=8388608 slot=3 chunk_start=2097152, invalid chunk num_stripes: 0

Btrfs_check_chunk_valid() is exported for super block syschunk array
verification.

Also make tree-checker to verify chunk items, this makes chunk item
checker covers all chunks and avoid fuzzed image.

Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 152 ++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/tree-checker.h |   3 +
 fs/btrfs/volumes.c      |  94 +------------------------
 3 files changed, 156 insertions(+), 93 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index b8cdaf472031..fe3f37c23c29 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -448,6 +448,152 @@ static int check_block_group_item(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+__printf(5, 6)
+__cold
+static void chunk_err(const struct btrfs_fs_info *fs_info,
+		      const struct extent_buffer *leaf,
+		      const struct btrfs_chunk *chunk, u64 logical,
+		      const char *fmt, ...)
+{
+	/* Only superblock eb is able to have such small offset */
+	bool is_sb = (leaf->start == BTRFS_SUPER_INFO_OFFSET);
+	struct va_format vaf;
+	va_list args;
+	int i;
+	int slot = -1;
+
+	if (!is_sb) {
+		/*
+		 * Get the slot number by iterating through all slots, this
+		 * would provide better readability.
+		 */
+		for (i = 0; i < btrfs_header_nritems(leaf); i++) {
+			if (btrfs_item_ptr_offset(leaf, i) ==
+					(unsigned long) chunk) {
+				slot = i;
+				break;
+			}
+		}
+	}
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	if (is_sb)
+		btrfs_crit(fs_info,
+		"corrupt superblock syschunk array: chunk_start=%llu, %pV",
+			   logical, &vaf);
+	else
+		btrfs_crit(fs_info,
+	"corrupt leaf: root=%llu block=%llu slot=%d chunk_start=%llu, %pV",
+			   BTRFS_CHUNK_TREE_OBJECTID, leaf->start, slot,
+			   logical, &vaf);
+	va_end(args);
+}
+
+/*
+ * The common chunk check which could also work on super block sys chunk array.
+ *
+ * Return -EUCLEAN if anything is corrupted.
+ * Return 0 if everything is OK.
+ */
+int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
+			    struct extent_buffer *leaf,
+			    struct btrfs_chunk *chunk, u64 logical)
+{
+	u64 length;
+	u64 stripe_len;
+	u16 num_stripes;
+	u16 sub_stripes;
+	u64 type;
+	u64 features;
+	bool mixed = false;
+
+	length = btrfs_chunk_length(leaf, chunk);
+	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
+	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
+	sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
+	type = btrfs_chunk_type(leaf, chunk);
+
+	if (!num_stripes) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk num_stripes: %u", num_stripes);
+		return -EUCLEAN;
+	}
+	if (!IS_ALIGNED(logical, fs_info->sectorsize)) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk logical %llu", logical);
+		return -EUCLEAN;
+	}
+	if (btrfs_chunk_sector_size(leaf, chunk) != fs_info->sectorsize) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk sectorsize %u",
+			  btrfs_chunk_sector_size(leaf, chunk));
+		return -EUCLEAN;
+	}
+	if (!length || !IS_ALIGNED(length, fs_info->sectorsize)) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk length %llu", length);
+		return -EUCLEAN;
+	}
+	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk stripe length: %llu", stripe_len);
+		return -EUCLEAN;
+	}
+	if (~(BTRFS_BLOCK_GROUP_TYPE_MASK | BTRFS_BLOCK_GROUP_PROFILE_MASK) &
+	    type) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "unrecognized chunk type: %llu",
+			  ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
+			    BTRFS_BLOCK_GROUP_PROFILE_MASK) &
+			  btrfs_chunk_type(leaf, chunk));
+		return -EUCLEAN;
+	}
+
+	if ((type & BTRFS_BLOCK_GROUP_TYPE_MASK) == 0) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "missing chunk type flag: 0x%llx", type);
+		return -EUCLEAN;
+	}
+
+	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
+	    (type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			"system chunk with data or metadata type: 0x%llx", type);
+		return -EUCLEAN;
+	}
+
+	features = btrfs_super_incompat_flags(fs_info->super_copy);
+	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
+		mixed = true;
+
+	if (!mixed) {
+		if ((type & BTRFS_BLOCK_GROUP_METADATA) &&
+		    (type & BTRFS_BLOCK_GROUP_DATA)) {
+			chunk_err(fs_info, leaf, chunk, logical,
+			"mixed chunk type in non-mixed mode: 0x%llx", type);
+			return -EUCLEAN;
+		}
+	}
+
+	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
+	    (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
+	    (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) ||
+	    ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
+	     num_stripes != 1)) {
+		chunk_err(fs_info, leaf, chunk, logical,
+			"invalid num_stripes:sub_stripes %u:%u for profile %llu",
+			num_stripes, sub_stripes,
+			type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
+		return -EUCLEAN;
+	}
+
+	return 0;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */
@@ -455,6 +601,7 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,
 			   struct extent_buffer *leaf,
 			   struct btrfs_key *key, int slot)
 {
+	struct btrfs_chunk *chunk;
 	int ret = 0;
 
 	switch (key->type) {
@@ -472,6 +619,11 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,
 	case BTRFS_BLOCK_GROUP_ITEM_KEY:
 		ret = check_block_group_item(fs_info, leaf, key, slot);
 		break;
+	case BTRFS_CHUNK_ITEM_KEY:
+		chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk);
+		ret = btrfs_check_chunk_valid(fs_info, leaf, chunk,
+					      key->offset);
+		break;
 	}
 	return ret;
 }
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 6f8d1b627c53..6cb96ba5e711 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -33,4 +33,7 @@ int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info,
 			   struct extent_buffer *leaf);
 int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node);
 
+int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
+			    struct extent_buffer *leaf,
+			    struct btrfs_chunk *chunk, u64 logical);
 #endif
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 15561926ab32..0e3822870f1e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -27,6 +27,7 @@
 #include "math.h"
 #include "dev-replace.h"
 #include "sysfs.h"
+#include "tree-checker.h"
 
 const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 	[BTRFS_RAID_RAID10] = {
@@ -6705,99 +6706,6 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 	return dev;
 }
 
-/* Return -EIO if any error, otherwise return 0. */
-static int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
-				   struct extent_buffer *leaf,
-				   struct btrfs_chunk *chunk, u64 logical)
-{
-	u64 length;
-	u64 stripe_len;
-	u16 num_stripes;
-	u16 sub_stripes;
-	u64 type;
-	u64 features;
-	bool mixed = false;
-
-	length = btrfs_chunk_length(leaf, chunk);
-	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
-	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
-	sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
-	type = btrfs_chunk_type(leaf, chunk);
-
-	if (!num_stripes) {
-		btrfs_err(fs_info, "invalid chunk num_stripes: %u",
-			  num_stripes);
-		return -EIO;
-	}
-	if (!IS_ALIGNED(logical, fs_info->sectorsize)) {
-		btrfs_err(fs_info, "invalid chunk logical %llu", logical);
-		return -EIO;
-	}
-	if (btrfs_chunk_sector_size(leaf, chunk) != fs_info->sectorsize) {
-		btrfs_err(fs_info, "invalid chunk sectorsize %u",
-			  btrfs_chunk_sector_size(leaf, chunk));
-		return -EIO;
-	}
-	if (!length || !IS_ALIGNED(length, fs_info->sectorsize)) {
-		btrfs_err(fs_info, "invalid chunk length %llu", length);
-		return -EIO;
-	}
-	if (!is_power_of_2(stripe_len) || stripe_len != BTRFS_STRIPE_LEN) {
-		btrfs_err(fs_info, "invalid chunk stripe length: %llu",
-			  stripe_len);
-		return -EIO;
-	}
-	if (~(BTRFS_BLOCK_GROUP_TYPE_MASK | BTRFS_BLOCK_GROUP_PROFILE_MASK) &
-	    type) {
-		btrfs_err(fs_info, "unrecognized chunk type: %llu",
-			  ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
-			    BTRFS_BLOCK_GROUP_PROFILE_MASK) &
-			  btrfs_chunk_type(leaf, chunk));
-		return -EIO;
-	}
-
-	if ((type & BTRFS_BLOCK_GROUP_TYPE_MASK) == 0) {
-		btrfs_err(fs_info, "missing chunk type flag: 0x%llx", type);
-		return -EIO;
-	}
-
-	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
-	    (type & (BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA))) {
-		btrfs_err(fs_info,
-			"system chunk with data or metadata type: 0x%llx", type);
-		return -EIO;
-	}
-
-	features = btrfs_super_incompat_flags(fs_info->super_copy);
-	if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS)
-		mixed = true;
-
-	if (!mixed) {
-		if ((type & BTRFS_BLOCK_GROUP_METADATA) &&
-		    (type & BTRFS_BLOCK_GROUP_DATA)) {
-			btrfs_err(fs_info,
-			"mixed chunk type in non-mixed mode: 0x%llx", type);
-			return -EIO;
-		}
-	}
-
-	if ((type & BTRFS_BLOCK_GROUP_RAID10 && sub_stripes != 2) ||
-	    (type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 1) ||
-	    (type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 2) ||
-	    (type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 3) ||
-	    (type & BTRFS_BLOCK_GROUP_DUP && num_stripes > 2) ||
-	    ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
-	     num_stripes != 1)) {
-		btrfs_err(fs_info,
-			"invalid num_stripes:sub_stripes %u:%u for profile %llu",
-			num_stripes, sub_stripes,
-			type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
-		return -EIO;
-	}
-
-	return 0;
-}
-
 static void btrfs_report_missing_device(struct btrfs_fs_info *fs_info,
 					u64 devid, u8 *uuid, bool error)
 {
-- 
2.21.0

Re: [PATCH 1/3] btrfs: tree-checker: Verify chunk items

[Nikolay Borisov]

On 8.03.19 г. 9:29 ч., Qu Wenruo wrote:
> We already have btrfs_check_chunk_valid() to verify each chunk before
> tree-checker.
> 
> Merge that function into tree-checker, and update its error message to
> be more readable.
> 
> Old error message would be something like:
>   BTRFS error (device dm-3): invalid chunk num_stipres: 0
> 
> New error message would be:
>   Btrfs critical (device dm-3): corrupt superblock syschunk array: chunk_start=2097152, invalid chunk num_stripes: 0
> Or
>   Btrfs critical (device dm-3): corrupt leaf: root=3 block=8388608 slot=3 chunk_start=2097152, invalid chunk num_stripes: 0
> 
> Btrfs_check_chunk_valid() is exported for super block syschunk array
> verification.
> 
> Also make tree-checker to verify chunk items, this makes chunk item
> checker covers all chunks and avoid fuzzed image.
> 
> Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 1/3] btrfs: tree-checker: Verify chunk items

[Nikolay Borisov]

On 8.03.19 г. 9:29 ч., Qu Wenruo wrote:
> We already have btrfs_check_chunk_valid() to verify each chunk before
> tree-checker.
> 
> Merge that function into tree-checker, and update its error message to
> be more readable.
> 
> Old error message would be something like:
>   BTRFS error (device dm-3): invalid chunk num_stipres: 0
> 
> New error message would be:
>   Btrfs critical (device dm-3): corrupt superblock syschunk array: chunk_start=2097152, invalid chunk num_stripes: 0
> Or
>   Btrfs critical (device dm-3): corrupt leaf: root=3 block=8388608 slot=3 chunk_start=2097152, invalid chunk num_stripes: 0
> 
> Btrfs_check_chunk_valid() is exported for super block syschunk array
> verification.
> 
> Also make tree-checker to verify chunk items, this makes chunk item
> checker covers all chunks and avoid fuzzed image.
> 
> Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Actually since the reporter made images available I would like to have
those integrated into the fsck group so that those changes can be
validated and ensure further regressions do not creep up.

Re: [PATCH 1/3] btrfs: tree-checker: Verify chunk items

[Qu Wenruo]

On 2019/3/11 下午11:25, Nikolay Borisov wrote:
> 
> 
> On 8.03.19 г. 9:29 ч., Qu Wenruo wrote:
>> We already have btrfs_check_chunk_valid() to verify each chunk before
>> tree-checker.
>>
>> Merge that function into tree-checker, and update its error message to
>> be more readable.
>>
>> Old error message would be something like:
>>   BTRFS error (device dm-3): invalid chunk num_stipres: 0
>>
>> New error message would be:
>>   Btrfs critical (device dm-3): corrupt superblock syschunk array: chunk_start=2097152, invalid chunk num_stripes: 0
>> Or
>>   Btrfs critical (device dm-3): corrupt leaf: root=3 block=8388608 slot=3 chunk_start=2097152, invalid chunk num_stripes: 0
>>
>> Btrfs_check_chunk_valid() is exported for super block syschunk array
>> verification.
>>
>> Also make tree-checker to verify chunk items, this makes chunk item
>> checker covers all chunks and avoid fuzzed image.
>>
>> Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Actually since the reporter made images available I would like to have
> those integrated into the fsck group so that those changes can be
> validated and ensure further regressions do not creep up.

Will be done after all the reported images can be handled by kernel.

And, I'll also try to create the minimal image for the fuzzed group, as
some image has unrelated problem populating the dmesg.

BTW, all those images can be handled by fsck, thus doesn't make much
sense for fsck group.

Thanks,
Qu

> 

Re: [PATCH 1/3] btrfs: tree-checker: Verify chunk items

[Qu Wenruo]

On 2019/3/11 下午11:25, Nikolay Borisov wrote:
>
>
> On 8.03.19 г. 9:29 ч., Qu Wenruo wrote:
>> We already have btrfs_check_chunk_valid() to verify each chunk before
>> tree-checker.
>>
>> Merge that function into tree-checker, and update its error message to
>> be more readable.
>>
>> Old error message would be something like:
>>   BTRFS error (device dm-3): invalid chunk num_stipres: 0
>>
>> New error message would be:
>>   Btrfs critical (device dm-3): corrupt superblock syschunk array: chunk_start=2097152, invalid chunk num_stripes: 0
>> Or
>>   Btrfs critical (device dm-3): corrupt leaf: root=3 block=8388608 slot=3 chunk_start=2097152, invalid chunk num_stripes: 0
>>
>> Btrfs_check_chunk_valid() is exported for super block syschunk array
>> verification.
>>
>> Also make tree-checker to verify chunk items, this makes chunk item
>> checker covers all chunks and avoid fuzzed image.
>>
>> Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202751
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Actually since the reporter made images available I would like to have
> those integrated into the fsck group so that those changes can be
> validated and ensure further regressions do not creep up.
>

Add David into this discussion.
This topic seems to be mentioned before.

Should we put kernel test cases into btrfs-progs?

We have fuzzed test groups for btrfs-progs mainly, but we don't really
have kernel tests in btrfs-progs.
Is it proper to put kernel-crashing tests into btrfs-progs?

If not, where should we put such tests?


And BTW, I originally wanted to craft the minimal image for those tests,
but quite a lot of the image needs several corruption combined to hit
the pitfall. I'm not sure if it's worthy to create the minimal image.

Thanks,
Qu

Re: [PATCH 1/3] btrfs: tree-checker: Verify chunk items

[David Sterba]

On Tue, Mar 19, 2019 at 03:58:24PM +0800, Qu Wenruo wrote:
> Should we put kernel test cases into btrfs-progs?

We collect all the images in btrfs-progs.

> We have fuzzed test groups for btrfs-progs mainly, but we don't really
> have kernel tests in btrfs-progs.
> Is it proper to put kernel-crashing tests into btrfs-progs?

Yes, but they should not be run by default. Currently there's not even a
test script for mounting the images. But the testsuite can be run
independently in a VM so the kernel tests make sense too.

> If not, where should we put such tests?
> 
> And BTW, I originally wanted to craft the minimal image for those tests,
> but quite a lot of the image needs several corruption combined to hit
> the pitfall. I'm not sure if it's worthy to create the minimal image.

That probably depends how much other work does the minimal image save.
The plan is to use python-btrfs to craft the images on-the-fly. We could
do that by specific tools written in C or enhance btrfs-corroupt-block,
but I'm strongly in favor of the python way due to simplicity.

Another way is to use the script only to generate the image and then
store it for tests to avoid the run-time dependency on python-btrfs.

Re: [PATCH 1/3] btrfs: tree-checker: Verify chunk items

[Qu Wenruo]

On 2019/3/29 上午12:58, David Sterba wrote:
> On Tue, Mar 19, 2019 at 03:58:24PM +0800, Qu Wenruo wrote:
>> Should we put kernel test cases into btrfs-progs?
>
> We collect all the images in btrfs-progs.
>
>> We have fuzzed test groups for btrfs-progs mainly, but we don't really
>> have kernel tests in btrfs-progs.
>> Is it proper to put kernel-crashing tests into btrfs-progs?
>
> Yes, but they should not be run by default. Currently there's not even a
> test script for mounting the images. But the testsuite can be run
> independently in a VM so the kernel tests make sense too.
>
>> If not, where should we put such tests?
>>
>> And BTW, I originally wanted to craft the minimal image for those tests,
>> but quite a lot of the image needs several corruption combined to hit
>> the pitfall. I'm not sure if it's worthy to create the minimal image.
>
> That probably depends how much other work does the minimal image save.

Just tens of kilo bytes for the image.
The original crafted image is around 100 kilo after compression.

The minimal image is about 20 kilo, but I can't reproduce the same bug
due to less corruption.

> The plan is to use python-btrfs to craft the images on-the-fly. We could
> do that by specific tools written in C or enhance btrfs-corroupt-block,
> but I'm strongly in favor of the python way due to simplicity.

Considering how many leaves and their copies needs to be corrupted, and
some tricky combination is needed.

I'm more or less tending to just use the fuzzed images provided by the
reporter.

Thanks,
Qu

>
> Another way is to use the script only to generate the image and then
> store it for tests to avoid the run-time dependency on python-btrfs.
>

[PATCH 2/3] btrfs: tree-checker: Verify dev item

[Qu Wenruo]

[BUG]
For fuzzed image whose DEV_ITEM has invalid total_bytes as 0, then
kernel will just panic:
  BUG: unable to handle kernel NULL pointer dereference at 0000000000000098
  #PF error: [normal kernel read fault]
  PGD 800000022b2bd067 P4D 800000022b2bd067 PUD 22b2bc067 PMD 0
  Oops: 0000 [#1] SMP PTI
  CPU: 0 PID: 1106 Comm: mount Not tainted 5.0.0-rc8+ #9
  RIP: 0010:btrfs_verify_dev_extents+0x2a5/0x5a0
  Call Trace:
   open_ctree+0x160d/0x2149
   btrfs_mount_root+0x5b2/0x680

[CAUSE]
If device extent verification finds a deivce with 0 total_bytes, then it
assumes it's a seed dummy, then search for seed devices.

But in this case, there is no seed device at all, causing NULL pointer.

[FIX]
Since this is caused by fuzzed image, let's go the tree-check way, just
add a new verification for device item.

Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202691
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 84 +++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c      |  9 -----
 fs/btrfs/volumes.h      |  9 +++++
 3 files changed, 93 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index fe3f37c23c29..5ccb4be583ea 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -594,6 +594,87 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+
+__printf(4, 5)
+__cold
+static void dev_item_err(const struct btrfs_fs_info *fs_info,
+			 const struct extent_buffer *eb, int slot,
+			 const char *fmt, ...)
+{
+	struct btrfs_key key;
+	struct va_format vaf;
+	va_list args;
+
+	btrfs_item_key_to_cpu(eb, &key, slot);
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	btrfs_crit(fs_info,
+	"corrupt %s: root=%llu block=%llu slot=%d devid=%llu %pV",
+		btrfs_header_level(eb) == 0 ? "leaf" : "node",
+		btrfs_header_owner(eb), btrfs_header_bytenr(eb), slot,
+		key.objectid, &vaf);
+	va_end(args);
+}
+
+static int check_dev_item(struct btrfs_fs_info *fs_info,
+			  struct extent_buffer *leaf,
+			  struct btrfs_key *key, int slot)
+{
+	struct btrfs_dev_item *ditem;
+	u64 max_devid = max(BTRFS_MAX_DEVS(fs_info), BTRFS_MAX_DEVS_SYS_CHUNK);
+
+	if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) {
+		dev_item_err(fs_info, leaf, slot,
+			     "invalid objectid: has=%llu expect=%llu",
+			     key->objectid, BTRFS_DEV_ITEMS_OBJECTID);
+		goto error;
+	}
+	if (key->offset > max_devid) {
+		dev_item_err(fs_info, leaf, slot,
+			     "invalid devid: has=%llu expect=[0, %llu]",
+			     key->offset, max_devid);
+		goto error;
+	}
+	ditem = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item);
+	if (btrfs_device_id(leaf, ditem) != key->offset) {
+		dev_item_err(fs_info, leaf, slot,
+			     "devid mismatch: key has=%llu item has=%llu",
+			     key->offset, btrfs_device_id(leaf, ditem));
+		goto error;
+	}
+
+	/*
+	 * Since btrfs device add doesn't check device size at all, we could
+	 * have device item whose size is smaller than 1M which is useless, but
+	 * still valid.
+	 * So here we can only check the obviously wrong case.
+	 */
+	if (btrfs_device_total_bytes(leaf, ditem) == 0) {
+		dev_item_err(fs_info, leaf, slot,
+			     "invalid total bytes: have 0");
+		goto error;
+	}
+	if (btrfs_device_bytes_used(leaf, ditem) >
+	    btrfs_device_total_bytes(leaf, ditem)) {
+		dev_item_err(fs_info, leaf, slot,
+			     "invalid bytes used: have %llu expect [0, %llu]",
+			     btrfs_device_bytes_used(leaf, ditem),
+			     btrfs_device_total_bytes(leaf, ditem));
+		goto error;
+	}
+	/*
+	 * Remaining members like io_align/type/gen/dev_group aren't really
+	 * utilized.
+	 * Skip them to make later usage of them easier.
+	 */
+	return 0;
+error:
+	return -EUCLEAN;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */
@@ -624,6 +705,9 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,
 		ret = btrfs_check_chunk_valid(fs_info, leaf, chunk,
 					      key->offset);
 		break;
+	case BTRFS_DEV_ITEM_KEY:
+		ret = check_dev_item(fs_info, leaf, key, slot);
+		break;
 	}
 	return ret;
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0e3822870f1e..0b839ecbe73f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4964,15 +4964,6 @@ static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type)
 	btrfs_set_fs_incompat(info, RAID56);
 }
 
-#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info)	\
-			- sizeof(struct btrfs_chunk))		\
-			/ sizeof(struct btrfs_stripe) + 1)
-
-#define BTRFS_MAX_DEVS_SYS_CHUNK ((BTRFS_SYSTEM_CHUNK_ARRAY_SIZE	\
-				- 2 * sizeof(struct btrfs_disk_key)	\
-				- 2 * sizeof(struct btrfs_chunk))	\
-				/ sizeof(struct btrfs_stripe) + 1)
-
 static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 			       u64 start, u64 type)
 {
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index ed806649a473..481c012eae79 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -258,6 +258,15 @@ struct btrfs_fs_devices {
 
 #define BTRFS_BIO_INLINE_CSUM_SIZE	64
 
+#define BTRFS_MAX_DEVS(info) ((BTRFS_MAX_ITEM_SIZE(info)	\
+			- sizeof(struct btrfs_chunk))		\
+			/ sizeof(struct btrfs_stripe) + 1)
+
+#define BTRFS_MAX_DEVS_SYS_CHUNK ((BTRFS_SYSTEM_CHUNK_ARRAY_SIZE	\
+				- 2 * sizeof(struct btrfs_disk_key)	\
+				- 2 * sizeof(struct btrfs_chunk))	\
+				/ sizeof(struct btrfs_stripe) + 1)
+
 /*
  * we need the mirror number and stripe index to be passed around
  * the call chain while we are processing end_io (especially errors).
-- 
2.21.0

[PATCH 3/3] btrfs: tree-checker: Fix NULL pointer access for corrupted chunk root

[Qu Wenruo]

[BUG]
For a special crafted image, kernel can cause NULL pointer dereference like:
  BUG: unable to handle kernel NULL pointer dereference at 0000000000000024
  #PF error: [normal kernel read fault]
  Oops: 0000 [#1] SMP PTI
  CPU: 0 PID: 146 Comm: kworker/u2:4 Not tainted 5.0.0-rc8+ #9
  Workqueue: btrfs-endio-meta btrfs_endio_meta_helper
  RIP: 0010:btrfs_root_node+0x10/0x50
  Call Trace:
   btrfs_read_lock_root_node+0x29/0x50
   btrfs_search_slot+0x529/0x920
   btrfs_find_root+0x56/0x240
   btrfs_read_tree_root+0x8b/0x130
   btrfs_read_fs_root+0x12/0x40
   btrfs_get_fs_root.part.49+0x53/0x170
   btrfs_get_fs_root+0x44/0xa0
   check_leaf+0xc0/0xa90
   btrfs_check_leaf_full+0x13/0x20
   btree_readpage_end_io_hook+0x242/0x290
   end_bio_extent_readpage+0x14f/0x660
   bio_endio+0xc4/0x140
   end_workqueue_fn+0x3d/0x40
   normal_work_helper+0xcb/0x320
   btrfs_endio_meta_helper+0x12/0x20
   process_one_work+0x167/0x410
   worker_thread+0x4d/0x460
   kthread+0x105/0x140
   ret_from_fork+0x35/0x40

[CAUSE]
Tree checker can be triggered when tree root is still not initialized.
This is for chunk tree read.

However if chunk tree is empty and has incorrect owner, then tree
checker will do comprehensive empty tree check.
This check involves search root tree to find the root, thus triggering
NULL pointer dereference as root tree is not yet initialized.

[FIX]
Just skip restrict owner check if tree root is not yet initialized.

Fix this fix, the corrupted image can be rejected as expected:
  BTRFS info (device loop0): disk space caching is enabled
  BTRFS info (device loop0): has skinny extents
  BTRFS error (device loop0): super_num_devices 1 mismatch with num_devices 0 found here
  BTRFS error (device loop0): failed to read chunk tree: -22
  BTRFS error (device loop0): open_ctree failed

Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202753
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 5ccb4be583ea..e01a84be768f 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -760,8 +760,12 @@ static int check_leaf(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf,
 		 * we can't use @owner as accurate owner indicator.
 		 * Case like balance and new tree block created for commit root
 		 * can break owner check easily.
+		 *
+		 * Also we could trigger tree checker before root tree
+		 * initialized (read chunk tree), skip strict owner check
+		 * if root tree is not initialized yet.
 		 */
-		if (!check_empty_leaf)
+		if (!check_empty_leaf || !fs_info->tree_root->node)
 			return 0;
 
 		key.objectid = owner;
-- 
2.21.0