btrfs: tree-checker: Fix false panic for sanity test

PATCH In review Qu Wenruo 3 participants 3 messages Nov 6, 2017 – Nov 8, 2017 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2017-11-06 05:47 UTC #0 lore raw

PATCH 1/1 btrfs: tree-checker: Fix false panic for sanity test

[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 <[email protected]>

Reported-by Lakshmipathi.G <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

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

fs/btrfs/disk-io.c +3 -2

index efce9a2fa9be..c65b63d6df27 100644
@@ -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);
 	}

fs/btrfs/tree-checker.c +11 -5

index 114fc5f0ecc5..a4c2517fa2a1 100644
@@ -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;

fs/btrfs/tree-checker.h +2 -1

index 96c486e95d70..9377028e9608 100644
@@ -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

Signature · 1 line

2.14.3

David Sterba

2017-11-07 21:14 UTC #1 lore raw

Quoted 3 lines · Qu Wenruo

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

Which patch causes that? The selftests used to catch various errors when the original dir_item verification patches were merged. Plus some fstests were failing. I burned a lot of time on that and don't want to repeat that, so I expect that all tree-checker patches pass self-test (incrementally, not just the whole series) and all relevant fstests.

Quoted 28 lines

------
...
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()

Quoted 4 lines

[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 this looks like a patch ordering problem. The weaker version of btrfs_check_leaf needs to come first and then the improved checks that cause the crashes.

Quoted 40 lines

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

Cc Filipe Manana <[email protected]>

Reported-by Lakshmipathi.G <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

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

fs/btrfs/disk-io.c +3 -2

index efce9a2fa9be..c65b63d6df27 100644
@@ -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);
 	}

fs/btrfs/tree-checker.c +0 -0

index 114fc5f0ecc5..a4c2517fa2a1 100644

fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 246

Quoted 6 lines

fs/btrfs/tree-checker.c +2 -1

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

The bool arguments are usally not very descriptive, I suggest to add

static inline int btrfs_check_leaf_relaxed(struct btrfs_root *root, struct extent_buffer *leaf)

{
	return btrfs_check_leaf_root, leaf, false);
}

to tree-check.h and use where appropriate.

Qu Wenruo

2017-11-08 00:10 UTC #2 lore raw

Quoted 6 lines · David Sterba

Quoted 3 lines · Qu Wenruo
[BUG]
If we run btrfs with CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y, it will instantly cause kernel panic like:
Which patch causes that?

To be specific, the file extent item checker. (As that's the first patch to check item members)
But to be honest, any checker checks member value will cause it.

Quoted 5 lines

The selftests used to catch various errors when
the original dir_item verification patches were merged. Plus some fstests were failing. I burned a lot of time on that and don't want to repeat that, so I expect that all tree-checker patches pass self-test (incrementally, not just the whole series) and all relevant fstests.

OK, I'll enable selftest in my kernel config.

Quoted 29 lines

Quoted 29 lines
------
...
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()

Quoted 8 lines

Quoted 4 lines
[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 this looks like a patch ordering problem. The weaker version of btrfs_check_leaf needs to come first and then the improved checks that cause the crashes.

Yes, that should be the case.

Quoted 50 lines

Quoted 41 lines

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

Cc Filipe Manana <[email protected]>

Reported-by Lakshmipathi.G <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

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

fs/btrfs/disk-io.c +3 -2

index efce9a2fa9be..c65b63d6df27 100644
@@ -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);
 	}

fs/btrfs/tree-checker.c +0 -0

index 114fc5f0ecc5..a4c2517fa2a1 100644

fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 246

Quoted 6 lines

fs/btrfs/tree-checker.c +2 -1

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

The bool arguments are usally not very descriptive, I suggest to add

Quoted 6 lines

static inline int btrfs_check_leaf_relaxed(struct btrfs_root *root, struct extent_buffer *leaf)
{
	return btrfs_check_leaf_root, leaf, false);
}
to tree-check.h and use where appropriate.

OK, I'll update the patch.

Quoted 5 lines

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