| Message ID | a15ff1e397312309c554482e55d929488e22dcca.1690970028.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix an ASSERT() triggered inside prepare_to_merge() | expand |
On Wed, Aug 2, 2023 at 12:24 PM Qu Wenruo <wqu@suse.com> wrote: > > [BUG] > Syzbot reported a crash that an ASSERT() got triggered inside > prepare_to_merge(). > > That ASSERT() makes sure the reloc tree is properly pointed back by its > subvolume tree. > > [CAUSE] > After more debugging output, it turns out we had an invalid reloc tree: > > BTRFS error (device loop1): reloc tree mismatch, root 8 has no reloc root, expect reloc root key (-8, 132, 8) gen 17 > > Note the above root key is (TREE_RELOC_OBJECTID, ROOT_ITEM, > QUOTA_TREE_OBJECTID), meaning it's a reloc tree for quota tree. > > But reloc trees can only exist for subvolumes, as for non-subvolume > trees, we just COW the involved tree block, no need to create a reloc > tree since those tree blocks won't be shared with other trees. > > Only subvolumes tree can share tree blocks with other trees (thus they > have BTRFS_ROOT_SHAREABLE flag). > > Thus this new debug output proves my previous assumption that corrupted > on-disk data can trigger that ASSERT(). > > [FIX] > Besides the dedicated fix and the graceful exit, also let tree-checker to > check such root keys, to make sure reloc trees can only exist for > subvolumes. > > Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/disk-io.c | 3 ++- > fs/btrfs/tree-checker.c | 15 +++++++++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 5fd336c597e9..a01eac963075 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1103,7 +1103,8 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) > btrfs_drew_lock_init(&root->snapshot_lock); > > if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID && > - !btrfs_is_data_reloc_root(root)) { > + !btrfs_is_data_reloc_root(root) && > + is_fstree(root->root_key.objectid)) { > set_bit(BTRFS_ROOT_SHAREABLE, &root->state); > btrfs_check_and_init_root_item(&root->root_item); > } > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 038dfa8f1788..dabb86314a4c 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -446,6 +446,21 @@ static int check_root_key(struct extent_buffer *leaf, struct btrfs_key *key, > btrfs_item_key_to_cpu(leaf, &item_key, slot); > is_root_item = (item_key.type == BTRFS_ROOT_ITEM_KEY); > > + /* > + * Bad rootid for reloc trees. > + * > + * Reloc trees are only for subvolume trees, other trees only needs > + * a COW to be relocated. ... other trees only need to be COWed to be relocated. > + */ > + if (unlikely(is_root_item && key->objectid == BTRFS_TREE_RELOC_OBJECTID && > + !is_fstree(key->offset))) { > + generic_err(leaf, slot, > + "invalid reloc tree for root %lld, root id is not a subvolume tree", > + key->offset); > + dump_stack(); Same logic as for places that do WARN_ON() or dump leaves: the dump_stack() should come before the error message. Other than that, it looks good to me: Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > + return -EUCLEAN; > + } > + > /* No such tree id */ > if (unlikely(key->objectid == 0)) { > if (is_root_item) > -- > 2.41.0 >
On 2023/8/2 20:11, Filipe Manana wrote: > On Wed, Aug 2, 2023 at 12:24 PM Qu Wenruo <wqu@suse.com> wrote: >> >> [BUG] >> Syzbot reported a crash that an ASSERT() got triggered inside >> prepare_to_merge(). >> >> That ASSERT() makes sure the reloc tree is properly pointed back by its >> subvolume tree. >> >> [CAUSE] >> After more debugging output, it turns out we had an invalid reloc tree: >> >> BTRFS error (device loop1): reloc tree mismatch, root 8 has no reloc root, expect reloc root key (-8, 132, 8) gen 17 >> >> Note the above root key is (TREE_RELOC_OBJECTID, ROOT_ITEM, >> QUOTA_TREE_OBJECTID), meaning it's a reloc tree for quota tree. >> >> But reloc trees can only exist for subvolumes, as for non-subvolume >> trees, we just COW the involved tree block, no need to create a reloc >> tree since those tree blocks won't be shared with other trees. >> >> Only subvolumes tree can share tree blocks with other trees (thus they >> have BTRFS_ROOT_SHAREABLE flag). >> >> Thus this new debug output proves my previous assumption that corrupted >> on-disk data can trigger that ASSERT(). >> >> [FIX] >> Besides the dedicated fix and the graceful exit, also let tree-checker to >> check such root keys, to make sure reloc trees can only exist for >> subvolumes. >> >> Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/disk-io.c | 3 ++- >> fs/btrfs/tree-checker.c | 15 +++++++++++++++ >> 2 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 5fd336c597e9..a01eac963075 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -1103,7 +1103,8 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) >> btrfs_drew_lock_init(&root->snapshot_lock); >> >> if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID && >> - !btrfs_is_data_reloc_root(root)) { >> + !btrfs_is_data_reloc_root(root) && >> + is_fstree(root->root_key.objectid)) { >> set_bit(BTRFS_ROOT_SHAREABLE, &root->state); >> btrfs_check_and_init_root_item(&root->root_item); >> } >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 038dfa8f1788..dabb86314a4c 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -446,6 +446,21 @@ static int check_root_key(struct extent_buffer *leaf, struct btrfs_key *key, >> btrfs_item_key_to_cpu(leaf, &item_key, slot); >> is_root_item = (item_key.type == BTRFS_ROOT_ITEM_KEY); >> >> + /* >> + * Bad rootid for reloc trees. >> + * >> + * Reloc trees are only for subvolume trees, other trees only needs >> + * a COW to be relocated. > > ... other trees only need to be COWed to be relocated. > >> + */ >> + if (unlikely(is_root_item && key->objectid == BTRFS_TREE_RELOC_OBJECTID && >> + !is_fstree(key->offset))) { >> + generic_err(leaf, slot, >> + "invalid reloc tree for root %lld, root id is not a subvolume tree", >> + key->offset); >> + dump_stack(); > > Same logic as for places that do WARN_ON() or dump leaves: the > dump_stack() should come before the error message. Oh my bad, the dump_stack() is only for debug purpose, should be removed. Thanks, Qu > > Other than that, it looks good to me: > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > > Thanks. > >> + return -EUCLEAN; >> + } >> + >> /* No such tree id */ >> if (unlikely(key->objectid == 0)) { >> if (is_root_item) >> -- >> 2.41.0 >>
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 5fd336c597e9..a01eac963075 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1103,7 +1103,8 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) btrfs_drew_lock_init(&root->snapshot_lock); if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID && - !btrfs_is_data_reloc_root(root)) { + !btrfs_is_data_reloc_root(root) && + is_fstree(root->root_key.objectid)) { set_bit(BTRFS_ROOT_SHAREABLE, &root->state); btrfs_check_and_init_root_item(&root->root_item); } diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 038dfa8f1788..dabb86314a4c 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -446,6 +446,21 @@ static int check_root_key(struct extent_buffer *leaf, struct btrfs_key *key, btrfs_item_key_to_cpu(leaf, &item_key, slot); is_root_item = (item_key.type == BTRFS_ROOT_ITEM_KEY); + /* + * Bad rootid for reloc trees. + * + * Reloc trees are only for subvolume trees, other trees only needs + * a COW to be relocated. + */ + if (unlikely(is_root_item && key->objectid == BTRFS_TREE_RELOC_OBJECTID && + !is_fstree(key->offset))) { + generic_err(leaf, slot, + "invalid reloc tree for root %lld, root id is not a subvolume tree", + key->offset); + dump_stack(); + return -EUCLEAN; + } + /* No such tree id */ if (unlikely(key->objectid == 0)) { if (is_root_item)
[BUG] Syzbot reported a crash that an ASSERT() got triggered inside prepare_to_merge(). That ASSERT() makes sure the reloc tree is properly pointed back by its subvolume tree. [CAUSE] After more debugging output, it turns out we had an invalid reloc tree: BTRFS error (device loop1): reloc tree mismatch, root 8 has no reloc root, expect reloc root key (-8, 132, 8) gen 17 Note the above root key is (TREE_RELOC_OBJECTID, ROOT_ITEM, QUOTA_TREE_OBJECTID), meaning it's a reloc tree for quota tree. But reloc trees can only exist for subvolumes, as for non-subvolume trees, we just COW the involved tree block, no need to create a reloc tree since those tree blocks won't be shared with other trees. Only subvolumes tree can share tree blocks with other trees (thus they have BTRFS_ROOT_SHAREABLE flag). Thus this new debug output proves my previous assumption that corrupted on-disk data can trigger that ASSERT(). [FIX] Besides the dedicated fix and the graceful exit, also let tree-checker to check such root keys, to make sure reloc trees can only exist for subvolumes. Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/disk-io.c | 3 ++- fs/btrfs/tree-checker.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-)