[PATCH v2 0/3] tree-checker bug fix and enhancement.

[PATCH v2 0/3] tree-checker bug fix and enhancement.

[Qu Wenruo]

This patchset fix a false panic introduced by tree-checker,
and then introduce dir_item checker, along with cleanups to remove
existing checkers sparsed in dir-item.

The 1st patch is a fix to address kernel panic with sanity test in btrfs.
The cause is there are several callers which call
btrfs_mark_buffer_dirty() while its data is not initialized.
(The same patch is resent along with the rest of the patchset)

Considering the number of callers and how many times btrfs_mark_buffer_dirty()
is called, the 1st patch will skip item data check, so check in
btrfs_mark_buffer_dirty() will keep the old behavior.
(Only item and item pointer overlap check is newly introduced in this case)

The 2nd patch introduce comprehensive check for dir item, which is used
for 3 key types: DIR_ITEM, DIR_INDEX and XATTR.
(Unlike previous attempts to check all items with variable length)

Except existing checks, new checks are:
1) Enhanced dir type check
   Now only XATTR key can have FT_ATTR dir item.

2) name hash for DIR_ITEM/XATTR_ITEM
   Original introduced in btrfs-progs for a corrupted (non-fuzzed)
   image.

The last patch will cleanup the related existing code, mostly related to
verify_dir_item() and btrfs_is_name_len_valid().

Unlike btrfs_check_leaf() called in btrfs_mark_buffer_dirty(), all the
verification calls are in read path, so we won't lost any early warning
since they didn't exist from the beginning.

Changelog:
v2:
  Rename btrfs_check_leaf() to btrfs_check_leaf_full() and
  btrfs_check_leaf_relaxed() to better show their difference. Suggested
  by David.

  Unintend message in 2nd patch.

Qu Wenruo (3):
  btrfs: tree-checker: Fix false panic for sanity test
  btrfs: tree-checker: Add checker for dir item
  btrfs: Cleanup existing name_len checks

 fs/btrfs/ctree.h        |   5 --
 fs/btrfs/dir-item.c     | 108 -------------------------------
 fs/btrfs/disk-io.c      |  10 ++-
 fs/btrfs/export.c       |   5 --
 fs/btrfs/inode.c        |   4 --
 fs/btrfs/props.c        |   7 --
 fs/btrfs/root-tree.c    |   7 --
 fs/btrfs/send.c         |   6 --
 fs/btrfs/tree-checker.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++--
 fs/btrfs/tree-checker.h |  14 +++-
 fs/btrfs/tree-log.c     |  47 +++-----------
 fs/btrfs/xattr.c        |   6 --
 12 files changed, 193 insertions(+), 194 deletions(-)

-- 
2.15.0

[PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test

[Qu Wenruo]

[BUG]
If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will
instantly cause kernel panic like:

------
...
assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853
...
Call Trace:
 btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs]
 setup_items_for_insert+0x385/0x650 [btrfs]
 __btrfs_drop_extents+0x129a/0x1870 [btrfs]
...
-----

[Cause]
Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check
if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y.

However quite some btrfs_mark_buffer_dirty() callers(*) don't really
initialize its item data but only initialize its item pointers, leaving
item data uninitialized.

This makes tree-checker catch uninitialized data as error, causing
such panic.

*: These callers include but not limited to
setup_items_for_insert()
btrfs_split_item()
btrfs_expand_item()

[Fix]
Add a new parameter @check_item_data to btrfs_check_leaf().
With @check_item_data set to false, item data check will be skipped and
fallback to old btrfs_check_leaf() behavior.

So we can still get early warning if we screw up item pointers, and
avoid false panic.

Cc: Filipe Manana <fdmanana@gmail.com>
Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c      | 10 ++++++++--
 fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++-----
 fs/btrfs/tree-checker.h | 14 +++++++++++++-
 3 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index efce9a2fa9be..10a2a579cc7f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 	 * that we don't try and read the other copies of this block, just
 	 * return -EIO.
 	 */
-	if (found_level == 0 && btrfs_check_leaf(root, eb)) {
+	if (found_level == 0 && btrfs_check_leaf_full(root, eb)) {
 		set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
 		ret = -EIO;
 	}
@@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 					 buf->len,
 					 fs_info->dirty_metadata_batch);
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
+	/*
+	 * Since btrfs_mark_buffer_dirty() can be called with item pointer set
+	 * but item data not updated.
+	 * So here we should only check item pointers, not item data.
+	 */
+	if (btrfs_header_level(buf) == 0 &&
+	    btrfs_check_leaf_relaxed(root, buf)) {
 		btrfs_print_leaf(buf);
 		ASSERT(0);
 	}
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 114fc5f0ecc5..ce4ed6ec8f39 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root,
 	return ret;
 }
 
-int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
+static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf,
+		      bool check_item_data)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	/* No valid key type is 0, so all key should be larger than this key */
@@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
 			return -EUCLEAN;
 		}
 
-		/* Check if the item size and content meet other criteria */
-		ret = check_leaf_item(root, leaf, &key, slot);
-		if (ret < 0)
-			return ret;
+		if (check_item_data) {
+			/*
+			 * Check if the item size and content meet other
+			 * criteria
+			 */
+			ret = check_leaf_item(root, leaf, &key, slot);
+			if (ret < 0)
+				return ret;
+		}
 
 		prev_key.objectid = key.objectid;
 		prev_key.type = key.type;
@@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
 	return 0;
 }
 
+int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf)
+{
+	return check_leaf(root, leaf, true);
+}
+
+int btrfs_check_leaf_relaxed(struct btrfs_root *root,
+			     struct extent_buffer *leaf)
+{
+	return check_leaf(root, leaf, false);
+}
+
 int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node)
 {
 	unsigned long nr = btrfs_header_nritems(node);
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 96c486e95d70..3d53e8d6fda0 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -20,7 +20,19 @@
 #include "ctree.h"
 #include "extent_io.h"
 
-int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf);
+/*
+ * Comprehensive leaf checker.
+ * Will check not only the item pointers, but also every possible member
+ * in item data.
+ */
+int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf);
+
+/*
+ * Less strict leaf checker.
+ * Will only check item pointers, not reading item data.
+ */
+int btrfs_check_leaf_relaxed(struct btrfs_root *root,
+			     struct extent_buffer *leaf);
 int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node);
 
 #endif
-- 
2.15.0

[PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item

[Qu Wenruo]

Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and
XATTR_ITEM.

This checker does comprehensive check for:
1) dir_item header and its data size
   Against item boundary and maximum name/xattr length.
   This part is mostly the same as old verify_dir_item().

2) dir_type
   Against maximum file types, and against key type.
   Since XATTR key should only have FT_XATTR dir item, and normal dir
   item type should not have XATTR key.

   The check between key->type and dir_type is newly introduced by this
   patch.

3) name hash
   For XATTR and DIR_ITEM key, key->offset is name hash (crc32).
   Check the hash of name against key to ensure it's correct.

   The name hash check is only found in btrfs-progs before this patch.

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

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index ce4ed6ec8f39..66dac0a4b01f 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -30,6 +30,7 @@
 #include "tree-checker.h"
 #include "disk-io.h"
 #include "compression.h"
+#include "hash.h"
 
 /*
  * Error message should follow the following format:
@@ -222,6 +223,141 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,
 	return 0;
 }
 
+/*
+ * Customized reported for dir_item, only important new info is key->objectid,
+ * which represents inode number
+ */
+__printf(4, 5)
+static void dir_item_err(const struct btrfs_root *root,
+			 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(root->fs_info,
+	"corrupt %s: root=%llu block=%llu slot=%d ino=%llu, %pV",
+		btrfs_header_level(eb) == 0 ? "leaf" : "node", root->objectid,
+		btrfs_header_bytenr(eb), slot, key.objectid, &vaf);
+	va_end(args);
+}
+
+static int check_dir_item(struct btrfs_root *root,
+			  struct extent_buffer *leaf,
+			  struct btrfs_key *key, int slot)
+{
+	struct btrfs_dir_item *di;
+	u32 item_size = btrfs_item_size_nr(leaf, slot);
+	u32 cur = 0;
+
+	di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
+	while (cur < item_size) {
+		char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
+		u32 name_len;
+		u32 data_len;
+		u32 max_name_len;
+		u32 total_size;
+		u32 name_hash;
+		u8 dir_type;
+
+		/* header itself should not cross item boundary */
+		if (cur + sizeof(*di) > item_size) {
+			dir_item_err(root, leaf, slot,
+		"dir item header crosses item boundary, have %lu boundary %u",
+				cur + sizeof(*di), item_size);
+			return -EUCLEAN;
+		}
+
+		/* dir type check */
+		dir_type = btrfs_dir_type(leaf, di);
+		if (dir_type >= BTRFS_FT_MAX) {
+			dir_item_err(root, leaf, slot,
+			"invalid dir item type, have %u expect [0, %u)",
+				dir_type, BTRFS_FT_MAX);
+			return -EUCLEAN;
+		}
+
+		if (key->type == BTRFS_XATTR_ITEM_KEY &&
+		    dir_type != BTRFS_FT_XATTR) {
+			dir_item_err(root, leaf, slot,
+		"invalid dir item type for XATTR key, have %u expect %u",
+				dir_type, BTRFS_FT_XATTR);
+			return -EUCLEAN;
+		}
+		if (dir_type == BTRFS_FT_XATTR &&
+		    key->type != BTRFS_XATTR_ITEM_KEY) {
+			dir_item_err(root, leaf, slot,
+			"xattr dir type found for non-XATTR key");
+			return -EUCLEAN;
+		}
+		if (dir_type == BTRFS_FT_XATTR)
+			max_name_len = XATTR_NAME_MAX;
+		else
+			max_name_len = BTRFS_NAME_LEN;
+
+		/* Name/data length check */
+		name_len = btrfs_dir_name_len(leaf, di);
+		data_len = btrfs_dir_data_len(leaf, di);
+		if (name_len > max_name_len) {
+			dir_item_err(root, leaf, slot,
+			"dir item name len too long, have %u max %u",
+				name_len, max_name_len);
+			return -EUCLEAN;
+		}
+		if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info)) {
+			dir_item_err(root, leaf, slot,
+			"dir item name and data len too long, have %u max %u",
+				name_len + data_len,
+				BTRFS_MAX_XATTR_SIZE(root->fs_info));
+			return -EUCLEAN;
+		}
+
+		if (data_len && dir_type != BTRFS_FT_XATTR) {
+			dir_item_err(root, leaf, slot,
+			"dir item with invalid data len, have %u expect 0",
+				data_len);
+			return -EUCLEAN;
+		}
+
+		total_size = sizeof(*di) + name_len + data_len;
+
+		/* header and name/data should not cross item boundary */
+		if (cur + total_size > item_size) {
+			dir_item_err(root, leaf, slot,
+		"dir item data crosses item boundary, have %u boundary %u",
+				cur + total_size, item_size);
+			return -EUCLEAN;
+		}
+
+		/*
+		 * Special check for XATTR/DIR_ITEM, as key->offset is name
+		 * hash, should match its name
+		 */
+		if (key->type == BTRFS_DIR_ITEM_KEY ||
+		    key->type == BTRFS_XATTR_ITEM_KEY) {
+			read_extent_buffer(leaf, namebuf,
+					(unsigned long)(di + 1), name_len);
+			name_hash = btrfs_name_hash(namebuf, name_len);
+			if (key->offset != name_hash) {
+				dir_item_err(root, leaf, slot,
+		"name hash mismatch with key, have 0x%016x expect 0x%016llx",
+					name_hash, key->offset);
+				return -EUCLEAN;
+			}
+		}
+		cur += total_size;
+		di = (struct btrfs_dir_item *)((void *)di + total_size);
+	}
+	return 0;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */
@@ -238,6 +374,11 @@ static int check_leaf_item(struct btrfs_root *root,
 	case BTRFS_EXTENT_CSUM_KEY:
 		ret = check_csum_item(root, leaf, key, slot);
 		break;
+	case BTRFS_DIR_ITEM_KEY:
+	case BTRFS_DIR_INDEX_KEY:
+	case BTRFS_XATTR_ITEM_KEY:
+		ret = check_dir_item(root, leaf, key, slot);
+		break;
 	}
 	return ret;
 }
-- 
2.15.0

[PATCH v2 3/3] btrfs: Cleanup existing name_len checks

[Qu Wenruo]

Since tree-checker has verified leaf when reading from disk, we don't
need the existing verify_dir_item() or btrfs_is_name_len_valid().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Unlike checks in btrfs_mark_buffer_dirty(), the existing checks all
happen in read routine, so there is no need to worry about losing early
warning.
---
 fs/btrfs/ctree.h     |   5 ---
 fs/btrfs/dir-item.c  | 108 ---------------------------------------------------
 fs/btrfs/export.c    |   5 ---
 fs/btrfs/inode.c     |   4 --
 fs/btrfs/props.c     |   7 ----
 fs/btrfs/root-tree.c |   7 ----
 fs/btrfs/send.c      |   6 ---
 fs/btrfs/tree-log.c  |  47 +++++-----------------
 fs/btrfs/xattr.c     |   6 ---
 9 files changed, 9 insertions(+), 186 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f7df5536ab61..8835188a8d51 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3060,15 +3060,10 @@ struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
 					  struct btrfs_path *path, u64 dir,
 					  const char *name, u16 name_len,
 					  int mod);
-int verify_dir_item(struct btrfs_fs_info *fs_info,
-		    struct extent_buffer *leaf, int slot,
-		    struct btrfs_dir_item *dir_item);
 struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info,
 						 struct btrfs_path *path,
 						 const char *name,
 						 int name_len);
-bool btrfs_is_name_len_valid(struct extent_buffer *leaf, int slot,
-			     unsigned long start, u16 name_len);
 
 /* orphan.c */
 int btrfs_insert_orphan_item(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 41cb9196eaa8..cbe421605cd5 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -403,8 +403,6 @@ struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info,
 			btrfs_dir_data_len(leaf, dir_item);
 		name_ptr = (unsigned long)(dir_item + 1);
 
-		if (verify_dir_item(fs_info, leaf, path->slots[0], dir_item))
-			return NULL;
 		if (btrfs_dir_name_len(leaf, dir_item) == name_len &&
 		    memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0)
 			return dir_item;
@@ -450,109 +448,3 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
 	}
 	return ret;
 }
-
-int verify_dir_item(struct btrfs_fs_info *fs_info,
-		    struct extent_buffer *leaf,
-		    int slot,
-		    struct btrfs_dir_item *dir_item)
-{
-	u16 namelen = BTRFS_NAME_LEN;
-	int ret;
-	u8 type = btrfs_dir_type(leaf, dir_item);
-
-	if (type >= BTRFS_FT_MAX) {
-		btrfs_crit(fs_info, "invalid dir item type: %d", (int)type);
-		return 1;
-	}
-
-	if (type == BTRFS_FT_XATTR)
-		namelen = XATTR_NAME_MAX;
-
-	if (btrfs_dir_name_len(leaf, dir_item) > namelen) {
-		btrfs_crit(fs_info, "invalid dir item name len: %u",
-		       (unsigned)btrfs_dir_name_len(leaf, dir_item));
-		return 1;
-	}
-
-	namelen = btrfs_dir_name_len(leaf, dir_item);
-	ret = btrfs_is_name_len_valid(leaf, slot,
-				      (unsigned long)(dir_item + 1), namelen);
-	if (!ret)
-		return 1;
-
-	/* BTRFS_MAX_XATTR_SIZE is the same for all dir items */
-	if ((btrfs_dir_data_len(leaf, dir_item) +
-	     btrfs_dir_name_len(leaf, dir_item)) >
-					BTRFS_MAX_XATTR_SIZE(fs_info)) {
-		btrfs_crit(fs_info, "invalid dir item name + data len: %u + %u",
-			   (unsigned)btrfs_dir_name_len(leaf, dir_item),
-			   (unsigned)btrfs_dir_data_len(leaf, dir_item));
-		return 1;
-	}
-
-	return 0;
-}
-
-bool btrfs_is_name_len_valid(struct extent_buffer *leaf, int slot,
-			     unsigned long start, u16 name_len)
-{
-	struct btrfs_fs_info *fs_info = leaf->fs_info;
-	struct btrfs_key key;
-	u32 read_start;
-	u32 read_end;
-	u32 item_start;
-	u32 item_end;
-	u32 size;
-	bool ret = true;
-
-	ASSERT(start > BTRFS_LEAF_DATA_OFFSET);
-
-	read_start = start - BTRFS_LEAF_DATA_OFFSET;
-	read_end = read_start + name_len;
-	item_start = btrfs_item_offset_nr(leaf, slot);
-	item_end = btrfs_item_end_nr(leaf, slot);
-
-	btrfs_item_key_to_cpu(leaf, &key, slot);
-
-	switch (key.type) {
-	case BTRFS_DIR_ITEM_KEY:
-	case BTRFS_XATTR_ITEM_KEY:
-	case BTRFS_DIR_INDEX_KEY:
-		size = sizeof(struct btrfs_dir_item);
-		break;
-	case BTRFS_INODE_REF_KEY:
-		size = sizeof(struct btrfs_inode_ref);
-		break;
-	case BTRFS_INODE_EXTREF_KEY:
-		size = sizeof(struct btrfs_inode_extref);
-		break;
-	case BTRFS_ROOT_REF_KEY:
-	case BTRFS_ROOT_BACKREF_KEY:
-		size = sizeof(struct btrfs_root_ref);
-		break;
-	default:
-		ret = false;
-		goto out;
-	}
-
-	if (read_start < item_start) {
-		ret = false;
-		goto out;
-	}
-	if (read_end > item_end) {
-		ret = false;
-		goto out;
-	}
-
-	/* there shall be item(s) before name */
-	if (read_start - item_start < size) {
-		ret = false;
-		goto out;
-	}
-
-out:
-	if (!ret)
-		btrfs_crit(fs_info, "invalid dir item name len: %u",
-			   (unsigned int)name_len);
-	return ret;
-}
diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
index fa66980726c9..87144c9f9593 100644
--- a/fs/btrfs/export.c
+++ b/fs/btrfs/export.c
@@ -282,11 +282,6 @@ static int btrfs_get_name(struct dentry *parent, char *name,
 		name_len = btrfs_inode_ref_name_len(leaf, iref);
 	}
 
-	ret = btrfs_is_name_len_valid(leaf, path->slots[0], name_ptr, name_len);
-	if (!ret) {
-		btrfs_free_path(path);
-		return -EIO;
-	}
 	read_extent_buffer(leaf, name, name_ptr, name_len);
 	btrfs_free_path(path);
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b93fe05a39c7..ce5569606b2c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5878,7 +5878,6 @@ static int btrfs_filldir(void *addr, int entries, struct dir_context *ctx)
 static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct inode *inode = file_inode(file);
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_file_private *private = file->private_data;
 	struct btrfs_dir_item *di;
@@ -5946,9 +5945,6 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 		if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
 			goto next;
 		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
-		if (verify_dir_item(fs_info, leaf, slot, di))
-			goto next;
-
 		name_len = btrfs_dir_name_len(leaf, di);
 		if ((total_len + sizeof(struct dir_entry) + name_len) >=
 		    PAGE_SIZE) {
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f6a05f836629..c39a940d0c75 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -164,7 +164,6 @@ static int iterate_object_props(struct btrfs_root *root,
 						 size_t),
 				void *ctx)
 {
-	struct btrfs_fs_info *fs_info = root->fs_info;
 	int ret;
 	char *name_buf = NULL;
 	char *value_buf = NULL;
@@ -215,12 +214,6 @@ static int iterate_object_props(struct btrfs_root *root,
 			name_ptr = (unsigned long)(di + 1);
 			data_ptr = name_ptr + name_len;
 
-			if (verify_dir_item(fs_info, leaf,
-					    path->slots[0], di)) {
-				ret = -EIO;
-				goto out;
-			}
-
 			if (name_len <= XATTR_BTRFS_PREFIX_LEN ||
 			    memcmp_extent_buffer(leaf, XATTR_BTRFS_PREFIX,
 						 name_ptr,
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 3338407ef0f0..aab0194efe46 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -387,13 +387,6 @@ int btrfs_del_root_ref(struct btrfs_trans_handle *trans,
 		WARN_ON(btrfs_root_ref_dirid(leaf, ref) != dirid);
 		WARN_ON(btrfs_root_ref_name_len(leaf, ref) != name_len);
 		ptr = (unsigned long)(ref + 1);
-		ret = btrfs_is_name_len_valid(leaf, path->slots[0], ptr,
-					      name_len);
-		if (!ret) {
-			err = -EIO;
-			goto out;
-		}
-
 		WARN_ON(memcmp_extent_buffer(leaf, name, ptr, name_len));
 		*sequence = btrfs_root_ref_sequence(leaf, ref);
 
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index c10e4c70f02d..2a2dc603da13 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1059,12 +1059,6 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 			}
 		}
 
-		ret = btrfs_is_name_len_valid(eb, path->slots[0],
-			  (unsigned long)(di + 1), name_len + data_len);
-		if (!ret) {
-			ret = -EIO;
-			goto out;
-		}
 		if (name_len + data_len > buf_len) {
 			buf_len = name_len + data_len;
 			if (is_vmalloc_addr(buf)) {
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index aa7c71cff575..1f79405fd933 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1173,19 +1173,15 @@ static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
-static int extref_get_fields(struct extent_buffer *eb, int slot,
-			     unsigned long ref_ptr, u32 *namelen, char **name,
-			     u64 *index, u64 *parent_objectid)
+static int extref_get_fields(struct extent_buffer *eb, unsigned long ref_ptr,
+			     u32 *namelen, char **name, u64 *index,
+			     u64 *parent_objectid)
 {
 	struct btrfs_inode_extref *extref;
 
 	extref = (struct btrfs_inode_extref *)ref_ptr;
 
 	*namelen = btrfs_inode_extref_name_len(eb, extref);
-	if (!btrfs_is_name_len_valid(eb, slot, (unsigned long)&extref->name,
-				     *namelen))
-		return -EIO;
-
 	*name = kmalloc(*namelen, GFP_NOFS);
 	if (*name == NULL)
 		return -ENOMEM;
@@ -1200,19 +1196,14 @@ static int extref_get_fields(struct extent_buffer *eb, int slot,
 	return 0;
 }
 
-static int ref_get_fields(struct extent_buffer *eb, int slot,
-			  unsigned long ref_ptr, u32 *namelen, char **name,
-			  u64 *index)
+static int ref_get_fields(struct extent_buffer *eb, unsigned long ref_ptr,
+			  u32 *namelen, char **name, u64 *index)
 {
 	struct btrfs_inode_ref *ref;
 
 	ref = (struct btrfs_inode_ref *)ref_ptr;
 
 	*namelen = btrfs_inode_ref_name_len(eb, ref);
-	if (!btrfs_is_name_len_valid(eb, slot, (unsigned long)(ref + 1),
-				     *namelen))
-		return -EIO;
-
 	*name = kmalloc(*namelen, GFP_NOFS);
 	if (*name == NULL)
 		return -ENOMEM;
@@ -1287,8 +1278,8 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 
 	while (ref_ptr < ref_end) {
 		if (log_ref_ver) {
-			ret = extref_get_fields(eb, slot, ref_ptr, &namelen,
-					  &name, &ref_index, &parent_objectid);
+			ret = extref_get_fields(eb, ref_ptr, &namelen, &name,
+						&ref_index, &parent_objectid);
 			/*
 			 * parent object can change from one array
 			 * item to another.
@@ -1300,8 +1291,8 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 				goto out;
 			}
 		} else {
-			ret = ref_get_fields(eb, slot, ref_ptr, &namelen,
-					     &name, &ref_index);
+			ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
+					     &ref_index);
 		}
 		if (ret)
 			goto out;
@@ -1835,7 +1826,6 @@ static noinline int replay_one_dir_item(struct btrfs_trans_handle *trans,
 					struct extent_buffer *eb, int slot,
 					struct btrfs_key *key)
 {
-	struct btrfs_fs_info *fs_info = root->fs_info;
 	int ret = 0;
 	u32 item_size = btrfs_item_size_nr(eb, slot);
 	struct btrfs_dir_item *di;
@@ -1848,8 +1838,6 @@ static noinline int replay_one_dir_item(struct btrfs_trans_handle *trans,
 	ptr_end = ptr + item_size;
 	while (ptr < ptr_end) {
 		di = (struct btrfs_dir_item *)ptr;
-		if (verify_dir_item(fs_info, eb, slot, di))
-			return -EIO;
 		name_len = btrfs_dir_name_len(eb, di);
 		ret = replay_one_name(trans, root, path, eb, di, key);
 		if (ret < 0)
@@ -2024,11 +2012,6 @@ static noinline int check_item_in_log(struct btrfs_trans_handle *trans,
 	ptr_end = ptr + item_size;
 	while (ptr < ptr_end) {
 		di = (struct btrfs_dir_item *)ptr;
-		if (verify_dir_item(fs_info, eb, slot, di)) {
-			ret = -EIO;
-			goto out;
-		}
-
 		name_len = btrfs_dir_name_len(eb, di);
 		name = kmalloc(name_len, GFP_NOFS);
 		if (!name) {
@@ -2109,7 +2092,6 @@ static int replay_xattr_deletes(struct btrfs_trans_handle *trans,
 			      struct btrfs_path *path,
 			      const u64 ino)
 {
-	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_key search_key;
 	struct btrfs_path *log_path;
 	int i;
@@ -2151,11 +2133,6 @@ static int replay_xattr_deletes(struct btrfs_trans_handle *trans,
 			u32 this_len = sizeof(*di) + name_len + data_len;
 			char *name;
 
-			ret = verify_dir_item(fs_info, path->nodes[0], i, di);
-			if (ret) {
-				ret = -EIO;
-				goto out;
-			}
 			name = kmalloc(name_len, GFP_NOFS);
 			if (!name) {
 				ret = -ENOMEM;
@@ -4572,12 +4549,6 @@ static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 			this_len = sizeof(*extref) + this_name_len;
 		}
 
-		ret = btrfs_is_name_len_valid(eb, slot, name_ptr,
-					      this_name_len);
-		if (!ret) {
-			ret = -EIO;
-			goto out;
-		}
 		if (this_name_len > name_len) {
 			char *new_name;
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 2c7e53f9ff1b..ad298c248da4 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -267,7 +267,6 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
 	struct btrfs_key key;
 	struct inode *inode = d_inode(dentry);
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	struct btrfs_path *path;
 	int ret = 0;
@@ -336,11 +335,6 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 			u32 this_len = sizeof(*di) + name_len + data_len;
 			unsigned long name_ptr = (unsigned long)(di + 1);
 
-			if (verify_dir_item(fs_info, leaf, slot, di)) {
-				ret = -EIO;
-				goto err;
-			}
-
 			total_size += name_len + 1;
 			/*
 			 * We are just looking for how big our buffer needs to
-- 
2.15.0

Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test

[Nikolay Borisov]

On  8.11.2017 02:54, Qu Wenruo wrote:
> [BUG]
> If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will
> instantly cause kernel panic like:
> 
> ------
> ...
> assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853
> ...
> Call Trace:
>  btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs]
>  setup_items_for_insert+0x385/0x650 [btrfs]
>  __btrfs_drop_extents+0x129a/0x1870 [btrfs]
> ...
> -----
> 
> [Cause]
> Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check
> if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y.
> 
> However quite some btrfs_mark_buffer_dirty() callers(*) don't really
> initialize its item data but only initialize its item pointers, leaving
> item data uninitialized.
> 
> This makes tree-checker catch uninitialized data as error, causing
> such panic.
> 
> *: These callers include but not limited to
> setup_items_for_insert()
> btrfs_split_item()
> btrfs_expand_item()
> 
> [Fix]
> Add a new parameter @check_item_data to btrfs_check_leaf().
> With @check_item_data set to false, item data check will be skipped and
> fallback to old btrfs_check_leaf() behavior.
> 
> So we can still get early warning if we screw up item pointers, and
> avoid false panic.
> 
> Cc: Filipe Manana <fdmanana@gmail.com>
> Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c      | 10 ++++++++--
>  fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++-----
>  fs/btrfs/tree-checker.h | 14 +++++++++++++-
>  3 files changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index efce9a2fa9be..10a2a579cc7f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>  	 * that we don't try and read the other copies of this block, just
>  	 * return -EIO.
>  	 */
> -	if (found_level == 0 && btrfs_check_leaf(root, eb)) {
> +	if (found_level == 0 && btrfs_check_leaf_full(root, eb)) {
>  		set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
>  		ret = -EIO;
>  	}
> @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
>  					 buf->len,
>  					 fs_info->dirty_metadata_batch);
>  #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
> -	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
> +	/*
> +	 * Since btrfs_mark_buffer_dirty() can be called with item pointer set
> +	 * but item data not updated.
> +	 * So here we should only check item pointers, not item data.
> +	 */
> +	if (btrfs_header_level(buf) == 0 &&
> +	    btrfs_check_leaf_relaxed(root, buf)) {
>  		btrfs_print_leaf(buf);
>  		ASSERT(0);
>  	}
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 114fc5f0ecc5..ce4ed6ec8f39 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root,
>  	return ret;
>  }
>  
> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
> +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf,
> +		      bool check_item_data)
>  {
>  	struct btrfs_fs_info *fs_info = root->fs_info;
>  	/* No valid key type is 0, so all key should be larger than this key */
> @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>  			return -EUCLEAN;
>  		}
>  
> -		/* Check if the item size and content meet other criteria */
> -		ret = check_leaf_item(root, leaf, &key, slot);
> -		if (ret < 0)
> -			return ret;
> +		if (check_item_data) {
> +			/*
> +			 * Check if the item size and content meet other
> +			 * criteria
> +			 */
> +			ret = check_leaf_item(root, leaf, &key, slot);
> +			if (ret < 0)
> +				return ret;
> +		}
>  
>  		prev_key.objectid = key.objectid;
>  		prev_key.type = key.type;
> @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>  	return 0;
>  }
>  
> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf)
> +{
> +	return check_leaf(root, leaf, true);
> +}
> +
> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
> +			     struct extent_buffer *leaf)
> +{
> +	return check_leaf(root, leaf, false);
> +}

Presumably the compiler will figure it out but such trivial function are
usually defined straight into the header file so that the compiler
inlines them. Can you check if you have separate objects for
btrfs_check_leaf_relaxed  with the way you've written the patch or
whether all instances have been inlined? If they are not, then move
those function definition into tree-checker.h and make them 'static
inline' as per the style of the whole kernel

> +
>  int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node)
>  {
>  	unsigned long nr = btrfs_header_nritems(node);
> diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
> index 96c486e95d70..3d53e8d6fda0 100644
> --- a/fs/btrfs/tree-checker.h
> +++ b/fs/btrfs/tree-checker.h
> @@ -20,7 +20,19 @@
>  #include "ctree.h"
>  #include "extent_io.h"
>  
> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf);
> +/*
> + * Comprehensive leaf checker.
> + * Will check not only the item pointers, but also every possible member
> + * in item data.
> + */
> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf);
> +
> +/*
> + * Less strict leaf checker.
> + * Will only check item pointers, not reading item data.
> + */
> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
> +			     struct extent_buffer *leaf);
>  int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node);
>  
>  #endif
> 

Re: [PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item

[Nikolay Borisov]

On  8.11.2017 02:54, Qu Wenruo wrote:
> Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and
> XATTR_ITEM.
> 
> This checker does comprehensive check for:
> 1) dir_item header and its data size
>    Against item boundary and maximum name/xattr length.
>    This part is mostly the same as old verify_dir_item().
> 
> 2) dir_type
>    Against maximum file types, and against key type.
>    Since XATTR key should only have FT_XATTR dir item, and normal dir
>    item type should not have XATTR key.
> 
>    The check between key->type and dir_type is newly introduced by this
>    patch.
> 
> 3) name hash
>    For XATTR and DIR_ITEM key, key->offset is name hash (crc32).
>    Check the hash of name against key to ensure it's correct.
> 
>    The name hash check is only found in btrfs-progs before this patch.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 141 insertions(+)
> 


I'm gonna 'hijack' this patch to discuss something  - we do return the
correct EUCLEAN status from the leaf checker, however the only place
where it's called is in btree_readpage_end_io_hook and if the check
fails we only return -EIO. I wonder whether want to propagate the
EUCLEAN from there, any thoughts?

Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test

[Qu Wenruo]

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



On 2017年11月08日 15:55, Nikolay Borisov wrote:
> 
> 
> On  8.11.2017 02:54, Qu Wenruo wrote:
>> [BUG]
>> If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will
>> instantly cause kernel panic like:
>>
>> ------
>> ...
>> assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853
>> ...
>> Call Trace:
>>  btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs]
>>  setup_items_for_insert+0x385/0x650 [btrfs]
>>  __btrfs_drop_extents+0x129a/0x1870 [btrfs]
>> ...
>> -----
>>
>> [Cause]
>> Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check
>> if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y.
>>
>> However quite some btrfs_mark_buffer_dirty() callers(*) don't really
>> initialize its item data but only initialize its item pointers, leaving
>> item data uninitialized.
>>
>> This makes tree-checker catch uninitialized data as error, causing
>> such panic.
>>
>> *: These callers include but not limited to
>> setup_items_for_insert()
>> btrfs_split_item()
>> btrfs_expand_item()
>>
>> [Fix]
>> Add a new parameter @check_item_data to btrfs_check_leaf().
>> With @check_item_data set to false, item data check will be skipped and
>> fallback to old btrfs_check_leaf() behavior.
>>
>> So we can still get early warning if we screw up item pointers, and
>> avoid false panic.
>>
>> Cc: Filipe Manana <fdmanana@gmail.com>
>> Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/disk-io.c      | 10 ++++++++--
>>  fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++-----
>>  fs/btrfs/tree-checker.h | 14 +++++++++++++-
>>  3 files changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index efce9a2fa9be..10a2a579cc7f 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>  	 * that we don't try and read the other copies of this block, just
>>  	 * return -EIO.
>>  	 */
>> -	if (found_level == 0 && btrfs_check_leaf(root, eb)) {
>> +	if (found_level == 0 && btrfs_check_leaf_full(root, eb)) {
>>  		set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
>>  		ret = -EIO;
>>  	}
>> @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
>>  					 buf->len,
>>  					 fs_info->dirty_metadata_batch);
>>  #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
>> -	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
>> +	/*
>> +	 * Since btrfs_mark_buffer_dirty() can be called with item pointer set
>> +	 * but item data not updated.
>> +	 * So here we should only check item pointers, not item data.
>> +	 */
>> +	if (btrfs_header_level(buf) == 0 &&
>> +	    btrfs_check_leaf_relaxed(root, buf)) {
>>  		btrfs_print_leaf(buf);
>>  		ASSERT(0);
>>  	}
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 114fc5f0ecc5..ce4ed6ec8f39 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root,
>>  	return ret;
>>  }
>>  
>> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>> +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf,
>> +		      bool check_item_data)
>>  {
>>  	struct btrfs_fs_info *fs_info = root->fs_info;
>>  	/* No valid key type is 0, so all key should be larger than this key */
>> @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>>  			return -EUCLEAN;
>>  		}
>>  
>> -		/* Check if the item size and content meet other criteria */
>> -		ret = check_leaf_item(root, leaf, &key, slot);
>> -		if (ret < 0)
>> -			return ret;
>> +		if (check_item_data) {
>> +			/*
>> +			 * Check if the item size and content meet other
>> +			 * criteria
>> +			 */
>> +			ret = check_leaf_item(root, leaf, &key, slot);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>>  
>>  		prev_key.objectid = key.objectid;
>>  		prev_key.type = key.type;
>> @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>>  	return 0;
>>  }
>>  
>> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf)
>> +{
>> +	return check_leaf(root, leaf, true);
>> +}
>> +
>> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
>> +			     struct extent_buffer *leaf)
>> +{
>> +	return check_leaf(root, leaf, false);
>> +}
> 
> Presumably the compiler will figure it out but such trivial function are
> usually defined straight into the header file so that the compiler
> inlines them.

In that case, the function check_leaf() must be exported, so that we can
inline it in header.

But exporting check_leaf() increases the possibility for caller to use
it incorrectly, so I prefer no to export any internal used function.


And compiler may or may not inline check_leaf() into
btrfs_check_leaf_relaxed() function, but it doesn't matter.

If optimization can be done by compiler, then let compiler to do it.

What we should do is to ensure the abstraction/interface design is good
enough, other than doing possible "over-optimization".

Thanks,
Qu

> Can you check if you have separate objects for
> btrfs_check_leaf_relaxed  with the way you've written the patch or
> whether all instances have been inlined? If they are not, then move
> those function definition into tree-checker.h and make them 'static
> inline' as per the style of the whole kernel
> 
>> +
>>  int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node)
>>  {
>>  	unsigned long nr = btrfs_header_nritems(node);
>> diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
>> index 96c486e95d70..3d53e8d6fda0 100644
>> --- a/fs/btrfs/tree-checker.h
>> +++ b/fs/btrfs/tree-checker.h
>> @@ -20,7 +20,19 @@
>>  #include "ctree.h"
>>  #include "extent_io.h"
>>  
>> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf);
>> +/*
>> + * Comprehensive leaf checker.
>> + * Will check not only the item pointers, but also every possible member
>> + * in item data.
>> + */
>> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf);
>> +
>> +/*
>> + * Less strict leaf checker.
>> + * Will only check item pointers, not reading item data.
>> + */
>> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
>> +			     struct extent_buffer *leaf);
>>  int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node);
>>  
>>  #endif
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

Re: [PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item

[Qu Wenruo]

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



On 2017年11月08日 15:59, Nikolay Borisov wrote:
> 
> 
> On  8.11.2017 02:54, Qu Wenruo wrote:
>> Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and
>> XATTR_ITEM.
>>
>> This checker does comprehensive check for:
>> 1) dir_item header and its data size
>>    Against item boundary and maximum name/xattr length.
>>    This part is mostly the same as old verify_dir_item().
>>
>> 2) dir_type
>>    Against maximum file types, and against key type.
>>    Since XATTR key should only have FT_XATTR dir item, and normal dir
>>    item type should not have XATTR key.
>>
>>    The check between key->type and dir_type is newly introduced by this
>>    patch.
>>
>> 3) name hash
>>    For XATTR and DIR_ITEM key, key->offset is name hash (crc32).
>>    Check the hash of name against key to ensure it's correct.
>>
>>    The name hash check is only found in btrfs-progs before this patch.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 141 insertions(+)
>>
> 
> 
> I'm gonna 'hijack' this patch to discuss something  - we do return the
> correct EUCLEAN status from the leaf checker, however the only place
> where it's called is in btree_readpage_end_io_hook and if the check
> fails we only return -EIO. I wonder whether want to propagate the
> EUCLEAN from there, any thoughts?

In this particular case, I think returning -EUCLEAN is *marginally* better.
Much more detailed info from block layer/csum verifier/tree-checker
makes more sense than just a single return value.

Returning any error to make the reading fail, and cause a graceful error
is good enough.
The returning values between EUCLEAN and EIO makes difference, but less
meaningful than specific output from tree-checker.

On the other hand, if in case where return value is the only meaningful
output to user-space (that's to say no much meaningful extra info like
kernel message), then return value makes sense.

But even in that case, the return value can't give much detailed info.
Here are several common errno:
EIO
ENOSPC
EUCLEAN
ENOMEM
ENOENT

We can only express at most less than 10 error types.
There are tons of places where thing can go wrong.

What we can do is just to try our best to classify these errors, and
hopes it makes sense for end user.
While more meaningful error/warning message will make both developer and
end user happier.

Thanks,
Qu

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


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

Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test

[David Sterba]

On Wed, Nov 08, 2017 at 04:03:35PM +0800, Qu Wenruo wrote:
> >> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf)
> >> +{
> >> +	return check_leaf(root, leaf, true);
> >> +}
> >> +
> >> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
> >> +			     struct extent_buffer *leaf)
> >> +{
> >> +	return check_leaf(root, leaf, false);
> >> +}
> > 
> > Presumably the compiler will figure it out but such trivial function are
> > usually defined straight into the header file so that the compiler
> > inlines them.
> 
> In that case, the function check_leaf() must be exported, so that we can
> inline it in header.
> 
> But exporting check_leaf() increases the possibility for caller to use
> it incorrectly, so I prefer no to export any internal used function.
> 
> 
> And compiler may or may not inline check_leaf() into
> btrfs_check_leaf_relaxed() function, but it doesn't matter.
> 
> If optimization can be done by compiler, then let compiler to do it.
> 
> What we should do is to ensure the abstraction/interface design is good
> enough, other than doing possible "over-optimization".

Though my original idea was closer to what Nikolay says, I'm fine with
the way you've actually implement it. We don't need the extended
check_leaf version that's hidden in the .c file.

The function inlining possibilities will be limited, but this is not a
performance critical code where the effects of inlining could be
observale in practice (I think, no numbers to back that).

Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test

[David Sterba]

On Wed, Nov 08, 2017 at 08:54:24AM +0800, Qu Wenruo wrote:
> [BUG]
> If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will
> instantly cause kernel panic like:
> 
> ------
> ...
> assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853
> ...
> Call Trace:
>  btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs]
>  setup_items_for_insert+0x385/0x650 [btrfs]
>  __btrfs_drop_extents+0x129a/0x1870 [btrfs]
> ...
> -----
> 
> [Cause]
> Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check
> if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y.
> 
> However quite some btrfs_mark_buffer_dirty() callers(*) don't really
> initialize its item data but only initialize its item pointers, leaving
> item data uninitialized.
> 
> This makes tree-checker catch uninitialized data as error, causing
> such panic.
> 
> *: These callers include but not limited to
> setup_items_for_insert()
> btrfs_split_item()
> btrfs_expand_item()
> 
> [Fix]
> Add a new parameter @check_item_data to btrfs_check_leaf().
> With @check_item_data set to false, item data check will be skipped and
> fallback to old btrfs_check_leaf() behavior.
> 
> So we can still get early warning if we screw up item pointers, and
> avoid false panic.
> 
> Cc: Filipe Manana <fdmanana@gmail.com>
> Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item

[David Sterba]

On Wed, Nov 08, 2017 at 08:54:25AM +0800, Qu Wenruo wrote:
> Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and
> XATTR_ITEM.
> 
> This checker does comprehensive check for:
> 1) dir_item header and its data size
>    Against item boundary and maximum name/xattr length.
>    This part is mostly the same as old verify_dir_item().
> 
> 2) dir_type
>    Against maximum file types, and against key type.
>    Since XATTR key should only have FT_XATTR dir item, and normal dir
>    item type should not have XATTR key.
> 
>    The check between key->type and dir_type is newly introduced by this
>    patch.
> 
> 3) name hash
>    For XATTR and DIR_ITEM key, key->offset is name hash (crc32).
>    Check the hash of name against key to ensure it's correct.
> 
>    The name hash check is only found in btrfs-progs before this patch.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 3/3] btrfs: Cleanup existing name_len checks

[David Sterba]

On Wed, Nov 08, 2017 at 08:54:26AM +0800, Qu Wenruo wrote:
> Since tree-checker has verified leaf when reading from disk, we don't
> need the existing verify_dir_item() or btrfs_is_name_len_valid().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item

[Liu Bo]

On Wed, Nov 08, 2017 at 09:59:39AM +0200, Nikolay Borisov wrote:
> 
> 
> On  8.11.2017 02:54, Qu Wenruo wrote:
> > Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and
> > XATTR_ITEM.
> > 
> > This checker does comprehensive check for:
> > 1) dir_item header and its data size
> >    Against item boundary and maximum name/xattr length.
> >    This part is mostly the same as old verify_dir_item().
> > 
> > 2) dir_type
> >    Against maximum file types, and against key type.
> >    Since XATTR key should only have FT_XATTR dir item, and normal dir
> >    item type should not have XATTR key.
> > 
> >    The check between key->type and dir_type is newly introduced by this
> >    patch.
> > 
> > 3) name hash
> >    For XATTR and DIR_ITEM key, key->offset is name hash (crc32).
> >    Check the hash of name against key to ensure it's correct.
> > 
> >    The name hash check is only found in btrfs-progs before this patch.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >  fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 141 insertions(+)
> > 
> 
> 
> I'm gonna 'hijack' this patch to discuss something  - we do return the
> correct EUCLEAN status from the leaf checker, however the only place
> where it's called is in btree_readpage_end_io_hook and if the check
> fails we only return -EIO. I wonder whether want to propagate the
> EUCLEAN from there, any thoughts?

The @ret from readpage_end_io_hook won't go upper layer as it's
wrapped in end_bio_extent_readpage().

The readpage/readpages will just check page's uptodate bit and error
bit.

Thanks,

-liubo

Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test

[Liu Bo]

On Wed, Nov 08, 2017 at 08:54:24AM +0800, Qu Wenruo wrote:
> [BUG]
> If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will
> instantly cause kernel panic like:
> 
> ------
> ...
> assertion failed: 0, file: fs/btrfs/disk-io.c, line: 3853
> ...
> Call Trace:
>  btrfs_mark_buffer_dirty+0x187/0x1f0 [btrfs]
>  setup_items_for_insert+0x385/0x650 [btrfs]
>  __btrfs_drop_extents+0x129a/0x1870 [btrfs]
> ...
> -----
> 
> [Cause]
> Btrfs will call btrfs_check_leaf() in btrfs_mark_buffer_dirty() to check
> if the leaf is valid with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y.
> 
> However quite some btrfs_mark_buffer_dirty() callers(*) don't really
> initialize its item data but only initialize its item pointers, leaving
> item data uninitialized.
> 
> This makes tree-checker catch uninitialized data as error, causing
> such panic.
> 
> *: These callers include but not limited to
> setup_items_for_insert()
> btrfs_split_item()
> btrfs_expand_item()
> 
> [Fix]
> Add a new parameter @check_item_data to btrfs_check_leaf().
> With @check_item_data set to false, item data check will be skipped and
> fallback to old btrfs_check_leaf() behavior.
> 
> So we can still get early warning if we screw up item pointers, and
> avoid false panic.
> 

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo
> Cc: Filipe Manana <fdmanana@gmail.com>
> Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c      | 10 ++++++++--
>  fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++-----
>  fs/btrfs/tree-checker.h | 14 +++++++++++++-
>  3 files changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index efce9a2fa9be..10a2a579cc7f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -610,7 +610,7 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>  	 * that we don't try and read the other copies of this block, just
>  	 * return -EIO.
>  	 */
> -	if (found_level == 0 && btrfs_check_leaf(root, eb)) {
> +	if (found_level == 0 && btrfs_check_leaf_full(root, eb)) {
>  		set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
>  		ret = -EIO;
>  	}
> @@ -3848,7 +3848,13 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
>  					 buf->len,
>  					 fs_info->dirty_metadata_batch);
>  #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
> -	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(root, buf)) {
> +	/*
> +	 * Since btrfs_mark_buffer_dirty() can be called with item pointer set
> +	 * but item data not updated.
> +	 * So here we should only check item pointers, not item data.
> +	 */
> +	if (btrfs_header_level(buf) == 0 &&
> +	    btrfs_check_leaf_relaxed(root, buf)) {
>  		btrfs_print_leaf(buf);
>  		ASSERT(0);
>  	}
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 114fc5f0ecc5..ce4ed6ec8f39 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -242,7 +242,8 @@ static int check_leaf_item(struct btrfs_root *root,
>  	return ret;
>  }
>  
> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
> +static int check_leaf(struct btrfs_root *root, struct extent_buffer *leaf,
> +		      bool check_item_data)
>  {
>  	struct btrfs_fs_info *fs_info = root->fs_info;
>  	/* No valid key type is 0, so all key should be larger than this key */
> @@ -361,10 +362,15 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>  			return -EUCLEAN;
>  		}
>  
> -		/* Check if the item size and content meet other criteria */
> -		ret = check_leaf_item(root, leaf, &key, slot);
> -		if (ret < 0)
> -			return ret;
> +		if (check_item_data) {
> +			/*
> +			 * Check if the item size and content meet other
> +			 * criteria
> +			 */
> +			ret = check_leaf_item(root, leaf, &key, slot);
> +			if (ret < 0)
> +				return ret;
> +		}
>  
>  		prev_key.objectid = key.objectid;
>  		prev_key.type = key.type;
> @@ -374,6 +380,17 @@ int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf)
>  	return 0;
>  }
>  
> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf)
> +{
> +	return check_leaf(root, leaf, true);
> +}
> +
> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
> +			     struct extent_buffer *leaf)
> +{
> +	return check_leaf(root, leaf, false);
> +}
> +
>  int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node)
>  {
>  	unsigned long nr = btrfs_header_nritems(node);
> diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
> index 96c486e95d70..3d53e8d6fda0 100644
> --- a/fs/btrfs/tree-checker.h
> +++ b/fs/btrfs/tree-checker.h
> @@ -20,7 +20,19 @@
>  #include "ctree.h"
>  #include "extent_io.h"
>  
> -int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf);
> +/*
> + * Comprehensive leaf checker.
> + * Will check not only the item pointers, but also every possible member
> + * in item data.
> + */
> +int btrfs_check_leaf_full(struct btrfs_root *root, struct extent_buffer *leaf);
> +
> +/*
> + * Less strict leaf checker.
> + * Will only check item pointers, not reading item data.
> + */
> +int btrfs_check_leaf_relaxed(struct btrfs_root *root,
> +			     struct extent_buffer *leaf);
>  int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node);
>  
>  #endif
> -- 
> 2.15.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html