[PATCh v2 0/9] btrfs: tree-checker: More enhancement for fuzzed

[PATCh v2 0/9] btrfs: tree-checker: More enhancement for fuzzed

[Qu Wenruo]

This patchset can be fetched from github:
It can be fetched from github:
https://github.com/adam900710/linux/tree/tree_checker_enhancement
Which is based on my previous write time tree checker patchset (based on
v5.1-rc1 tag)

Thanks for the report from Yoon Jungyeon <jungyeon@gatech.edu>, we have
more fuzzed image to torture btrfs.

Those images exposed the following problems:

- Chunk check is not comprehensive nor early enough
  Chunk item check lacks profile bits check (e.g RAID|DUP profile is
  invalid).
  And for certain fuzzed image, the other copy can be valid, current
  check timming is after tree block read, so no way to retry the other
  copy.

  Address the check timing in the 1st~4th patch, while for the profile bits,
  check it in the 7th patch.

- Lack of device item check
  Address it in the 5nd patch.

- First key and level check be exploited by cached extent buffer
  Cached bad extent buffer can avoid first key and level check.
  This is addressed in the 6rd patch.

- Inode type mismatch can lead to NULL dereference in endio function
  If an inode claims itself as symlink but still has regular file
  extent, then endio function will cause NULL pointer dereference.
  Fix it by do extra inode mode and dir item type cross check, at
  get_extent() time and inode lookup time.
  Addressed in the last 2 patches.

Changelog:
v2:
- Split patches for btrfs_check_chunk_valid() merge into tree-checker.
- Rebase to v5.1-rc1 based write_time_tree_checker branch.
- Add reviewed-by tags.

Qu Wenruo (9):
  btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it
  btrfs: tree-checker: Make chunk item checker more readable
  btrfs: tree-checker: Make btrfs_check_chunk_valid() return EUCLEAN
    instead of EIO
  btrfs: tree-checker: Check chunk item at tree block read time
  btrfs: tree-checker: Verify dev item
  btrfs: Check the first key and level for cached extent buffer
  btrfs: tree-checker: Enhance chunk checker to validate chunk profiler
  btrfs: tree-checker: Verify inode item
  btrfs: inode: Verify inode mode to avoid NULL pointer dereference

 fs/btrfs/ctree.c             |  10 +
 fs/btrfs/ctree.h             |   2 +
 fs/btrfs/disk-io.c           |  10 +-
 fs/btrfs/disk-io.h           |   3 +
 fs/btrfs/inode.c             |  38 +++-
 fs/btrfs/tests/inode-tests.c |   1 +
 fs/btrfs/tree-checker.c      | 349 +++++++++++++++++++++++++++++++++++
 fs/btrfs/tree-checker.h      |   3 +
 fs/btrfs/volumes.c           | 115 +-----------
 fs/btrfs/volumes.h           |   9 +
 10 files changed, 422 insertions(+), 118 deletions(-)

-- 
2.21.0

[PATCh v2 1/9] btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it

[Qu Wenruo]

By function, chunk item verification is more suitable to be done inside
tree-checker.

So move btrfs_check_chunk_valid() to tree-checker.c and export it.

And since it's now moved to tree-checker, also add a better comment for
what this function is doing.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 99 +++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/tree-checker.h |  3 ++
 fs/btrfs/volumes.c      | 94 +-------------------------------------
 3 files changed, 103 insertions(+), 93 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index b8cdaf472031..4e44323ae758 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -448,6 +448,105 @@ static int check_block_group_item(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+/*
+ * 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) {
+		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 != 2) ||
+	    (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;
+}
+
+
 /*
  * Common point to switch the item-specific validation.
  */
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 9024eee889b9..2a4ac5aa2a16 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] = {
@@ -6714,99 +6715,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 != 2) ||
-	    (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

[PATCh v2 2/9] btrfs: tree-checker: Make chunk item checker more readable

[Qu Wenruo]

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

And for certain error message, also output expected value.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 81 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 4e44323ae758..c2e321d2b7bc 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -448,6 +448,51 @@ 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, ...)
+{
+	bool is_sb;
+	struct va_format vaf;
+	va_list args;
+	int i;
+	int slot = -1;
+
+	/* Only superblock eb is able to have such small offset */
+	is_sb = (leaf->start == BTRFS_SUPER_INFO_OFFSET);
+
+	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.
  *
@@ -473,31 +518,38 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	type = btrfs_chunk_type(leaf, chunk);
 
 	if (!num_stripes) {
-		btrfs_err(fs_info, "invalid chunk num_stripes: %u",
-			  num_stripes);
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk num_stripes, have %u", num_stripes);
 		return -EIO;
 	}
 	if (!IS_ALIGNED(logical, fs_info->sectorsize)) {
-		btrfs_err(fs_info, "invalid chunk logical %llu", logical);
+		chunk_err(fs_info, leaf, chunk, logical,
+		"invalid chunk logical, have %llu should aligned to %u",
+			  logical, fs_info->sectorsize);
 		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));
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk sectorsize, have %u expect %u",
+			  btrfs_chunk_sector_size(leaf, chunk),
+			  fs_info->sectorsize);
 		return -EIO;
 	}
 	if (!length || !IS_ALIGNED(length, fs_info->sectorsize)) {
-		btrfs_err(fs_info, "invalid chunk length %llu", length);
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "invalid chunk length, have %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",
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "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",
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "unrecognized chunk type: 0x%llx",
 			  ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
 			    BTRFS_BLOCK_GROUP_PROFILE_MASK) &
 			  btrfs_chunk_type(leaf, chunk));
@@ -505,14 +557,17 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	}
 
 	if ((type & BTRFS_BLOCK_GROUP_TYPE_MASK) == 0) {
-		btrfs_err(fs_info, "missing chunk type flag: 0x%llx", type);
+		chunk_err(fs_info, leaf, chunk, logical,
+	"missing chunk type flag, have 0x%llx one bit must be set in 0x%llx",
+			  type, BTRFS_BLOCK_GROUP_TYPE_MASK);
 		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);
+		chunk_err(fs_info, leaf, chunk, logical,
+			  "system chunk with data or metadata type: 0x%llx",
+			  type);
 		return -EIO;
 	}
 
@@ -523,7 +578,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	if (!mixed) {
 		if ((type & BTRFS_BLOCK_GROUP_METADATA) &&
 		    (type & BTRFS_BLOCK_GROUP_DATA)) {
-			btrfs_err(fs_info,
+			chunk_err(fs_info, leaf, chunk, logical,
 			"mixed chunk type in non-mixed mode: 0x%llx", type);
 			return -EIO;
 		}
@@ -536,7 +591,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	    (type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 2) ||
 	    ((type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
 	     num_stripes != 1)) {
-		btrfs_err(fs_info,
+		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);
-- 
2.21.0

[PATCh v2 3/9] btrfs: tree-checker: Make btrfs_check_chunk_valid() return EUCLEAN instead of EIO

[Qu Wenruo]

To follow the standard behavior of tree-checker.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index c2e321d2b7bc..63fbe0b5ae8e 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -520,31 +520,31 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	if (!num_stripes) {
 		chunk_err(fs_info, leaf, chunk, logical,
 			  "invalid chunk num_stripes, have %u", num_stripes);
-		return -EIO;
+		return -EUCLEAN;
 	}
 	if (!IS_ALIGNED(logical, fs_info->sectorsize)) {
 		chunk_err(fs_info, leaf, chunk, logical,
 		"invalid chunk logical, have %llu should aligned to %u",
 			  logical, fs_info->sectorsize);
-		return -EIO;
+		return -EUCLEAN;
 	}
 	if (btrfs_chunk_sector_size(leaf, chunk) != fs_info->sectorsize) {
 		chunk_err(fs_info, leaf, chunk, logical,
 			  "invalid chunk sectorsize, have %u expect %u",
 			  btrfs_chunk_sector_size(leaf, chunk),
 			  fs_info->sectorsize);
-		return -EIO;
+		return -EUCLEAN;
 	}
 	if (!length || !IS_ALIGNED(length, fs_info->sectorsize)) {
 		chunk_err(fs_info, leaf, chunk, logical,
 			  "invalid chunk length, have %llu", length);
-		return -EIO;
+		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 -EIO;
+		return -EUCLEAN;
 	}
 	if (~(BTRFS_BLOCK_GROUP_TYPE_MASK | BTRFS_BLOCK_GROUP_PROFILE_MASK) &
 	    type) {
@@ -553,14 +553,14 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 			  ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
 			    BTRFS_BLOCK_GROUP_PROFILE_MASK) &
 			  btrfs_chunk_type(leaf, chunk));
-		return -EIO;
+		return -EUCLEAN;
 	}
 
 	if ((type & BTRFS_BLOCK_GROUP_TYPE_MASK) == 0) {
 		chunk_err(fs_info, leaf, chunk, logical,
 	"missing chunk type flag, have 0x%llx one bit must be set in 0x%llx",
 			  type, BTRFS_BLOCK_GROUP_TYPE_MASK);
-		return -EIO;
+		return -EUCLEAN;
 	}
 
 	if ((type & BTRFS_BLOCK_GROUP_SYSTEM) &&
@@ -568,7 +568,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 		chunk_err(fs_info, leaf, chunk, logical,
 			  "system chunk with data or metadata type: 0x%llx",
 			  type);
-		return -EIO;
+		return -EUCLEAN;
 	}
 
 	features = btrfs_super_incompat_flags(fs_info->super_copy);
@@ -580,7 +580,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 		    (type & BTRFS_BLOCK_GROUP_DATA)) {
 			chunk_err(fs_info, leaf, chunk, logical,
 			"mixed chunk type in non-mixed mode: 0x%llx", type);
-			return -EIO;
+			return -EUCLEAN;
 		}
 	}
 
@@ -595,7 +595,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *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 -EUCLEAN;
 	}
 
 	return 0;
-- 
2.21.0

[PATCh v2 4/9] btrfs: tree-checker: Check chunk item at tree block read time

[Qu Wenruo]

Since we have btrfs_check_chunk_valid() in tree-checker, let's do
chunk item verification in tree-checker too.

Since the tree-checker is run at endio time, if one chunk leaf fails
chunk verification, we can still retry the other copy, making btrfs more
robust to fuzzed image as we may still get a good chunk item.

Also since we have done chunk verification in tree block read time, skip
the btrfs_check_chunk_valid() call in read_one_chunk() if we're reading
chunk items from leaf.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c |  6 ++++++
 fs/btrfs/volumes.c      | 12 +++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 63fbe0b5ae8e..86dee12d7917 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -610,6 +610,7 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,
 			   struct btrfs_key *key, int slot)
 {
 	int ret = 0;
+	struct btrfs_chunk *chunk;
 
 	switch (key->type) {
 	case BTRFS_EXTENT_DATA_KEY:
@@ -626,6 +627,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/volumes.c b/fs/btrfs/volumes.c
index 2a4ac5aa2a16..645ffc9c47b0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6745,9 +6745,15 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key,
 	length = btrfs_chunk_length(leaf, chunk);
 	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
 
-	ret = btrfs_check_chunk_valid(fs_info, leaf, chunk, logical);
-	if (ret)
-		return ret;
+	/*
+	 * Only need to verify chunk item if we're reading from sys chunk array,
+	 * as chunk item in tree block is already verified by tree-checker.
+	 */
+	if (leaf->start == BTRFS_SUPER_INFO_OFFSET) {
+		ret = btrfs_check_chunk_valid(fs_info, leaf, chunk, logical);
+		if (ret)
+			return ret;
+	}
 
 	read_lock(&map_tree->map_tree.lock);
 	em = lookup_extent_mapping(&map_tree->map_tree, logical, 1);
-- 
2.21.0

[PATCh v2 5/9] 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>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/tree-checker.c | 83 +++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c      |  9 -----
 fs/btrfs/volumes.h      |  9 +++++
 3 files changed, 92 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 86dee12d7917..004219175f0c 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -602,6 +602,86 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 }
 
 
+__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.
  */
@@ -632,6 +712,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 645ffc9c47b0..7510272408e8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4958,15 +4958,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 3ad9d58d1b66..38ed94b77202 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 v2 6/9] btrfs: Check the first key and level for cached extent buffer

[Qu Wenruo]

[BUG]
When reading a file from a fuzzed image, kernel can panic like:
  BTRFS warning (device loop0): csum failed root 5 ino 270 off 0 csum 0x98f94189 expected csum 0x00000000 mirror 1
  assertion failed: !memcmp_extent_buffer(b, &disk_key, offsetof(struct btrfs_leaf, items[0].key), sizeof(disk_key)), file: fs/btrfs/ctree.c, line: 2544
  ------------[ cut here ]------------
  kernel BUG at fs/btrfs/ctree.h:3500!
  invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
  RIP: 0010:btrfs_search_slot.cold.24+0x61/0x63 [btrfs]
  Call Trace:
   btrfs_lookup_csum+0x52/0x150 [btrfs]
   __btrfs_lookup_bio_sums+0x209/0x640 [btrfs]
   btrfs_submit_bio_hook+0x103/0x170 [btrfs]
   submit_one_bio+0x59/0x80 [btrfs]
   extent_read_full_page+0x58/0x80 [btrfs]
   generic_file_read_iter+0x2f6/0x9d0
   __vfs_read+0x14d/0x1a0
   vfs_read+0x8d/0x140
   ksys_read+0x52/0xc0
   do_syscall_64+0x60/0x210
   entry_SYSCALL_64_after_hwframe+0x49/0xbe

[CAUSE]
The fuzzed image has a corrupted leaf whose first key doesn't match with its parent:
  checksum tree key (CSUM_TREE ROOT_ITEM 0)
  node 29741056 level 1 items 14 free 107 generation 19 owner CSUM_TREE
  fs uuid 3381d111-94a3-4ac7-8f39-611bbbdab7e6
  chunk uuid 9af1c3c7-2af5-488b-8553-530bd515f14c
  	...
          key (EXTENT_CSUM EXTENT_CSUM 79691776) block 29761536 gen 19

  leaf 29761536 items 1 free space 1726 generation 19 owner CSUM_TREE
  leaf 29761536 flags 0x1(WRITTEN) backref revision 1
  fs uuid 3381d111-94a3-4ac7-8f39-611bbbdab7e6
  chunk uuid 9af1c3c7-2af5-488b-8553-530bd515f14c
          item 0 key (EXTENT_CSUM EXTENT_CSUM 8798638964736) itemoff 1751 itemsize 2244
                  range start 8798638964736 end 8798641262592 length 2297856

When reading above tree block, we have extent_buffer->refs = 2 in the
context:
- initial one from __alloc_extent_buffer()
  alloc_extent_buffer()
  |- __alloc_extent_buffer()
     |- atomic_set(&eb->refs, 1)

- one being added to fs_info->buffer_radix
  alloc_extent_buffer()
  |- check_buffer_tree_ref()
     |- atomic_inc(&eb->refs)

So even we call free_extent_buffer() in read_tree_block or other similar
situation, we only decrease the refs by 1, it doesn't reach 0 and won't
be freed right now.

The staled eb and its corrupted content will still be kept cached.

Further more, we have several extra cases where we either don't do
first key check or the check is not proper for all callers:
- scrub
  We just don't have first key in this context.

- shared tree block
  One tree block can be shared by several snapshot/subvolume trees.
  In that case, the first key check for one subvolume doesn't apply to
  another.

So for above reasons, a corrupted extent buffer can sneak into the
buffer cache.

[FIX]
Export verify_level_key() as btrfs_verify_level_key() and call it in
read_block_for_search() to fill the hole.

Due to above described reasons, even we can free corrupted extent buffer
from cache, we still need the check in read_block_for_search(), for
scrub and shared tree blocks.

Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202755
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202757
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202759
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202761
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202767
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202769
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c   | 10 ++++++++++
 fs/btrfs/disk-io.c | 10 +++++-----
 fs/btrfs/disk-io.h |  3 +++
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 324df36d28bf..65b12963e72b 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2416,6 +2416,16 @@ read_block_for_search(struct btrfs_root *root, struct btrfs_path *p,
 	if (tmp) {
 		/* first we do an atomic uptodate check */
 		if (btrfs_buffer_uptodate(tmp, gen, 1) > 0) {
+			/*
+			 * Do extra check for first_key, eb can be stale due to
+			 * being cached, read from scrub, or have multiple
+			 * parents (shared tree blocks).
+			 */
+			if (btrfs_verify_level_key(fs_info, tmp,
+					parent_level - 1, &first_key, gen)) {
+				free_extent_buffer(tmp);
+				return -EUCLEAN;
+			}
 			*eb_ret = tmp;
 			return 0;
 		}
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 327a98ca0d14..8b207419c685 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -424,9 +424,9 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
-static int verify_level_key(struct btrfs_fs_info *fs_info,
-			    struct extent_buffer *eb, int level,
-			    struct btrfs_key *first_key, u64 parent_transid)
+int btrfs_verify_level_key(struct btrfs_fs_info *fs_info,
+			   struct extent_buffer *eb, int level,
+			   struct btrfs_key *first_key, u64 parent_transid)
 {
 	int found_level;
 	struct btrfs_key found_key;
@@ -501,8 +501,8 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
 			if (verify_parent_transid(io_tree, eb,
 						   parent_transid, 0))
 				ret = -EIO;
-			else if (verify_level_key(fs_info, eb, level,
-						  first_key, parent_transid))
+			else if (btrfs_verify_level_key(fs_info, eb, level,
+						first_key, parent_transid))
 				ret = -EUCLEAN;
 			else
 				break;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 987a64bc0c66..67a9fe2d29c7 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -39,6 +39,9 @@ static inline u64 btrfs_sb_offset(int mirror)
 struct btrfs_device;
 struct btrfs_fs_devices;
 
+int btrfs_verify_level_key(struct btrfs_fs_info *fs_info,
+			   struct extent_buffer *eb, int level,
+			   struct btrfs_key *first_key, u64 parent_transid);
 struct extent_buffer *read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
 				      u64 parent_transid, int level,
 				      struct btrfs_key *first_key);
-- 
2.21.0

[PATCh v2 7/9] btrfs: tree-checker: Enhance chunk checker to validate chunk profiler

[Qu Wenruo]

Btrfs-progs already has comprehensive type checker, to ensure there is
only 0 (SINGLE profile) or 1 (DUP/RAID0/1/5/6/10) bit set for chunk
profile bits.

Do the same work for kernel.

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

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 004219175f0c..fef0cd8c90a5 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -556,6 +556,13 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 		return -EUCLEAN;
 	}
 
+	if (!is_power_of_2(type & BTRFS_BLOCK_GROUP_PROFILE_MASK) &&
+	    (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) != 0) {
+		chunk_err(fs_info, leaf, chunk, logical,
+		"invalid chunk profile flag: 0x%llx, expect 0 or 1 bit set",
+			  type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
+		return -EUCLEAN;
+	}
 	if ((type & BTRFS_BLOCK_GROUP_TYPE_MASK) == 0) {
 		chunk_err(fs_info, leaf, chunk, logical,
 	"missing chunk type flag, have 0x%llx one bit must be set in 0x%llx",
-- 
2.21.0

[PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

There is a report in kernel bugzilla about mismatch file type in dir
item and inode item.

This inspires us to check inode mode in inode item.

This patch will check the following members:
- inode key objectid
  Should be ROOT_DIR_DIR or [256, (u64)-256] or FREE_INO.

- inode key offset
  Should be 0

- inode item generation
- inode item transid
  No newer than sb generation + 1.
  The +1 is for log tree.

- inode item mode
  No unknown bits.
  No invalid S_IF* bit.
  NOTE: S_IFMT check is not enough, need to check every know type.

- inode item nlink
  Dir should have no more link than 1.

- inode item flags

Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h        |  2 +
 fs/btrfs/tree-checker.c | 99 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index b3642367a595..b0f19cc56485 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1539,6 +1539,8 @@ do {                                                                   \
 #define BTRFS_INODE_COMPRESS		(1 << 11)
 
 #define BTRFS_INODE_ROOT_ITEM_INIT	(1 << 31)
+#define BTRFS_INODE_FLAG_MASK		(((1 << 12) - 1) |\
+					 BTRFS_INODE_ROOT_ITEM_INIT)
 
 struct btrfs_map_token {
 	const struct extent_buffer *eb;
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index fef0cd8c90a5..fa0ad9e7de6e 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -689,6 +689,102 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
 	return -EUCLEAN;
 }
 
+/* Inode item error output has the same format as dir_item_err() */
+#define inode_item_err(fs_info, eb, slot, fmt, ...)	\
+	dir_item_err(fs_info, eb, slot, fmt, __VA_ARGS__)
+
+static int check_inode_item(struct btrfs_fs_info *fs_info,
+			    struct extent_buffer *leaf,
+			    struct btrfs_key *key, int slot)
+{
+	struct btrfs_inode_item *iitem;
+	u64 super_gen = btrfs_super_generation(fs_info->super_copy);
+	u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777);
+	u32 mode;
+
+	if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID ||
+	     key->objectid > BTRFS_LAST_FREE_OBJECTID) &&
+	     key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
+	     key->objectid != BTRFS_FREE_INO_OBJECTID) {
+		generic_err(fs_info, leaf, slot,
+	"invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu",
+			    key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID,
+			    BTRFS_FIRST_FREE_OBJECTID,
+			    BTRFS_LAST_FREE_OBJECTID,
+			    BTRFS_FREE_INO_OBJECTID);
+		goto error;
+	}
+	if (key->offset != 0) {
+		inode_item_err(fs_info, leaf, slot,
+			"invalid key offset: has %llu expect 0",
+			key->offset);
+		goto error;
+	}
+	iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item);
+
+	/* Here we use super block generation + 1 to handle log tree */
+	if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
+		inode_item_err(fs_info, leaf, slot,
+			"invalid inode generation: has %llu expect (0, %llu]",
+			       btrfs_inode_generation(leaf, iitem),
+			       super_gen + 1);
+		goto error;
+	}
+	/* Note for ROOT_TREE_DIR_ITEM, mkfs could make its transid as 0 */
+	if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
+		inode_item_err(fs_info, leaf, slot,
+			"invalid inode generation: has %llu expect [0, %llu]",
+			       btrfs_inode_transid(leaf, iitem),
+			       super_gen + 1);
+		goto error;
+	}
+
+	/*
+	 * For size and nbytes it's better not to be too strict, as for dir
+	 * item its size/nbytes can easily get wrong, but doesn't affect
+	 * any thing of the fs. So here we skip the check.
+	 */
+
+	mode = btrfs_inode_mode(leaf, iitem);
+	if (mode & ~valid_mask) {
+		inode_item_err(fs_info, leaf, slot,
+			       "unknown mode bit detected: 0x%x",
+			       mode & ~valid_mask);
+		goto error;
+	}
+
+	/*
+	 * S_IFMT is not bit mapped so we can't completely rely is_power_of_2(),
+	 * but is_power_of_2() can save us from checking FIFO/CHR/DIR/REG.
+	 * Only needs to check BLK, LNK and SOCKS
+	 */
+	if (!is_power_of_2(mode & S_IFMT)) {
+		if (!S_ISLNK(mode) && ! S_ISBLK(mode) && !S_ISSOCK(mode)) {
+			inode_item_err(fs_info, leaf, slot,
+			"invalid mode: has 0%o expect valid S_IF* bit(s)",
+				       mode & S_IFMT);
+			goto error;
+		}
+	}
+	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
+		inode_item_err(fs_info, leaf, slot,
+			       "invalid nlink: has %u expect no more than 1 for dir",
+			btrfs_inode_nlink(leaf, iitem));
+		goto error;
+	}
+	if (btrfs_inode_flags(leaf, iitem) & ~BTRFS_INODE_FLAG_MASK) {
+		inode_item_err(fs_info, leaf, slot,
+			       "unknown flags detected: 0x%llx",
+			       btrfs_inode_flags(leaf, iitem) &
+			       ~BTRFS_INODE_FLAG_MASK);
+		goto error;
+	}
+	return 0;
+
+error:
+	return -EUCLEAN;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */
@@ -722,6 +818,9 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,
 	case BTRFS_DEV_ITEM_KEY:
 		ret = check_dev_item(fs_info, leaf, key, slot);
 		break;
+	case BTRFS_INODE_ITEM_KEY:
+		ret = check_inode_item(fs_info, leaf, key, slot);
+		break;
 	}
 	return ret;
 }
-- 
2.21.0

[PATCh v2 9/9] btrfs: inode: Verify inode mode to avoid NULL pointer dereference

[Qu Wenruo]

[BUG]
When access a file on a crafted image, btrfs can crash in block layer:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
PGD 136501067 P4D 136501067 PUD 124519067 PMD 0
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.0.0-rc8-default #252
RIP: 0010:end_bio_extent_readpage+0x144/0x700
Call Trace:
 <IRQ>
 blk_update_request+0x8f/0x350
 blk_mq_end_request+0x1a/0x120
 blk_done_softirq+0x99/0xc0
 __do_softirq+0xc7/0x467
 irq_exit+0xd1/0xe0
 call_function_single_interrupt+0xf/0x20
 </IRQ>
RIP: 0010:default_idle+0x1e/0x170

[CAUSE]
The crafted image has a pretty tricky corruption, the INODE_ITEM has a
different type against its parent dir:
        item 20 key (268 INODE_ITEM 0) itemoff 2808 itemsize 160
                generation 13 transid 13 size 1048576 nbytes 1048576
                block group 0 mode 121644 links 1 uid 0 gid 0 rdev 0
                sequence 9 flags 0x0(none)

This mode number 0120000 means it's a soft link.

But the dir item think it's still a regular file:
        item 8 key (264 DIR_INDEX 5) itemoff 3707 itemsize 32
                location key (268 INODE_ITEM 0) type FILE
                transid 13 data_len 0 name_len 2
                name: f4
        item 40 key (264 DIR_ITEM 51821248) itemoff 1573 itemsize 32
                location key (268 INODE_ITEM 0) type FILE
                transid 13 data_len 0 name_len 2
                name: f4

For btrfs symlink, we don't set BTRFS_I(inode)->io_tree.ops and leave
it empty, as symlink is only designed to have inlined extent, all
handled by tree block read.
Thus no need to trigger btrfs_submit_bio_hook() for inline file extent.

However for end_bio_extent_readpage() it expects tree->ops populated, as
it's reading regular data extent.
This causes NULL pointer dereference.

[FIX]
This patch fixes the problem by 2 directions:
- Verify inode mode against its dir item when looking up inode
  So in btrfs_lookup_dentry() if we find inode mode mismatch with dir
  item, we error out so that corrupted inode will not be access.

- Verify inode mode when getting extent mapping
  Only regular file should have regular or preallocated extent.
  If we found regular/preallocated file extent for soft link or
  whatever, we error out before we submit read bio.

With this fix that crafted image can be rejected gracefully:
  BTRFS critical (device loop0): inode mode mismatch with dir: inode mode=0121644 btrfs type=7 dir type=1

Reported-by: Yoon Jungyeon <jungyeon@gatech.edu>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202763
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c             | 38 ++++++++++++++++++++++++++++--------
 fs/btrfs/tests/inode-tests.c |  1 +
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 82fdda8ff5ab..2e16b7779217 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5448,12 +5448,13 @@ void btrfs_evict_inode(struct inode *inode)
 }
 
 /*
- * this returns the key found in the dir entry in the location pointer.
+ * this returns the key found in the dir entry in the location pointer,
+ * fill @type with BTRFS_FT_*, and return 0.
  * If no dir entries were found, returns -ENOENT.
  * If found a corrupted location in dir entry, returns -EUCLEAN.
  */
 static int btrfs_inode_by_name(struct inode *dir, struct dentry *dentry,
-			       struct btrfs_key *location)
+			       struct btrfs_key *location, u8 *type)
 {
 	const char *name = dentry->d_name.name;
 	int namelen = dentry->d_name.len;
@@ -5482,6 +5483,8 @@ static int btrfs_inode_by_name(struct inode *dir, struct dentry *dentry,
 			   __func__, name, btrfs_ino(BTRFS_I(dir)),
 			   location->objectid, location->type, location->offset);
 	}
+	if (!ret)
+		*type = btrfs_dir_type(path->nodes[0], di);
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -5719,6 +5722,11 @@ static struct inode *new_simple_dir(struct super_block *s,
 	return inode;
 }
 
+static inline u8 btrfs_inode_type(struct inode *inode)
+{
+	return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT];
+}
+
 struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb);
@@ -5726,18 +5734,29 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
 	struct btrfs_root *root = BTRFS_I(dir)->root;
 	struct btrfs_root *sub_root = root;
 	struct btrfs_key location;
+	u8 di_type = 0;
 	int index;
 	int ret = 0;
 
 	if (dentry->d_name.len > BTRFS_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	ret = btrfs_inode_by_name(dir, dentry, &location);
+	ret = btrfs_inode_by_name(dir, dentry, &location, &di_type);
 	if (ret < 0)
 		return ERR_PTR(ret);
 
 	if (location.type == BTRFS_INODE_ITEM_KEY) {
 		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
+
+		/* Do extra check against inode mode with di_type */
+		if (btrfs_inode_type(inode) != di_type) {
+			btrfs_crit(fs_info,
+"inode mode mismatch with dir: inode mode=0%o btrfs type=%u dir type=%u",
+				  inode->i_mode, btrfs_inode_type(inode),
+				  di_type);
+			iput(inode);
+			return ERR_PTR(-EUCLEAN);
+		}
 		return inode;
 	}
 
@@ -6342,11 +6361,6 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	return ERR_PTR(ret);
 }
 
-static inline u8 btrfs_inode_type(struct inode *inode)
-{
-	return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT];
-}
-
 /*
  * utility function to add 'inode' into 'parent_inode' with
  * a give name and a given sequence number.
@@ -6864,6 +6878,14 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	extent_start = found_key.offset;
 	if (extent_type == BTRFS_FILE_EXTENT_REG ||
 	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
+		/* Only regular file could have regular/prealloc extent */
+		if (!S_ISREG(inode->vfs_inode.i_mode)) {
+			ret = -EUCLEAN;
+			btrfs_crit(fs_info,
+		"regular/prealloc extent found for non-regular inode %llu",
+				   btrfs_ino(inode));
+			goto out;
+		}
 		extent_end = extent_start +
 		       btrfs_file_extent_num_bytes(leaf, item);
 
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index af0c8e30d9e2..b01dbcab9eed 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -232,6 +232,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
 		return ret;
 	}
 
+	inode->i_mode = S_IFREG;
 	BTRFS_I(inode)->location.type = BTRFS_INODE_ITEM_KEY;
 	BTRFS_I(inode)->location.objectid = BTRFS_FIRST_FREE_OBJECTID;
 	BTRFS_I(inode)->location.offset = 0;
-- 
2.21.0

Re: [PATCh v2 1/9] btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 2/9] btrfs: tree-checker: Make chunk item checker more readable

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Although I think it would've been worth to explicitly mention that you
increased the severity level from error to critical.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 3/9] btrfs: tree-checker: Make btrfs_check_chunk_valid() return EUCLEAN instead of EIO

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 4/9] btrfs: tree-checker: Check chunk item at tree block read time

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 5/9] btrfs: tree-checker: Verify dev item

[Johannes Thumshirn]

On 20/03/2019 07:37, Qu Wenruo wrote:
[...]

> +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;
> +}
> +

Why aren't you directly returning -EUCLEAN instead of the gotos? There's
no cleanup pending so the additional jump label is unnecessary.



-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 5/9] btrfs: tree-checker: Verify dev item

[Qu Wenruo]

On 2019/3/20 下午7:51, Johannes Thumshirn wrote:
> On 20/03/2019 07:37, Qu Wenruo wrote:
> [...]
> 
>> +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;
>> +}
>> +
> 
> Why aren't you directly returning -EUCLEAN instead of the gotos? There's
> no cleanup pending so the additional jump label is unnecessary.

Just a coding preference.

Will it impact the performance or compiler is clever enough to change
the goto line to return -EUCLEAN?

Thanks,
Qu

Re: [PATCh v2 6/9] btrfs: Check the first key and level for cached extent buffer

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 7/9] btrfs: tree-checker: Enhance chunk checker to validate chunk profiler

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Although I prefer direct returns instead of gotos if there's no cleanup.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 9/9] btrfs: inode: Verify inode mode to avoid NULL pointer dereference

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

On 2019/3/20 下午2:37, Qu Wenruo wrote:
> There is a report in kernel bugzilla about mismatch file type in dir
> item and inode item.
>
> This inspires us to check inode mode in inode item.
>
> This patch will check the following members:
> - inode key objectid
>   Should be ROOT_DIR_DIR or [256, (u64)-256] or FREE_INO.
>
> - inode key offset
>   Should be 0
>
> - inode item generation
> - inode item transid
>   No newer than sb generation + 1.
>   The +1 is for log tree.
>
> - inode item mode
>   No unknown bits.
>   No invalid S_IF* bit.
>   NOTE: S_IFMT check is not enough, need to check every know type.
>
> - inode item nlink
>   Dir should have no more link than 1.
>
> - inode item flags
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

There is some bug report of kernel producing free space cache inode with
mode 0, which is invalid and can be detected by this patch.

Although the patch itself is good, I'm afraid we need to address the
invalid inode mode created by old kernel in btrfs-progs at least before
merging this patch into upstream.

Thankfully we still have one release cycle to handle it.

Thanks,
Qu

> ---
>  fs/btrfs/ctree.h        |  2 +
>  fs/btrfs/tree-checker.c | 99 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index b3642367a595..b0f19cc56485 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1539,6 +1539,8 @@ do {                                                                   \
>  #define BTRFS_INODE_COMPRESS		(1 << 11)
>
>  #define BTRFS_INODE_ROOT_ITEM_INIT	(1 << 31)
> +#define BTRFS_INODE_FLAG_MASK		(((1 << 12) - 1) |\
> +					 BTRFS_INODE_ROOT_ITEM_INIT)
>
>  struct btrfs_map_token {
>  	const struct extent_buffer *eb;
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index fef0cd8c90a5..fa0ad9e7de6e 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -689,6 +689,102 @@ static int check_dev_item(struct btrfs_fs_info *fs_info,
>  	return -EUCLEAN;
>  }
>
> +/* Inode item error output has the same format as dir_item_err() */
> +#define inode_item_err(fs_info, eb, slot, fmt, ...)	\
> +	dir_item_err(fs_info, eb, slot, fmt, __VA_ARGS__)
> +
> +static int check_inode_item(struct btrfs_fs_info *fs_info,
> +			    struct extent_buffer *leaf,
> +			    struct btrfs_key *key, int slot)
> +{
> +	struct btrfs_inode_item *iitem;
> +	u64 super_gen = btrfs_super_generation(fs_info->super_copy);
> +	u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777);
> +	u32 mode;
> +
> +	if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID ||
> +	     key->objectid > BTRFS_LAST_FREE_OBJECTID) &&
> +	     key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
> +	     key->objectid != BTRFS_FREE_INO_OBJECTID) {
> +		generic_err(fs_info, leaf, slot,
> +	"invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu",
> +			    key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID,
> +			    BTRFS_FIRST_FREE_OBJECTID,
> +			    BTRFS_LAST_FREE_OBJECTID,
> +			    BTRFS_FREE_INO_OBJECTID);
> +		goto error;
> +	}
> +	if (key->offset != 0) {
> +		inode_item_err(fs_info, leaf, slot,
> +			"invalid key offset: has %llu expect 0",
> +			key->offset);
> +		goto error;
> +	}
> +	iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item);
> +
> +	/* Here we use super block generation + 1 to handle log tree */
> +	if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			"invalid inode generation: has %llu expect (0, %llu]",
> +			       btrfs_inode_generation(leaf, iitem),
> +			       super_gen + 1);
> +		goto error;
> +	}
> +	/* Note for ROOT_TREE_DIR_ITEM, mkfs could make its transid as 0 */
> +	if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			"invalid inode generation: has %llu expect [0, %llu]",
> +			       btrfs_inode_transid(leaf, iitem),
> +			       super_gen + 1);
> +		goto error;
> +	}
> +
> +	/*
> +	 * For size and nbytes it's better not to be too strict, as for dir
> +	 * item its size/nbytes can easily get wrong, but doesn't affect
> +	 * any thing of the fs. So here we skip the check.
> +	 */
> +
> +	mode = btrfs_inode_mode(leaf, iitem);
> +	if (mode & ~valid_mask) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "unknown mode bit detected: 0x%x",
> +			       mode & ~valid_mask);
> +		goto error;
> +	}
> +
> +	/*
> +	 * S_IFMT is not bit mapped so we can't completely rely is_power_of_2(),
> +	 * but is_power_of_2() can save us from checking FIFO/CHR/DIR/REG.
> +	 * Only needs to check BLK, LNK and SOCKS
> +	 */
> +	if (!is_power_of_2(mode & S_IFMT)) {
> +		if (!S_ISLNK(mode) && ! S_ISBLK(mode) && !S_ISSOCK(mode)) {
> +			inode_item_err(fs_info, leaf, slot,
> +			"invalid mode: has 0%o expect valid S_IF* bit(s)",
> +				       mode & S_IFMT);
> +			goto error;
> +		}
> +	}
> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "invalid nlink: has %u expect no more than 1 for dir",
> +			btrfs_inode_nlink(leaf, iitem));
> +		goto error;
> +	}
> +	if (btrfs_inode_flags(leaf, iitem) & ~BTRFS_INODE_FLAG_MASK) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "unknown flags detected: 0x%llx",
> +			       btrfs_inode_flags(leaf, iitem) &
> +			       ~BTRFS_INODE_FLAG_MASK);
> +		goto error;
> +	}
> +	return 0;
> +
> +error:
> +	return -EUCLEAN;
> +}
> +
>  /*
>   * Common point to switch the item-specific validation.
>   */
> @@ -722,6 +818,9 @@ static int check_leaf_item(struct btrfs_fs_info *fs_info,
>  	case BTRFS_DEV_ITEM_KEY:
>  		ret = check_dev_item(fs_info, leaf, key, slot);
>  		break;
> +	case BTRFS_INODE_ITEM_KEY:
> +		ret = check_inode_item(fs_info, leaf, key, slot);
> +		break;
>  	}
>  	return ret;
>  }
>

Re: [PATCh v2 5/9] btrfs: tree-checker: Verify dev item

[David Sterba]

On Wed, Mar 20, 2019 at 07:53:18PM +0800, Qu Wenruo wrote:
> >> +error:
> >> +	return -EUCLEAN;
> >> +}
> >> +
> > 
> > Why aren't you directly returning -EUCLEAN instead of the gotos? There's
> > no cleanup pending so the additional jump label is unnecessary.
> 
> Just a coding preference.
> 
> Will it impact the performance or compiler is clever enough to change
> the goto line to return -EUCLEAN?

This is a matter of codingstyle, it should be consistent, so if one
function uses return -EUCLEAN and another goto error:, then one of them
has to be changed.

Re: [PATCh v2 1/9] btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it

[David Sterba]

On Wed, Mar 20, 2019 at 02:37:09PM +0800, Qu Wenruo wrote:
> By function, chunk item verification is more suitable to be done inside
> tree-checker.
> 
> So move btrfs_check_chunk_valid() to tree-checker.c and export it.
> 
> And since it's now moved to tree-checker, also add a better comment for
> what this function is doing.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c | 99 +++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/tree-checker.h |  3 ++
>  fs/btrfs/volumes.c      | 94 +-------------------------------------
>  3 files changed, 103 insertions(+), 93 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index b8cdaf472031..4e44323ae758 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -448,6 +448,105 @@ static int check_block_group_item(struct btrfs_fs_info *fs_info,
>  	return 0;
>  }
>  
> +/*
> + * The common chunk check which could also work on super block sys chunk array.
> + *
> + * Return -EUCLEAN if anything is corrupted.

Well, that's still confusing if you say EUCLEAN in the commend and use
EIO in the code.

Re: [PATCh v2 1/9] btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it

[Qu Wenruo]

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



On 2019/3/26 上午1:06, David Sterba wrote:
> On Wed, Mar 20, 2019 at 02:37:09PM +0800, Qu Wenruo wrote:
>> By function, chunk item verification is more suitable to be done inside
>> tree-checker.
>>
>> So move btrfs_check_chunk_valid() to tree-checker.c and export it.
>>
>> And since it's now moved to tree-checker, also add a better comment for
>> what this function is doing.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/tree-checker.c | 99 +++++++++++++++++++++++++++++++++++++++++
>>  fs/btrfs/tree-checker.h |  3 ++
>>  fs/btrfs/volumes.c      | 94 +-------------------------------------
>>  3 files changed, 103 insertions(+), 93 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index b8cdaf472031..4e44323ae758 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -448,6 +448,105 @@ static int check_block_group_item(struct btrfs_fs_info *fs_info,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * The common chunk check which could also work on super block sys chunk array.
>> + *
>> + * Return -EUCLEAN if anything is corrupted.
> 
> Well, that's still confusing if you say EUCLEAN in the commend and use
> EIO in the code.
> 
Oh, that EIO to EUCLEAN change is in later patch (3/9).

Do I need to resend the patchset?

Thanks,
Qu


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

Re: [PATCh v2 1/9] btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it

[David Sterba]

On Tue, Mar 26, 2019 at 07:02:20AM +0800, Qu Wenruo wrote:
> On 2019/3/26 上午1:06, David Sterba wrote:
> > On Wed, Mar 20, 2019 at 02:37:09PM +0800, Qu Wenruo wrote:
> >> By function, chunk item verification is more suitable to be done inside
> >> tree-checker.
> >>
> >> So move btrfs_check_chunk_valid() to tree-checker.c and export it.
> >>
> >> And since it's now moved to tree-checker, also add a better comment for
> >> what this function is doing.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  fs/btrfs/tree-checker.c | 99 +++++++++++++++++++++++++++++++++++++++++
> >>  fs/btrfs/tree-checker.h |  3 ++
> >>  fs/btrfs/volumes.c      | 94 +-------------------------------------
> >>  3 files changed, 103 insertions(+), 93 deletions(-)
> >>
> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> >> index b8cdaf472031..4e44323ae758 100644
> >> --- a/fs/btrfs/tree-checker.c
> >> +++ b/fs/btrfs/tree-checker.c
> >> @@ -448,6 +448,105 @@ static int check_block_group_item(struct btrfs_fs_info *fs_info,
> >>  	return 0;
> >>  }
> >>  
> >> +/*
> >> + * The common chunk check which could also work on super block sys chunk array.
> >> + *
> >> + * Return -EUCLEAN if anything is corrupted.
> > 
> > Well, that's still confusing if you say EUCLEAN in the commend and use
> > EIO in the code.
> > 
> Oh, that EIO to EUCLEAN change is in later patch (3/9).

Yes, but this patch when viewed on itself is confusing. The EIO->EUCLEAN
in the comment belongs to 3/9 too.

> Do I need to resend the patchset?

No, such small fixups I do myself but I need to point that out so we
reach a common understanding and what's expected.

Re: [PATCh v2 2/9] btrfs: tree-checker: Make chunk item checker more readable

[David Sterba]

On Wed, Mar 20, 2019 at 11:41:44AM +0100, Johannes Thumshirn wrote:
> Looks good,
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> 
> Although I think it would've been worth to explicitly mention that you
> increased the severity level from error to critical.

Agreed, changelog updated.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[David Sterba]

On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
> @@ -1539,6 +1539,8 @@ do {                                                                   \
>  #define BTRFS_INODE_COMPRESS		(1 << 11)
>  
>  #define BTRFS_INODE_ROOT_ITEM_INIT	(1 << 31)
> +#define BTRFS_INODE_FLAG_MASK		(((1 << 12) - 1) |\
> +					 BTRFS_INODE_ROOT_ITEM_INIT)

That's fragile, the mask constant should enumerate all bits it's
supposed to cover, like

+#define BTRFS_INODE_FLAG_MASK                                          \
+       (BTRFS_INODE_NODATASUM |                                        \
+        BTRFS_INODE_NODATACOW |                                        \
+        BTRFS_INODE_READONLY |                                         \
+        BTRFS_INODE_NOCOMPRESS |                                       \
+        BTRFS_INODE_PREALLOC |                                         \
+        BTRFS_INODE_SYNC |                                             \
+        BTRFS_INODE_IMMUTABLE |                                        \
+        BTRFS_INODE_APPEND |                                           \
+        BTRFS_INODE_NODUMP |                                           \
+        BTRFS_INODE_NOATIME |                                          \
+        BTRFS_INODE_DIRSYNC |                                          \
+        BTRFS_INODE_COMPRESS |                                         \
+        BTRFS_INODE_ROOT_ITEM_INIT)
+

> +	u64 super_gen = btrfs_super_generation(fs_info->super_copy);
> +	u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777);
> +	u32 mode;
> +
> +	if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID ||
> +	     key->objectid > BTRFS_LAST_FREE_OBJECTID) &&
> +	     key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
> +	     key->objectid != BTRFS_FREE_INO_OBJECTID) {
> +		generic_err(fs_info, leaf, slot,
> +	"invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu",
> +			    key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID,
> +			    BTRFS_FIRST_FREE_OBJECTID,
> +			    BTRFS_LAST_FREE_OBJECTID,
> +			    BTRFS_FREE_INO_OBJECTID);
> +		goto error;
> +	}
> +	if (key->offset != 0) {
> +		inode_item_err(fs_info, leaf, slot,
> +			"invalid key offset: has %llu expect 0",
> +			key->offset);
> +		goto error;
> +	}
> +	iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item);
> +
> +	/* Here we use super block generation + 1 to handle log tree */
> +	if (btrfs_inode_generation(leaf, iitem) > super_gen + 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			"invalid inode generation: has %llu expect (0, %llu]",
> +			       btrfs_inode_generation(leaf, iitem),
> +			       super_gen + 1);
> +		goto error;
> +	}
> +	/* Note for ROOT_TREE_DIR_ITEM, mkfs could make its transid as 0 */
> +	if (btrfs_inode_transid(leaf, iitem) > super_gen + 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			"invalid inode generation: has %llu expect [0, %llu]",
> +			       btrfs_inode_transid(leaf, iitem),
> +			       super_gen + 1);
> +		goto error;
> +	}
> +
> +	/*
> +	 * For size and nbytes it's better not to be too strict, as for dir
> +	 * item its size/nbytes can easily get wrong, but doesn't affect
> +	 * any thing of the fs. So here we skip the check.
> +	 */
> +
> +	mode = btrfs_inode_mode(leaf, iitem);
> +	if (mode & ~valid_mask) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "unknown mode bit detected: 0x%x",
> +			       mode & ~valid_mask);
> +		goto error;
> +	}
> +
> +	/*
> +	 * S_IFMT is not bit mapped so we can't completely rely is_power_of_2(),
> +	 * but is_power_of_2() can save us from checking FIFO/CHR/DIR/REG.
> +	 * Only needs to check BLK, LNK and SOCKS
> +	 */
> +	if (!is_power_of_2(mode & S_IFMT)) {
> +		if (!S_ISLNK(mode) && ! S_ISBLK(mode) && !S_ISSOCK(mode)) {
> +			inode_item_err(fs_info, leaf, slot,
> +			"invalid mode: has 0%o expect valid S_IF* bit(s)",
> +				       mode & S_IFMT);
> +			goto error;
> +		}
> +	}
> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "invalid nlink: has %u expect no more than 1 for dir",
> +			btrfs_inode_nlink(leaf, iitem));
> +		goto error;
> +	}
> +	if (btrfs_inode_flags(leaf, iitem) & ~BTRFS_INODE_FLAG_MASK) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "unknown flags detected: 0x%llx",
> +			       btrfs_inode_flags(leaf, iitem) &
> +			       ~BTRFS_INODE_FLAG_MASK);
> +		goto error;
> +	}
> +	return 0;
> +
> +error:
> +	return -EUCLEAN;

Switched to local returns instead of gotos.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[David Sterba]

On Mon, Mar 25, 2019 at 12:27:24PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/3/20 下午2:37, Qu Wenruo wrote:
> > There is a report in kernel bugzilla about mismatch file type in dir
> > item and inode item.
> >
> > This inspires us to check inode mode in inode item.
> >
> > This patch will check the following members:
> > - inode key objectid
> >   Should be ROOT_DIR_DIR or [256, (u64)-256] or FREE_INO.
> >
> > - inode key offset
> >   Should be 0
> >
> > - inode item generation
> > - inode item transid
> >   No newer than sb generation + 1.
> >   The +1 is for log tree.
> >
> > - inode item mode
> >   No unknown bits.
> >   No invalid S_IF* bit.
> >   NOTE: S_IFMT check is not enough, need to check every know type.
> >
> > - inode item nlink
> >   Dir should have no more link than 1.
> >
> > - inode item flags
> >
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> There is some bug report of kernel producing free space cache inode with
> mode 0, which is invalid and can be detected by this patch.
> 
> Although the patch itself is good, I'm afraid we need to address the
> invalid inode mode created by old kernel in btrfs-progs at least before
> merging this patch into upstream.

Can this be addressed on the kernel side? Like detecting the invalid
mode, print a warning and the fix on the next write. The progs can
detect and fix that too of course.

So I'll keep the patch working as-is, we can relax the error to a
warning if we're out of time or find out that it needs to be that way
due to backward compatibilit reasons.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

On 2019/3/27 上午12:02, David Sterba wrote:
> On Mon, Mar 25, 2019 at 12:27:24PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/3/20 下午2:37, Qu Wenruo wrote:
>>> There is a report in kernel bugzilla about mismatch file type in dir
>>> item and inode item.
>>>
>>> This inspires us to check inode mode in inode item.
>>>
>>> This patch will check the following members:
>>> - inode key objectid
>>>   Should be ROOT_DIR_DIR or [256, (u64)-256] or FREE_INO.
>>>
>>> - inode key offset
>>>   Should be 0
>>>
>>> - inode item generation
>>> - inode item transid
>>>   No newer than sb generation + 1.
>>>   The +1 is for log tree.
>>>
>>> - inode item mode
>>>   No unknown bits.
>>>   No invalid S_IF* bit.
>>>   NOTE: S_IFMT check is not enough, need to check every know type.
>>>
>>> - inode item nlink
>>>   Dir should have no more link than 1.
>>>
>>> - inode item flags
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>> There is some bug report of kernel producing free space cache inode with
>> mode 0, which is invalid and can be detected by this patch.
>>
>> Although the patch itself is good, I'm afraid we need to address the
>> invalid inode mode created by old kernel in btrfs-progs at least before
>> merging this patch into upstream.
> 
> Can this be addressed on the kernel side? Like detecting the invalid
> mode, print a warning and the fix on the next write. The progs can
> detect and fix that too of course.

So far even on older fs images (like those in btrfs-progs fsck tests), I
noticed no such invalid free space inode at all.

And from the history of that code, the mode is fixed to 100600 since 2010.
Currently I believe it's uncommon to see that case.

Furthermore, the btrfs-progs fix for such case is already submitted, as
long as we have a minor release to include that fix, it should be OK.

> 
> So I'll keep the patch working as-is, we can relax the error to a
> warning if we're out of time or find out that it needs to be that way
> due to backward compatibilit reasons.
> 

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[David Sterba]

On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
> +		inode_item_err(fs_info, leaf, slot,
> +			       "invalid nlink: has %u expect no more than 1 for dir",
> +			btrfs_inode_nlink(leaf, iitem));
> +		goto error;
> +	}

I'm not sure about this check, the number of links for a directory is 1,
but the exact count could be implemented (there's a project idea for
that). I don't know if this will require an incompat bit or not.

As long as the number is not authoritative (ie. can be verified and
fixed from the actual directory items), then I'd say don't check it at
all, or perhaps do only weak check if it's > 0.

Out of all the verified inode data, this is the least important, I think
we're not losing some significant piece of information.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

On 2019/3/28 下午9:38, David Sterba wrote:
> On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
>> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
>> +		inode_item_err(fs_info, leaf, slot,
>> +			       "invalid nlink: has %u expect no more than 1 for dir",
>> +			btrfs_inode_nlink(leaf, iitem));
>> +		goto error;
>> +	}
> 
> I'm not sure about this check, the number of links for a directory is 1,
> but the exact count could be implemented (there's a project idea for
> that). I don't know if this will require an incompat bit or not.

That means we have hard link for directories.

That's definitely something current VFS can't handle, things like path
resolve loop is definitely going to happen.

> 
> As long as the number is not authoritative (ie. can be verified and
> fixed from the actual directory items), then I'd say don't check it at
> all, or perhaps do only weak check if it's > 0.

Current the number is authoritative already.
It means the number of hard links, both for directory and regular
inodes, also indicates the number of INODE_REF.

Btrfs check will verify that.

> 
> Out of all the verified inode data, this is the least important, I think
> we're not losing some significant piece of information.

That's true, nlinks is not that important, but just as always,
tree-checker is checking every meaningful member and try to catch
anything uncommon.
Although that restrict behavior has already caused a lot of false alerts
in the past, I still prefer to do such restrict check.

Thanks,
Qu

Re: [PATCh v2 9/9] btrfs: inode: Verify inode mode to avoid NULL pointer dereference

[David Sterba]

On Wed, Mar 20, 2019 at 02:37:17PM +0800, Qu Wenruo wrote:
> @@ -5726,18 +5734,29 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
>  	struct btrfs_root *root = BTRFS_I(dir)->root;
>  	struct btrfs_root *sub_root = root;
>  	struct btrfs_key location;
> +	u8 di_type = 0;
>  	int index;
>  	int ret = 0;
>  
>  	if (dentry->d_name.len > BTRFS_NAME_LEN)
>  		return ERR_PTR(-ENAMETOOLONG);
>  
> -	ret = btrfs_inode_by_name(dir, dentry, &location);
> +	ret = btrfs_inode_by_name(dir, dentry, &location, &di_type);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
>  
>  	if (location.type == BTRFS_INODE_ITEM_KEY) {
>  		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
> +
> +		/* Do extra check against inode mode with di_type */
> +		if (btrfs_inode_type(inode) != di_type) {
> +			btrfs_crit(fs_info,
> +"inode mode mismatch with dir: inode mode=0%o btrfs type=%u dir type=%u",
> +				  inode->i_mode, btrfs_inode_type(inode),
> +				  di_type);
> +			iput(inode);

The iput here seems suspicious.

> +			return ERR_PTR(-EUCLEAN);
> +		}
>  		return inode;
>  	}

5792         index = srcu_read_lock(&fs_info->subvol_srcu);
5793         ret = fixup_tree_root_location(fs_info, dir, dentry,
5794                                        &location, &sub_root);
5795         if (ret < 0) {
5796                 if (ret != -ENOENT)
5797                         inode = ERR_PTR(ret);
5798                 else
5799                         inode = new_simple_dir(dir->i_sb, &location, sub_root);
5800         } else {
5801                 inode = btrfs_iget(dir->i_sb, &location, sub_root, NULL);
5802         }

The iget happens after this block so either there's another one in the caller
to pair, or the above iput is wrong.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[David Sterba]

On Thu, Mar 28, 2019 at 09:42:59PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/3/28 下午9:38, David Sterba wrote:
> > On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
> >> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
> >> +		inode_item_err(fs_info, leaf, slot,
> >> +			       "invalid nlink: has %u expect no more than 1 for dir",
> >> +			btrfs_inode_nlink(leaf, iitem));
> >> +		goto error;
> >> +	}
> > 
> > I'm not sure about this check, the number of links for a directory is 1,
> > but the exact count could be implemented (there's a project idea for
> > that). I don't know if this will require an incompat bit or not.
> 
> That means we have hard link for directories.

Yes, hard links of directories are forbidden by VFS but that's not the
point here:

https://btrfs.wiki.kernel.org/index.php/Project_ideas#Track_link_count_for_directories

"The link count for directories is traditionally used to count the number
of subdirectores iff the link count is >= 2."

Re: [PATCh v2 9/9] btrfs: inode: Verify inode mode to avoid NULL pointer dereference

[Qu Wenruo]

On 2019/3/28 下午9:53, David Sterba wrote:
> On Wed, Mar 20, 2019 at 02:37:17PM +0800, Qu Wenruo wrote:
>> @@ -5726,18 +5734,29 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
>>  	struct btrfs_root *root = BTRFS_I(dir)->root;
>>  	struct btrfs_root *sub_root = root;
>>  	struct btrfs_key location;
>> +	u8 di_type = 0;
>>  	int index;
>>  	int ret = 0;
>>  
>>  	if (dentry->d_name.len > BTRFS_NAME_LEN)
>>  		return ERR_PTR(-ENAMETOOLONG);
>>  
>> -	ret = btrfs_inode_by_name(dir, dentry, &location);
>> +	ret = btrfs_inode_by_name(dir, dentry, &location, &di_type);
>>  	if (ret < 0)
>>  		return ERR_PTR(ret);
>>  
>>  	if (location.type == BTRFS_INODE_ITEM_KEY) {
>>  		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
>> +
>> +		/* Do extra check against inode mode with di_type */
>> +		if (btrfs_inode_type(inode) != di_type) {
>> +			btrfs_crit(fs_info,
>> +"inode mode mismatch with dir: inode mode=0%o btrfs type=%u dir type=%u",
>> +				  inode->i_mode, btrfs_inode_type(inode),
>> +				  di_type);
>> +			iput(inode);
> 
> The iput here seems suspicious.

How? this pairs to that inode = btrfs_iget() line.

And as long as we hit this branch, we will return either a valid inode
or an error pointer.
> 
>> +			return ERR_PTR(-EUCLEAN);
>> +		}
>>  		return inode;
>>  	}
> 
> 5792         index = srcu_read_lock(&fs_info->subvol_srcu);
> 5793         ret = fixup_tree_root_location(fs_info, dir, dentry,
> 5794                                        &location, &sub_root);
> 5795         if (ret < 0) {
> 5796                 if (ret != -ENOENT)
> 5797                         inode = ERR_PTR(ret);
> 5798                 else
> 5799                         inode = new_simple_dir(dir->i_sb, &location, sub_root);
> 5800         } else {
> 5801                 inode = btrfs_iget(dir->i_sb, &location, sub_root, NULL);
> 5802         }
> 
> The iget happens after this block so either there's another one in the caller
> to pair, or the above iput is wrong.

This block is unrelated to above (location.type == BTRFS_INODE_ITEM_KEY)
branch.

Or did I miss something?

Thanks,
Qu

> 

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

On 2019/3/28 下午9:57, David Sterba wrote:
> On Thu, Mar 28, 2019 at 09:42:59PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/3/28 下午9:38, David Sterba wrote:
>>> On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
>>>> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
>>>> +		inode_item_err(fs_info, leaf, slot,
>>>> +			       "invalid nlink: has %u expect no more than 1 for dir",
>>>> +			btrfs_inode_nlink(leaf, iitem));
>>>> +		goto error;
>>>> +	}
>>>
>>> I'm not sure about this check, the number of links for a directory is 1,
>>> but the exact count could be implemented (there's a project idea for
>>> that). I don't know if this will require an incompat bit or not.
>>
>> That means we have hard link for directories.
> 
> Yes, hard links of directories are forbidden by VFS but that's not the
> point here:
> 
> https://btrfs.wiki.kernel.org/index.php/Project_ideas#Track_link_count_for_directories
> 
> "The link count for directories is traditionally used to count the number
> of subdirectores iff the link count is >= 2."

Oh, got it.

But for that case, it would be much better to go for things like nbytes
or size, as by all means, those two members are less meaningful than
nlinks for directory.

Thanks,
Qu

> 

Re: [PATCh v2 9/9] btrfs: inode: Verify inode mode to avoid NULL pointer dereference

[David Sterba]

On Thu, Mar 28, 2019 at 09:58:26PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/3/28 下午9:53, David Sterba wrote:
> > On Wed, Mar 20, 2019 at 02:37:17PM +0800, Qu Wenruo wrote:
> >> @@ -5726,18 +5734,29 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
> >>  	struct btrfs_root *root = BTRFS_I(dir)->root;
> >>  	struct btrfs_root *sub_root = root;
> >>  	struct btrfs_key location;
> >> +	u8 di_type = 0;
> >>  	int index;
> >>  	int ret = 0;
> >>  
> >>  	if (dentry->d_name.len > BTRFS_NAME_LEN)
> >>  		return ERR_PTR(-ENAMETOOLONG);
> >>  
> >> -	ret = btrfs_inode_by_name(dir, dentry, &location);
> >> +	ret = btrfs_inode_by_name(dir, dentry, &location, &di_type);
> >>  	if (ret < 0)
> >>  		return ERR_PTR(ret);
> >>  
> >>  	if (location.type == BTRFS_INODE_ITEM_KEY) {
> >>  		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
> >> +
> >> +		/* Do extra check against inode mode with di_type */
> >> +		if (btrfs_inode_type(inode) != di_type) {
> >> +			btrfs_crit(fs_info,
> >> +"inode mode mismatch with dir: inode mode=0%o btrfs type=%u dir type=%u",
> >> +				  inode->i_mode, btrfs_inode_type(inode),
> >> +				  di_type);
> >> +			iput(inode);
> > 
> > The iput here seems suspicious.
> 
> How? this pairs to that inode = btrfs_iget() line.

Right, that's what I overlooked, thanks.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[David Sterba]

On Thu, Mar 28, 2019 at 10:00:22PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/3/28 下午9:57, David Sterba wrote:
> > On Thu, Mar 28, 2019 at 09:42:59PM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2019/3/28 下午9:38, David Sterba wrote:
> >>> On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
> >>>> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
> >>>> +		inode_item_err(fs_info, leaf, slot,
> >>>> +			       "invalid nlink: has %u expect no more than 1 for dir",
> >>>> +			btrfs_inode_nlink(leaf, iitem));
> >>>> +		goto error;
> >>>> +	}
> >>>
> >>> I'm not sure about this check, the number of links for a directory is 1,
> >>> but the exact count could be implemented (there's a project idea for
> >>> that). I don't know if this will require an incompat bit or not.
> >>
> >> That means we have hard link for directories.
> > 
> > Yes, hard links of directories are forbidden by VFS but that's not the
> > point here:
> > 
> > https://btrfs.wiki.kernel.org/index.php/Project_ideas#Track_link_count_for_directories
> > 
> > "The link count for directories is traditionally used to count the number
> > of subdirectores iff the link count is >= 2."
> 
> Oh, got it.
> 
> But for that case, it would be much better to go for things like nbytes
> or size, as by all means, those two members are less meaningful than
> nlinks for directory.

size of a directory is currently 2 * sum of all names in the directory,
ie. both files and directories. The nlink is used as an optimization
during traversal by existing tools (find), we can't simply change that
but would still like to update btrfs to provide support for that.

On the wiki there's note that it's backward compatible, I don't recall
the actual details why it's safe though.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

On 2019/3/28 下午10:07, David Sterba wrote:
> On Thu, Mar 28, 2019 at 10:00:22PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/3/28 下午9:57, David Sterba wrote:
>>> On Thu, Mar 28, 2019 at 09:42:59PM +0800, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2019/3/28 下午9:38, David Sterba wrote:
>>>>> On Wed, Mar 20, 2019 at 02:37:16PM +0800, Qu Wenruo wrote:
>>>>>> +	if (S_ISDIR(mode) && btrfs_inode_nlink(leaf, iitem) > 1) {
>>>>>> +		inode_item_err(fs_info, leaf, slot,
>>>>>> +			       "invalid nlink: has %u expect no more than 1 for dir",
>>>>>> +			btrfs_inode_nlink(leaf, iitem));
>>>>>> +		goto error;
>>>>>> +	}
>>>>>
>>>>> I'm not sure about this check, the number of links for a directory is 1,
>>>>> but the exact count could be implemented (there's a project idea for
>>>>> that). I don't know if this will require an incompat bit or not.
>>>>
>>>> That means we have hard link for directories.
>>>
>>> Yes, hard links of directories are forbidden by VFS but that's not the
>>> point here:
>>>
>>> https://btrfs.wiki.kernel.org/index.php/Project_ideas#Track_link_count_for_directories
>>>
>>> "The link count for directories is traditionally used to count the number
>>> of subdirectores iff the link count is >= 2."
>>
>> Oh, got it.
>>
>> But for that case, it would be much better to go for things like nbytes
>> or size, as by all means, those two members are less meaningful than
>> nlinks for directory.
>
> size of a directory is currently 2 * sum of all names in the directory,
> ie. both files and directories.

In fact that's not always the case for older kernels/progs.

IIRC it's in recent years that we enhanced btrfs-progs to detect and fix
that.


> The nlink is used as an optimization
> during traversal by existing tools (find), we can't simply change that
> but would still like to update btrfs to provide support for that.

But current nlinks is persistent against all inodes. It's always the the
number of INODE_REF the inode has.

While for size/nbytes, it doesn't make anything for directory inode at all.
Kernel doesn't care, it's mostly btrfs-check check and fix.

Furthermore, only size is fixed to 2 * num_children.
nbytes is only instructive and at least in lowmem mode, nbytes is only
checked for alignment, even it's unaligned, lowmem check outputs warning
only, doesn't count as an error.

So at least to me, directory nbytes is a better alternative, and it's
back compatible.

Thanks,
Qu
>
> On the wiki there's note that it's backward compatible, I don't recall
> the actual details why it's safe though.
>

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[David Sterba]

On Thu, Mar 28, 2019 at 10:13:19PM +0800, Qu Wenruo wrote:
> >>>> That means we have hard link for directories.
> >>>
> >>> Yes, hard links of directories are forbidden by VFS but that's not the
> >>> point here:
> >>>
> >>> https://btrfs.wiki.kernel.org/index.php/Project_ideas#Track_link_count_for_directories
> >>>
> >>> "The link count for directories is traditionally used to count the number
> >>> of subdirectores iff the link count is >= 2."
> >>
> >> Oh, got it.
> >>
> >> But for that case, it would be much better to go for things like nbytes
> >> or size, as by all means, those two members are less meaningful than
> >> nlinks for directory.
> >
> > size of a directory is currently 2 * sum of all names in the directory,
> > ie. both files and directories.
> 
> In fact that's not always the case for older kernels/progs.
> 
> IIRC it's in recent years that we enhanced btrfs-progs to detect and fix
> that.
> 
> > The nlink is used as an optimization
> > during traversal by existing tools (find), we can't simply change that
> > but would still like to update btrfs to provide support for that.
> 
> But current nlinks is persistent against all inodes. It's always the the
> number of INODE_REF the inode has.
> 
> While for size/nbytes, it doesn't make anything for directory inode at all.
> Kernel doesn't care, it's mostly btrfs-check check and fix.
> 
> Furthermore, only size is fixed to 2 * num_children.
> nbytes is only instructive and at least in lowmem mode, nbytes is only
> checked for alignment, even it's unaligned, lowmem check outputs warning
> only, doesn't count as an error.
> 
> So at least to me, directory nbytes is a better alternative, and it's
> back compatible.

The point here is to use nlinks of directories for the directory
traversal, the size/nbytes is not relevant here for the reasons you
state above.

I don't see how you think the proposed nbytes alternative would be used,
eg. by find.

Re: [PATCh v2 0/9] btrfs: tree-checker: More enhancement for fuzzed

[David Sterba]

On Wed, Mar 20, 2019 at 02:37:08PM +0800, Qu Wenruo wrote:
> This patchset can be fetched from github:
> It can be fetched from github:
> https://github.com/adam900710/linux/tree/tree_checker_enhancement
> Which is based on my previous write time tree checker patchset (based on
> v5.1-rc1 tag)
> 
> Thanks for the report from Yoon Jungyeon <jungyeon@gatech.edu>, we have
> more fuzzed image to torture btrfs.
> 
> Those images exposed the following problems:
> 
> - Chunk check is not comprehensive nor early enough
>   Chunk item check lacks profile bits check (e.g RAID|DUP profile is
>   invalid).
>   And for certain fuzzed image, the other copy can be valid, current
>   check timming is after tree block read, so no way to retry the other
>   copy.
> 
>   Address the check timing in the 1st~4th patch, while for the profile bits,
>   check it in the 7th patch.
> 
> - Lack of device item check
>   Address it in the 5nd patch.
> 
> - First key and level check be exploited by cached extent buffer
>   Cached bad extent buffer can avoid first key and level check.
>   This is addressed in the 6rd patch.
> 
> - Inode type mismatch can lead to NULL dereference in endio function
>   If an inode claims itself as symlink but still has regular file
>   extent, then endio function will cause NULL pointer dereference.
>   Fix it by do extra inode mode and dir item type cross check, at
>   get_extent() time and inode lookup time.
>   Addressed in the last 2 patches.
> 
> Changelog:
> v2:
> - Split patches for btrfs_check_chunk_valid() merge into tree-checker.
> - Rebase to v5.1-rc1 based write_time_tree_checker branch.
> - Add reviewed-by tags.
> 
> Qu Wenruo (9):
>   btrfs: Move btrfs_check_chunk_valid() to tree-check.[ch] and export it
>   btrfs: tree-checker: Make chunk item checker more readable
>   btrfs: tree-checker: Make btrfs_check_chunk_valid() return EUCLEAN
>     instead of EIO
>   btrfs: tree-checker: Check chunk item at tree block read time
>   btrfs: tree-checker: Verify dev item
>   btrfs: Check the first key and level for cached extent buffer
>   btrfs: tree-checker: Enhance chunk checker to validate chunk profiler
>   btrfs: tree-checker: Verify inode item
>   btrfs: inode: Verify inode mode to avoid NULL pointer dereference

Patchset merged to misc-next, thanks. There were some fixups to comments
and changelogs. The inode item check part that checks nlink is
unchanged, it'll be removed eventually when we agree that on the reasoning
behind it.

Re: [PATCh v2 8/9] btrfs: tree-checker: Verify inode item

[Qu Wenruo]

On 2019/3/28 下午10:25, David Sterba wrote:
> On Thu, Mar 28, 2019 at 10:13:19PM +0800, Qu Wenruo wrote:
>>>>>> That means we have hard link for directories.
>>>>>
>>>>> Yes, hard links of directories are forbidden by VFS but that's not the
>>>>> point here:
>>>>>
>>>>> https://btrfs.wiki.kernel.org/index.php/Project_ideas#Track_link_count_for_directories
>>>>>
>>>>> "The link count for directories is traditionally used to count the number
>>>>> of subdirectores iff the link count is >= 2."
>>>>
>>>> Oh, got it.
>>>>
>>>> But for that case, it would be much better to go for things like nbytes
>>>> or size, as by all means, those two members are less meaningful than
>>>> nlinks for directory.
>>>
>>> size of a directory is currently 2 * sum of all names in the directory,
>>> ie. both files and directories.
>>
>> In fact that's not always the case for older kernels/progs.
>>
>> IIRC it's in recent years that we enhanced btrfs-progs to detect and fix
>> that.
>>
>>> The nlink is used as an optimization
>>> during traversal by existing tools (find), we can't simply change that
>>> but would still like to update btrfs to provide support for that.
>>
>> But current nlinks is persistent against all inodes. It's always the the
>> number of INODE_REF the inode has.
>>
>> While for size/nbytes, it doesn't make anything for directory inode at all.
>> Kernel doesn't care, it's mostly btrfs-check check and fix.
>>
>> Furthermore, only size is fixed to 2 * num_children.
>> nbytes is only instructive and at least in lowmem mode, nbytes is only
>> checked for alignment, even it's unaligned, lowmem check outputs warning
>> only, doesn't count as an error.
>>
>> So at least to me, directory nbytes is a better alternative, and it's
>> back compatible.
>
> The point here is to use nlinks of directories for the directory
> traversal, the size/nbytes is not relevant here for the reasons you
> state above.
>
> I don't see how you think the proposed nbytes alternative would be used,
> eg. by find.


In state() call or whatever syscall find is using, export nbytes instead
of nlink for dir.
Of course, this needs some compatibility check, e.g. nbytes should never
exceed size/2, or the nbytes must be old value (nodesize).

My point is, if we're going to give new meaning of an existing member,
at least choose a member that is unused and doesn't have persistent
meaning among all inodes, not to mention current btrfs check will
definitely complain about nlinks.

Thanks,
Qu

Re: [PATCh v2 5/9] btrfs: tree-checker: Verify dev item

[Qu Wenruo]

On 2019/3/20 下午2:37, Qu Wenruo wrote:
[snip]
> +
> +	/*
> +	 * 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;
> +	}


Hi David,

Please remove this patch from misc-next queue.

Under the following call trace, we could create device with total_bytes
== 0;

btrfs_rm_device()
|- btrfs_shrink_device()
|  |- btrfs_device_set_total_bytes(device, 0)
|  |- btrfs_update_device()
|  |- btrfs_commit_transaction() #1
|- btrfs_rm_dev_item()

This will trigger write time tree checker warning.
And further more, this can create valid btrfs with device->total_bytes
== 0 and triggering read time tree-checker if power loss happens after
above transaction #1 but before next transaction.

So this dev item check is too restrict.

And furthermore, the error output is misleading, its devid is extracted
from key->objectid, but it should be key->offset.

For the fuzzed image, I'd like to fix it by either enhancing the seed
device lookup procedure.

Thanks,
Qu

> +	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.
>   */
> @@ -632,6 +712,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 645ffc9c47b0..7510272408e8 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4958,15 +4958,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 3ad9d58d1b66..38ed94b77202 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).
>