tree-checker bug fix and enhancement.
v2 cover tree-checker bug fix and enhancement.
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(-)
Signature · 1 line
2.15.0
PATCH v2 1/3 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 tosetup_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.
--- 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(-)
fs/btrfs/disk-io.c +8 -2
@@ -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 ); }
fs/btrfs/tree-checker.c +22 -5
@@ -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);
fs/btrfs/tree-checker.h +13 -1
@@ -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
Signature · 1 line
2.15.0
Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test
Quoted 114 lines · Qu Wenruo
[BUG]
If we run btrfs withCONFIG_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 callbtrfs_check_leaf()inbtrfs_mark_buffer_dirty()to check if the leaf is valid withCONFIG_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 tobtrfs_check_leaf(). With @check_item_data set to false, item data check will be skipped and fallback to oldbtrfs_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 | 10 ++++++++-- fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++----- fs/btrfs/tree-checker.h | 14 +++++++++++++- 3 files changed, 43 insertions(+), 8 deletions(-)fs/btrfs/disk-io.c +8
-2@@ -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 ); }fs/btrfs/tree-checker.c +11
-5@@ -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 ; }
fs/btrfs/tree-checker.c int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) · line 392
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-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
Quoted 30 lines
fs/btrfs/tree-checker.c +1
-0+int btrfs_check_node (struct btrfs_root *root,struct extent_buffer *node) {unsigned long nr =btrfs_header_nritems (node);fs/btrfs/tree-checker.h +13
-1@@ -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 1/3] btrfs: tree-checker: Fix false panic for sanity test
Quoted 117 lines · Nikolay Borisov
Quoted 114 lines · Qu Wenruo
[BUG]
If we run btrfs withCONFIG_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 callbtrfs_check_leaf()inbtrfs_mark_buffer_dirty()to check if the leaf is valid withCONFIG_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 tobtrfs_check_leaf(). With @check_item_data set to false, item data check will be skipped and fallback to oldbtrfs_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 | 10 ++++++++-- fs/btrfs/tree-checker.c | 27 ++++++++++++++++++++++----- fs/btrfs/tree-checker.h | 14 +++++++++++++- 3 files changed, 43 insertions(+), 8 deletions(-)fs/btrfs/disk-io.c +8
-2@@ -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 ); }fs/btrfs/tree-checker.c +11
-5@@ -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 ; }
Quoted 14 lines
fs/btrfs/tree-checker.c int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) · line 392
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-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
Quoted 41 lines
Can you check if you have separate objects for
btrfs_check_leaf_relaxedwith the way you've written the patch or whether all instances have been inlined? If they are not, then move those function definition intotree-checker.hand make them 'static inline' as per the style of the whole kernelQuoted 20 lines
fs/btrfs/tree-checker.c +1
-0+int btrfs_check_node (struct btrfs_root *root,struct extent_buffer *node) {unsigned long nr =btrfs_header_nritems (node);fs/btrfs/tree-checker.h +6
-1@@ -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);fs/btrfs/tree-checker.h line 38
Quoted 10 lines
fs/btrfs/tree-checker.h +7
-0+ +/* +* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test
Quoted 22 lines · Qu Wenruo
Quoted 6 lines
Quoted 6 lines
fs/btrfs/tree-checker.c +6
-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,Quoted 8 lines
fs/btrfs/tree-checker.c int btrfs_check_leaf(struct btrfs_root *root, struct extent_buffer *leaf) · line 392
Quoted 4 lines
fs/btrfs/tree-checker.c +4
-0+ 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.
Quoted 7 lines
And compiler may or may not inline
check_leaf()intobtrfs_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
In reply to Qu WenruoQuoted 36 lines · Qu Wenruo
[BUG]
If we run btrfs withCONFIG_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 callbtrfs_check_leaf()inbtrfs_mark_buffer_dirty()to check if the leaf is valid withCONFIG_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 tobtrfs_check_leaf(). With @check_item_data set to false, item data check will be skipped and fallback to oldbtrfs_check_leaf()behavior.
Quoted 6 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]>
Re: [PATCH v2 1/3] btrfs: tree-checker: Fix false panic for sanity test
In reply to Qu WenruoQuoted 31 lines · Qu Wenruo
[BUG]
If we run btrfs withCONFIG_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 callbtrfs_check_leaf()inbtrfs_mark_buffer_dirty()to check if the leaf is valid withCONFIG_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
[Fix]
Add a new parameter @check_item_data tobtrfs_check_leaf(). With @check_item_data set to false, item data check will be skipped and fallback to oldbtrfs_check_leaf()behavior.So we can still get early warning if we screw up item pointers, and avoid false panic.
Thanks,
-liubo
Quoted 121 lines
Cc Filipe Manana <[email protected]>Reported-by Lakshmipathi.G <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- 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(-)fs/btrfs/disk-io.c +8
-2@@ -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 ); }fs/btrfs/tree-checker.c +22
-5@@ -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);fs/btrfs/tree-checker.h +13
-1@@ -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 Signature · 6 lines
2.15.0 -- 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
PATCH v2 2/3 btrfs: tree-checker: Add checker for dir item
In reply to Qu WenruoAdd 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.
--- fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)
fs/btrfs/tree-checker.c +141 -0
@@ -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; }
Signature · 1 line
2.15.0
Re: [PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item
Quoted 20 lines · Qu Wenruo
Add checker for dir item, for key types
DIR_ITEM,DIR_INDEXandXATTR_ITEM.This checker does comprehensive check for:
1)dir_itemheader and its data size
Against item boundary and maximum name/xattr length. This part is mostly the same as oldverify_dir_item().2)
dir_type
Against maximum file types, and against key type.
Since XATTR key should only haveFT_XATTRdir item, and normal dir item type should not have XATTR key.The check between
key->typeanddir_typeis newly introduced by this patch.3) name hash
For XATTR andDIR_ITEMkey,key->offsetis name hash (crc32). Check the hash of name against key to ensure it's correct.
Quoted 7 lines
The name hash check is only found in btrfs-progs before this patch.
Signed-off-by Qu Wenruo <[email protected]>--- 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 2/3] btrfs: tree-checker: Add checker for dir item
Quoted 32 lines · Nikolay Borisov
Quoted 20 lines · Qu Wenruo
Add checker for dir item, for key types
DIR_ITEM,DIR_INDEXandXATTR_ITEM.This checker does comprehensive check for:
1)dir_itemheader and its data size
Against item boundary and maximum name/xattr length. This part is mostly the same as oldverify_dir_item().2)
dir_type
Against maximum file types, and against key type.
Since XATTR key should only haveFT_XATTRdir item, and normal dir item type should not have XATTR key.The check between
key->typeanddir_typeis newly introduced by this patch.3) name hash
For XATTR andDIR_ITEMkey,key->offsetis name hash (crc32). Check the hash of name against key to ensure it's correct.Quoted 7 lines
The name hash check is only found in btrfs-progs before this patch.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)
Quoted 5 lines
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_hookand 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
Quoted 7 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
Re: [PATCH v2 2/3] btrfs: tree-checker: Add checker for dir item
In reply to Nikolay BorisovQuoted 32 lines · Nikolay Borisov
Quoted 20 lines · Qu Wenruo
Add checker for dir item, for key types
DIR_ITEM,DIR_INDEXandXATTR_ITEM.This checker does comprehensive check for:
1)dir_itemheader and its data size
Against item boundary and maximum name/xattr length. This part is mostly the same as oldverify_dir_item().2)
dir_type
Against maximum file types, and against key type.
Since XATTR key should only haveFT_XATTRdir item, and normal dir item type should not have XATTR key.The check between
key->typeanddir_typeis newly introduced by this patch.3) name hash
For XATTR andDIR_ITEMkey,key->offsetis name hash (crc32). Check the hash of name against key to ensure it's correct.Quoted 7 lines
The name hash check is only found in btrfs-progs before this patch.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)
Quoted 5 lines
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_hookand 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 2/3] btrfs: tree-checker: Add checker for dir item
In reply to Qu WenruoQuoted 16 lines · Qu Wenruo
Add checker for dir item, for key types
DIR_ITEM,DIR_INDEXandXATTR_ITEM.This checker does comprehensive check for:
1)dir_itemheader and its data size
Against item boundary and maximum name/xattr length. This part is mostly the same as oldverify_dir_item().2)
dir_type
Against maximum file types, and against key type.
Since XATTR key should only haveFT_XATTRdir item, and normal dir item type should not have XATTR key.The check between
key->typeanddir_typeis newly introduced by this patch.
Quoted 7 lines
3) name hash
For XATTR andDIR_ITEMkey,key->offsetis 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 <[email protected]>
PATCH v2 3/3 btrfs: Cleanup existing name_len checks
In reply to Qu WenruoSince tree-checker has verified leaf when reading from disk, we don't need the existing verify_dir_item() or btrfs_is_name_len_valid().
---
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(-)
fs/btrfs/ctree.h +0 -5
@@ -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,
fs/btrfs/dir-item.c +0 -108
117 lines not on this page · raw message
fs/btrfs/export.c +0 -5
@@ -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);
fs/btrfs/inode.c +0 -4
@@ -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 ) {
fs/btrfs/props.c +0 -7
@@ -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,
fs/btrfs/root-tree.c +0 -7
@@ -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);
fs/btrfs/send.c +0 -6
@@ -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)) {
fs/btrfs/tree-log.c +9 -38
119 lines not on this page · raw message
fs/btrfs/xattr.c +0 -6
@@ -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
Signature · 1 line
2.15.0
Re: [PATCH v2 3/3] btrfs: Cleanup existing name_len checks
Quoted 4 lines · Qu Wenruo
Since tree-checker has verified leaf when reading from disk, we don't need the existing
verify_dir_item()orbtrfs_is_name_len_valid().Signed-off-by Qu Wenruo <[email protected]>