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

[PATCH 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.

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      |   5 +-
 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 | 157 ++++++++++++++++++++++++++++++++++++++++++++++--
 fs/btrfs/tree-checker.h |   3 +-
 fs/btrfs/tree-log.c     |  47 +++------------
 fs/btrfs/xattr.c        |   6 --
 12 files changed, 166 insertions(+), 194 deletions(-)

-- 
2.15.0

[PATCH 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      |  5 +++--
 fs/btrfs/tree-checker.c | 16 +++++++++++-----
 fs/btrfs/tree-checker.h |  3 ++-
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index efce9a2fa9be..c65b63d6df27 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(root, eb, true)) {
 		set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
 		ret = -EIO;
 	}
@@ -3848,7 +3848,8 @@ 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)) {
+	if (btrfs_header_level(buf) == 0 &&
+	    btrfs_check_leaf(root, buf, false)) {
 		btrfs_print_leaf(buf);
 		ASSERT(0);
 	}
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 114fc5f0ecc5..a4c2517fa2a1 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)
+int btrfs_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;
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 96c486e95d70..9377028e9608 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -20,7 +20,8 @@
 #include "ctree.h"
 #include "extent_io.h"
 
-int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf);
+int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf,
+		     bool check_item_data);
 int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node);
 
 #endif
-- 
2.15.0

[PATCH 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 a4c2517fa2a1..9644c9616f95 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 expect (%u, %u]",
+				cur + sizeof(*di), cur, 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 expect (0, %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 expect (0, %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 expect (%lu, %u]",
+				cur + total_size, cur + sizeof(*di), 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 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 2/3] btrfs: tree-checker: Add checker for dir item

[Nikolay Borisov]

On  6.11.2017 11:20, 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.

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

> 
> 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 a4c2517fa2a1..9644c9616f95 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 expect (%u, %u]",
> +				cur + sizeof(*di), cur, 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 expect (0, %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 expect (0, %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 expect (%lu, %u]",
> +				cur + total_size, cur + sizeof(*di), 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;
>  }
> 

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

[Nikolay Borisov]

On  6.11.2017 11:20, 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: Nikolay Borisov <nborisov@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(-)

That's the diff stat trend I'd like to see :>

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

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

[Su Yue]

On 11/06/2017 05:20 PM, 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.
> 

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>

> 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 a4c2517fa2a1..9644c9616f95 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 expect (%u, %u]",
> +				cur + sizeof(*di), cur, 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 expect (0, %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 expect (0, %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 expect (%lu, %u]",
> +				cur + total_size, cur + sizeof(*di), 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;
>   }
>