Re: [PATCH v2 2/2] btrfs: Validate child tree block's level and first key
On 2018年04月07日 01:07, David Sterba wrote: > On Mon, Apr 02, 2018 at 06:47:32PM +0800, Qu Wenruo wrote: >> On 2018年03月28日 23:49, David Sterba wrote: >>> On Tue, Mar 27, 2018 at 08:44:19PM +0800, Qu Wenruo wrote: We have several reports about node pointer points to incorrect child tree blocks, which could have even wrong owner and level but still with valid generation and checksum. Although btrfs check could handle it and print error message like: leaf parent key incorrect 60670574592 Kernel doesn't have enough check on this type of corruption correctly. At least add such check to read_tree_block() and btrfs_read_buffer(), where we need two new parameters @level and @first_key to verify the child tree block. The new @level check is mandatory and all call sites are already modified to extract expected level from its call chain. While @first_key is optional, the following call sites are skipping such check: 1) Root node/leaf As ROOT_ITEM doesn't contain the first key, skip @first_key check. 2) Direct backref Only parent bytenr and level is known and we need to resolve the key all by ourselves, skip @first_key check. Another note of this verification is, it needs extra info from nodeptr or ROOT_ITEM, so it can't fit into current tree-checker framework, which is limited to node/leaf boundary. Signed-off-by: Qu Wenruo --- changelog: v2: Make @level check mandatory, suggesed by Jeff and Nikolay. Change parameter order as @level is now mandatory, put it in front of @first_key. Change verify_parent_level() to verify_key_level() to avoid confusion on the @level parameter. Add btrfs_error() output for CONFIG_BTRFS_DEBUG to help debugging. >>> >>> That's much better overall, thanks. Adding it to next. >> >> Nikolay reported a case where @first_key check seems to cause false alert. >> (Although my xfstests check hasn't exposed it yet) >> >> Please discard this patch since it has the possibility to cause false >> alert for btrfs core functionality. > > Too late, the patch is in master now, so we need to fix it. Seems to be a very rare race in tree operations, still under investigation. Thanks, Qu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] btrfs: Validate child tree block's level and first key
On Mon, Apr 02, 2018 at 06:47:32PM +0800, Qu Wenruo wrote: > On 2018年03月28日 23:49, David Sterba wrote: > > On Tue, Mar 27, 2018 at 08:44:19PM +0800, Qu Wenruo wrote: > >> We have several reports about node pointer points to incorrect child > >> tree blocks, which could have even wrong owner and level but still with > >> valid generation and checksum. > >> > >> Although btrfs check could handle it and print error message like: > >> leaf parent key incorrect 60670574592 > >> > >> Kernel doesn't have enough check on this type of corruption correctly. > >> At least add such check to read_tree_block() and btrfs_read_buffer(), > >> where we need two new parameters @level and @first_key to verify the > >> child tree block. > >> > >> The new @level check is mandatory and all call sites are already > >> modified to extract expected level from its call chain. > >> > >> While @first_key is optional, the following call sites are skipping such > >> check: > >> 1) Root node/leaf > >>As ROOT_ITEM doesn't contain the first key, skip @first_key check. > >> 2) Direct backref > >>Only parent bytenr and level is known and we need to resolve the key > >>all by ourselves, skip @first_key check. > >> > >> Another note of this verification is, it needs extra info from nodeptr > >> or ROOT_ITEM, so it can't fit into current tree-checker framework, which > >> is limited to node/leaf boundary. > >> > >> Signed-off-by: Qu Wenruo > >> --- > >> changelog: > >> v2: > >> Make @level check mandatory, suggesed by Jeff and Nikolay. > >> Change parameter order as @level is now mandatory, put it in front of > >> @first_key. > >> Change verify_parent_level() to verify_key_level() to avoid confusion > >> on the @level parameter. > >> Add btrfs_error() output for CONFIG_BTRFS_DEBUG to help debugging. > > > > That's much better overall, thanks. Adding it to next. > > Nikolay reported a case where @first_key check seems to cause false alert. > (Although my xfstests check hasn't exposed it yet) > > Please discard this patch since it has the possibility to cause false > alert for btrfs core functionality. Too late, the patch is in master now, so we need to fix it. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] btrfs: Validate child tree block's level and first key
On 2018年04月02日 18:47, Qu Wenruo wrote: > > > On 2018年03月28日 23:49, David Sterba wrote: >> On Tue, Mar 27, 2018 at 08:44:19PM +0800, Qu Wenruo wrote: >>> We have several reports about node pointer points to incorrect child >>> tree blocks, which could have even wrong owner and level but still with >>> valid generation and checksum. >>> >>> Although btrfs check could handle it and print error message like: >>> leaf parent key incorrect 60670574592 >>> >>> Kernel doesn't have enough check on this type of corruption correctly. >>> At least add such check to read_tree_block() and btrfs_read_buffer(), >>> where we need two new parameters @level and @first_key to verify the >>> child tree block. >>> >>> The new @level check is mandatory and all call sites are already >>> modified to extract expected level from its call chain. >>> >>> While @first_key is optional, the following call sites are skipping such >>> check: >>> 1) Root node/leaf >>>As ROOT_ITEM doesn't contain the first key, skip @first_key check. >>> 2) Direct backref >>>Only parent bytenr and level is known and we need to resolve the key >>>all by ourselves, skip @first_key check. >>> >>> Another note of this verification is, it needs extra info from nodeptr >>> or ROOT_ITEM, so it can't fit into current tree-checker framework, which >>> is limited to node/leaf boundary. >>> >>> Signed-off-by: Qu Wenruo >>> --- >>> changelog: >>> v2: >>> Make @level check mandatory, suggesed by Jeff and Nikolay. >>> Change parameter order as @level is now mandatory, put it in front of >>> @first_key. >>> Change verify_parent_level() to verify_key_level() to avoid confusion >>> on the @level parameter. >>> Add btrfs_error() output for CONFIG_BTRFS_DEBUG to help debugging. >> >> That's much better overall, thanks. Adding it to next. > > Nikolay reported a case where @first_key check seems to cause false alert. > (Although my xfstests check hasn't exposed it yet) > > Please discard this patch since it has the possibility to cause false > alert for btrfs core functionality. It turns out to be a racing related case in btree operations like push_leaf_right(). So it indeed brings some clue here. (Although still pretty hard to reproduce) Thanks, Qu > > Thanks, > Qu > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 2/2] btrfs: Validate child tree block's level and first key
On 2018年03月28日 23:49, David Sterba wrote: > On Tue, Mar 27, 2018 at 08:44:19PM +0800, Qu Wenruo wrote: >> We have several reports about node pointer points to incorrect child >> tree blocks, which could have even wrong owner and level but still with >> valid generation and checksum. >> >> Although btrfs check could handle it and print error message like: >> leaf parent key incorrect 60670574592 >> >> Kernel doesn't have enough check on this type of corruption correctly. >> At least add such check to read_tree_block() and btrfs_read_buffer(), >> where we need two new parameters @level and @first_key to verify the >> child tree block. >> >> The new @level check is mandatory and all call sites are already >> modified to extract expected level from its call chain. >> >> While @first_key is optional, the following call sites are skipping such >> check: >> 1) Root node/leaf >>As ROOT_ITEM doesn't contain the first key, skip @first_key check. >> 2) Direct backref >>Only parent bytenr and level is known and we need to resolve the key >>all by ourselves, skip @first_key check. >> >> Another note of this verification is, it needs extra info from nodeptr >> or ROOT_ITEM, so it can't fit into current tree-checker framework, which >> is limited to node/leaf boundary. >> >> Signed-off-by: Qu Wenruo >> --- >> changelog: >> v2: >> Make @level check mandatory, suggesed by Jeff and Nikolay. >> Change parameter order as @level is now mandatory, put it in front of >> @first_key. >> Change verify_parent_level() to verify_key_level() to avoid confusion >> on the @level parameter. >> Add btrfs_error() output for CONFIG_BTRFS_DEBUG to help debugging. > > That's much better overall, thanks. Adding it to next. Nikolay reported a case where @first_key check seems to cause false alert. (Although my xfstests check hasn't exposed it yet) Please discard this patch since it has the possibility to cause false alert for btrfs core functionality. Thanks, Qu > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 2/2] btrfs: Validate child tree block's level and first key
On Tue, Mar 27, 2018 at 08:44:19PM +0800, Qu Wenruo wrote: > We have several reports about node pointer points to incorrect child > tree blocks, which could have even wrong owner and level but still with > valid generation and checksum. > > Although btrfs check could handle it and print error message like: > leaf parent key incorrect 60670574592 > > Kernel doesn't have enough check on this type of corruption correctly. > At least add such check to read_tree_block() and btrfs_read_buffer(), > where we need two new parameters @level and @first_key to verify the > child tree block. > > The new @level check is mandatory and all call sites are already > modified to extract expected level from its call chain. > > While @first_key is optional, the following call sites are skipping such > check: > 1) Root node/leaf >As ROOT_ITEM doesn't contain the first key, skip @first_key check. > 2) Direct backref >Only parent bytenr and level is known and we need to resolve the key >all by ourselves, skip @first_key check. > > Another note of this verification is, it needs extra info from nodeptr > or ROOT_ITEM, so it can't fit into current tree-checker framework, which > is limited to node/leaf boundary. > > Signed-off-by: Qu Wenruo > --- > changelog: > v2: > Make @level check mandatory, suggesed by Jeff and Nikolay. > Change parameter order as @level is now mandatory, put it in front of > @first_key. > Change verify_parent_level() to verify_key_level() to avoid confusion > on the @level parameter. > Add btrfs_error() output for CONFIG_BTRFS_DEBUG to help debugging. That's much better overall, thanks. Adding it to next. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] btrfs: Validate child tree block's level and first key
We have several reports about node pointer points to incorrect child
tree blocks, which could have even wrong owner and level but still with
valid generation and checksum.
Although btrfs check could handle it and print error message like:
leaf parent key incorrect 60670574592
Kernel doesn't have enough check on this type of corruption correctly.
At least add such check to read_tree_block() and btrfs_read_buffer(),
where we need two new parameters @level and @first_key to verify the
child tree block.
The new @level check is mandatory and all call sites are already
modified to extract expected level from its call chain.
While @first_key is optional, the following call sites are skipping such
check:
1) Root node/leaf
As ROOT_ITEM doesn't contain the first key, skip @first_key check.
2) Direct backref
Only parent bytenr and level is known and we need to resolve the key
all by ourselves, skip @first_key check.
Another note of this verification is, it needs extra info from nodeptr
or ROOT_ITEM, so it can't fit into current tree-checker framework, which
is limited to node/leaf boundary.
Signed-off-by: Qu Wenruo
---
changelog:
v2:
Make @level check mandatory, suggesed by Jeff and Nikolay.
Change parameter order as @level is now mandatory, put it in front of
@first_key.
Change verify_parent_level() to verify_key_level() to avoid confusion
on the @level parameter.
Add btrfs_error() output for CONFIG_BTRFS_DEBUG to help debugging.
---
fs/btrfs/backref.c | 6 ++--
fs/btrfs/ctree.c | 28 +++
fs/btrfs/disk-io.c | 95 +++---
fs/btrfs/disk-io.h | 8 +++--
fs/btrfs/extent-tree.c | 6 +++-
fs/btrfs/print-tree.c | 10 --
fs/btrfs/qgroup.c | 7 ++--
fs/btrfs/ref-verify.c | 7 +++-
fs/btrfs/relocation.c | 21 ---
fs/btrfs/tree-log.c| 28 +--
10 files changed, 170 insertions(+), 46 deletions(-)
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 26484648d090..ec71c7e66cfa 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -738,7 +738,8 @@ static int add_missing_keys(struct btrfs_fs_info *fs_info,
BUG_ON(ref->key_for_search.type);
BUG_ON(!ref->wanted_disk_byte);
- eb = read_tree_block(fs_info, ref->wanted_disk_byte, 0);
+ eb = read_tree_block(fs_info, ref->wanted_disk_byte, 0,
+ref->level - 1, NULL);
if (IS_ERR(eb)) {
free_pref(ref);
return PTR_ERR(eb);
@@ -1291,7 +1292,8 @@ static int find_parent_nodes(struct btrfs_trans_handle
*trans,
ref->level == 0) {
struct extent_buffer *eb;
- eb = read_tree_block(fs_info, ref->parent, 0);
+ eb = read_tree_block(fs_info, ref->parent, 0,
+ref->level, NULL);
if (IS_ERR(eb)) {
ret = PTR_ERR(eb);
goto out;
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b88a79e69ddf..4f1aefccd34a 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1418,6 +1418,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
struct tree_mod_root *old_root = NULL;
u64 old_generation = 0;
u64 logical;
+ int level;
eb_root = btrfs_read_lock_root_node(root);
tm = __tree_mod_log_oldest_root(fs_info, eb_root, time_seq);
@@ -1428,15 +1429,17 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
old_root = &tm->old_root;
old_generation = tm->generation;
logical = old_root->logical;
+ level = old_root->level;
} else {
logical = eb_root->start;
+ level = btrfs_header_level(eb_root);
}
tm = tree_mod_log_search(fs_info, logical, time_seq);
if (old_root && tm && tm->op != MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
btrfs_tree_read_unlock(eb_root);
free_extent_buffer(eb_root);
- old = read_tree_block(fs_info, logical, 0);
+ old = read_tree_block(fs_info, logical, 0, level, NULL);
if (WARN_ON(IS_ERR(old) || !extent_buffer_uptodate(old))) {
if (!IS_ERR(old))
free_extent_buffer(old);
@@ -1656,6 +1659,7 @@ int btrfs_realloc_node(struct btrfs_trans_handle *trans,
btrfs_set_lock_blocking(parent);
for (i = start_slot; i <= end_slot; i++) {
+ struct btrfs_key first_key;
int close = 1;
btrfs_node_key(parent, &disk_key, i);
@@ -1665,6 +1669,7 @@ int btrfs_realloc_node(struct btrfs_trans_handle *trans,
progress_passed = 1;
blocknr = btrfs_node_
