* [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images @ 2019-07-16 9:00 Qu Wenruo 2019-07-16 9:00 ` [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() Qu Wenruo ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Qu Wenruo @ 2019-07-16 9:00 UTC (permalink / raw) To: linux-btrfs; +Cc: Jungyeon Yoon This wave has the following features: - Hunt down BUG_ON() in btrfs_delete_delayed_dir_index() EEXIST can cause BUG_ON(). And all callers of this function has already handled error by aborting transacation. - Only allocate extents from the same block group type This is a very tricky bug, needs MIXED_GROUP super flag with regular block groups (separate META and DATA) and corrupted extent tree. - ROOT_ITEM check for tree checker This kills the unaligned bytenr, invalid level and incorrect reloc tree. Reported-by: Jungyeon Yoon <jungyeon.yoon@gmail.com> Qu Wenruo (3): btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() btrfs: extent-tree: Make sure we only allocate extents from block groups with the same type btrfs: tree-checker: Add ROOT_ITEM check fs/btrfs/delayed-inode.c | 14 +++++- fs/btrfs/extent-tree.c | 9 ++++ fs/btrfs/tree-checker.c | 92 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) -- 2.22.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() 2019-07-16 9:00 [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images Qu Wenruo @ 2019-07-16 9:00 ` Qu Wenruo 2019-07-31 15:44 ` David Sterba 2019-07-16 9:00 ` [PATCH 2/3] btrfs: extent-tree: Make sure we only allocate extents from block groups with the same type Qu Wenruo ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Qu Wenruo @ 2019-07-16 9:00 UTC (permalink / raw) To: linux-btrfs There is one report of fuzzed image which leads to BUG_ON() in btrfs_delete_delayed_dir_index(). Although that fuzzed image can already be addressed by enhanced extent-tree error handler, it's still better to hunt down more BUG_ON(). This patch will hunt down two BUG_ON()s in btrfs_delete_delayed_dir_index(): - One for error from btrfs_delayed_item_reserve_metadata() Instead of BUG_ON(), we output an error message and free the item. And return the error. All callers of this function handles the error by aborting current trasaction. - One for possible EEXIST from __btrfs_add_delayed_deletion_item() That function can return -EEXIST. We already have a good enough error message for that, only need to clean up the reserved metadata space and allocated item. To help above cleanup, also modifiy __btrfs_remove_delayed_item() called in btrfs_release_delayed_item(), to skip unassociated item. Link: https://bugzilla.kernel.org/show_bug.cgi?id=203253 Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/delayed-inode.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index 43fdb2992956..c4946d58f49b 100644 --- a/fs/btrfs/delayed-inode.c +++ b/fs/btrfs/delayed-inode.c @@ -474,6 +474,9 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item) struct rb_root_cached *root; struct btrfs_delayed_root *delayed_root; + /* Not associated with any delayed_node */ + if (!delayed_item->delayed_node) + return; delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root; BUG_ON(!delayed_root); @@ -1525,7 +1528,13 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans, * we have reserved enough space when we start a new transaction, * so reserving metadata failure is impossible. */ - BUG_ON(ret); + if (ret < 0) { + btrfs_err(trans->fs_info, +"metadata reserve fail at %s, should have already reserved space, ret=%d", + __func__, ret); + btrfs_release_delayed_item(item); + goto end; + } mutex_lock(&node->mutex); ret = __btrfs_add_delayed_deletion_item(node, item); @@ -1534,7 +1543,8 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans, "err add delayed dir index item(index: %llu) into the deletion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)", index, node->root->root_key.objectid, node->inode_id, ret); - BUG(); + btrfs_delayed_item_release_metadata(dir->root, item); + btrfs_release_delayed_item(item); } mutex_unlock(&node->mutex); end: -- 2.22.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() 2019-07-16 9:00 ` [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() Qu Wenruo @ 2019-07-31 15:44 ` David Sterba 0 siblings, 0 replies; 9+ messages in thread From: David Sterba @ 2019-07-31 15:44 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Tue, Jul 16, 2019 at 05:00:32PM +0800, Qu Wenruo wrote: > There is one report of fuzzed image which leads to BUG_ON() in > btrfs_delete_delayed_dir_index(). > > Although that fuzzed image can already be addressed by enhanced > extent-tree error handler, it's still better to hunt down more BUG_ON(). > > This patch will hunt down two BUG_ON()s in > btrfs_delete_delayed_dir_index(): > - One for error from btrfs_delayed_item_reserve_metadata() > Instead of BUG_ON(), we output an error message and free the item. > And return the error. > All callers of this function handles the error by aborting current > trasaction. > > - One for possible EEXIST from __btrfs_add_delayed_deletion_item() > That function can return -EEXIST. > We already have a good enough error message for that, only need to > clean up the reserved metadata space and allocated item. > > To help above cleanup, also modifiy __btrfs_remove_delayed_item() called > in btrfs_release_delayed_item(), to skip unassociated item. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=203253 > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/delayed-inode.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c > index 43fdb2992956..c4946d58f49b 100644 > --- a/fs/btrfs/delayed-inode.c > +++ b/fs/btrfs/delayed-inode.c > @@ -474,6 +474,9 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item) > struct rb_root_cached *root; > struct btrfs_delayed_root *delayed_root; > > + /* Not associated with any delayed_node */ > + if (!delayed_item->delayed_node) > + return; > delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root; > > BUG_ON(!delayed_root); > @@ -1525,7 +1528,13 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans, > * we have reserved enough space when we start a new transaction, > * so reserving metadata failure is impossible. > */ > - BUG_ON(ret); > + if (ret < 0) { > + btrfs_err(trans->fs_info, > +"metadata reserve fail at %s, should have already reserved space, ret=%d", I'd rather avoid using the function name in non-debugging messages, let's use eg "metadata reservation failed for delayed dir item deltion", and printing ret here is not necessary as it will be printed in the transaction abort message. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] btrfs: extent-tree: Make sure we only allocate extents from block groups with the same type 2019-07-16 9:00 [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images Qu Wenruo 2019-07-16 9:00 ` [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() Qu Wenruo @ 2019-07-16 9:00 ` Qu Wenruo 2019-07-16 9:00 ` [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check Qu Wenruo 2019-07-31 15:58 ` [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images David Sterba 3 siblings, 0 replies; 9+ messages in thread From: Qu Wenruo @ 2019-07-16 9:00 UTC (permalink / raw) To: linux-btrfs [BUG] With fuzzed image and MIXED_GROUPS super flag, we can hitthe following BUG_ON(): ------------[ cut here ]------------ kernel BUG at fs/btrfs/delayed-ref.c:491! invalid opcode: 0000 [#1] PREEMPT SMP NOPTI CPU: 0 PID: 1849 Comm: sync Tainted: G O 5.2.0-custom #27 RIP: 0010:update_existing_head_ref.cold+0x44/0x46 [btrfs] Call Trace: add_delayed_ref_head+0x20c/0x2d0 [btrfs] btrfs_add_delayed_tree_ref+0x1fc/0x490 [btrfs] btrfs_free_tree_block+0x123/0x380 [btrfs] __btrfs_cow_block+0x435/0x500 [btrfs] btrfs_cow_block+0x110/0x240 [btrfs] btrfs_search_slot+0x230/0xa00 [btrfs] ? __lock_acquire+0x105e/0x1e20 btrfs_insert_empty_items+0x67/0xc0 [btrfs] alloc_reserved_file_extent+0x9e/0x340 [btrfs] __btrfs_run_delayed_refs+0x78e/0x1240 [btrfs] ? kvm_clock_read+0x18/0x30 ? __sched_clock_gtod_offset+0x21/0x50 btrfs_run_delayed_refs.part.0+0x4e/0x180 [btrfs] btrfs_run_delayed_refs+0x23/0x30 [btrfs] btrfs_commit_transaction+0x53/0x9f0 [btrfs] btrfs_sync_fs+0x7c/0x1c0 [btrfs] ? __ia32_sys_fdatasync+0x20/0x20 sync_fs_one_sb+0x23/0x30 iterate_supers+0x95/0x100 ksys_sync+0x62/0xb0 __ia32_sys_sync+0xe/0x20 do_syscall_64+0x65/0x240 entry_SYSCALL_64_after_hwframe+0x49/0xbe ---[ end trace 4372e814d777d9b9 ]--- [CAUSE] This situation is caused by several factors: - Fuzzed image The extent tree of this fs missed one backref for extent tree root. So we can allocated space from that slot. - MIXED_BG feature Super block has MIXED_BG flag. - No mixed block groups exists All block groups are just regular ones. This makes data space_info->block_groups[] contains metadata block groups. And when we reserve space for data, we can use space in metadata block group. Then we hit the following file operations: - falloc We need to allocate data extents. find_free_extent() choose to use the metadata block to allocate space from, and choose the space of extent tree root, since its backref is missing. This generate one delayed ref head with is_data = 1. - extent tree update We need to update extent tree at run_delayed_ref time. This generate one delayed ref head with is_data = 0, for the same bytenr of old extent tree root. Then we trigger the BUG_ON(). [FIX] The quick fix here is to check block_group->flags before using it. The problem can only happen for MIXED_GROUPS fs. Regular filesystems won't have space_info with DATA|METADATA flag, and no way to hit the bug. Link: https://bugzilla.kernel.org/show_bug.cgi?id=203255 Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent-tree.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 72868d9ac58e..69d6b2e308fc 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -8013,6 +8013,15 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info, */ if ((flags & extra) && !(block_group->flags & extra)) goto loop; + + /* + * This block group has different flags than we what. + * It's possible that we have MIXED_GROUP flag but no + * block groups is mixed. + * Just skip such block group. + */ + btrfs_release_block_group(block_group, delalloc); + continue; } have_block_group: -- 2.22.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check 2019-07-16 9:00 [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images Qu Wenruo 2019-07-16 9:00 ` [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() Qu Wenruo 2019-07-16 9:00 ` [PATCH 2/3] btrfs: extent-tree: Make sure we only allocate extents from block groups with the same type Qu Wenruo @ 2019-07-16 9:00 ` Qu Wenruo 2019-07-26 15:29 ` David Sterba 2019-07-31 15:58 ` [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images David Sterba 3 siblings, 1 reply; 9+ messages in thread From: Qu Wenruo @ 2019-07-16 9:00 UTC (permalink / raw) To: linux-btrfs This patch will introduce ROOT_ITEM check, which includes: - Key->objectid and key->offset check Currently only some easy check, e.g. 0 as rootid is invalid. - Item size check Root item size is fixed. - Generation checks Generation, v2_generaetion and last_snapshot should not pass super generation + 1 - Level and alignment check Level should be in [0, 7], and bytenr must be aligned to sector size. - Flags check Link: https://bugzilla.kernel.org/show_bug.cgi?id=203261 Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 92 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index a4c7f7ed8490..71bbbce3076d 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -810,6 +810,95 @@ static int check_inode_item(struct extent_buffer *leaf, return 0; } +static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key, + int slot) +{ + struct btrfs_fs_info *fs_info = leaf->fs_info; + struct btrfs_root_item ri; + u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY | + BTRFS_ROOT_SUBVOL_DEAD; + + /* No Such Tree id*/ + if (key->objectid == 0) { + generic_err(leaf, slot, "invalid root id 0"); + return -EUCLEAN; + } + + /* + * Some older kernel may create ROOT_ITEM with non-zero offset, + * so here we only check offset for reloc tree whose key->offset + * must be a valid tree. + */ + if (key->objectid == BTRFS_TREE_RELOC_OBJECTID && key->offset == 0) { + generic_err(leaf, slot, "invalid root id 0 for reloc tree"); + return -EUCLEAN; + } + + if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) { + generic_err(leaf, slot, + "invalid root item size, have %u expect %lu", + btrfs_item_size_nr(leaf, slot), sizeof(ri)); + } + + read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot), + sizeof(ri)); + + /* Generateion related */ + if (btrfs_root_generation(&ri) > + btrfs_super_generation(fs_info->super_copy) + 1) { + generic_err(leaf, slot, + "invalid root generaetion, have %llu expect (0, %llu]", + btrfs_root_generation(&ri), + btrfs_super_generation(fs_info->super_copy) + 1); + return -EUCLEAN; + } + if (btrfs_root_generation_v2(&ri) > + btrfs_super_generation(fs_info->super_copy) + 1) { + generic_err(leaf, slot, + "invalid root v2 generaetion, have %llu expect (0, %llu]", + btrfs_root_generation_v2(&ri), + btrfs_super_generation(fs_info->super_copy) + 1); + return -EUCLEAN; + } + if (btrfs_root_last_snapshot(&ri) > + btrfs_super_generation(fs_info->super_copy) + 1) { + generic_err(leaf, slot, + "invalid root last_snapshot, have %llu expect (0, %llu]", + btrfs_root_last_snapshot(&ri), + btrfs_super_generation(fs_info->super_copy) + 1); + return -EUCLEAN; + } + + /* Alignment and level check */ + if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) { + generic_err(leaf, slot, + "invalid root bytenr, have %llu expect to be aligned to %u", + btrfs_root_bytenr(&ri), fs_info->sectorsize); + return -EUCLEAN; + } + if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) { + generic_err(leaf, slot, + "invalid root level, have %u expect [0, %u]", + btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1); + return -EUCLEAN; + } + if (ri.drop_level >= BTRFS_MAX_LEVEL) { + generic_err(leaf, slot, + "invalid root level, have %u expect [0, %u]", + ri.drop_level, BTRFS_MAX_LEVEL - 1); + return -EUCLEAN; + } + + /* Flags check */ + if (btrfs_root_flags(&ri) & ~valid_root_flags) { + generic_err(leaf, slot, + "invalid root flags, have 0x%llx expect mask 0x%llu", + btrfs_root_flags(&ri), valid_root_flags); + return -EUCLEAN; + } + return 0; +} + /* * Common point to switch the item-specific validation. */ @@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf, case BTRFS_INODE_ITEM_KEY: ret = check_inode_item(leaf, key, slot); break; + case BTRFS_ROOT_ITEM_KEY: + ret = check_root_item(leaf, key, slot); + break; } return ret; } -- 2.22.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check 2019-07-16 9:00 ` [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check Qu Wenruo @ 2019-07-26 15:29 ` David Sterba 2019-07-26 23:26 ` Qu Wenruo 0 siblings, 1 reply; 9+ messages in thread From: David Sterba @ 2019-07-26 15:29 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Tue, Jul 16, 2019 at 05:00:34PM +0800, Qu Wenruo wrote: > This patch will introduce ROOT_ITEM check, which includes: > - Key->objectid and key->offset check > Currently only some easy check, e.g. 0 as rootid is invalid. > > - Item size check > Root item size is fixed. > > - Generation checks > Generation, v2_generaetion and last_snapshot should not pass super > generation + 1 > > - Level and alignment check > Level should be in [0, 7], and bytenr must be aligned to sector size. > > - Flags check Nice. I found some small things that I can fix, no need to resend. > Link: https://bugzilla.kernel.org/show_bug.cgi?id=203261 > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 92 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 92 insertions(+) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index a4c7f7ed8490..71bbbce3076d 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -810,6 +810,95 @@ static int check_inode_item(struct extent_buffer *leaf, > return 0; > } > > +static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key, > + int slot) > +{ > + struct btrfs_fs_info *fs_info = leaf->fs_info; > + struct btrfs_root_item ri; > + u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY | > + BTRFS_ROOT_SUBVOL_DEAD; You can use 'const' here. > + > + /* No Such Tree id*/ > + if (key->objectid == 0) { > + generic_err(leaf, slot, "invalid root id 0"); > + return -EUCLEAN; > + } > + > + /* > + * Some older kernel may create ROOT_ITEM with non-zero offset, > + * so here we only check offset for reloc tree whose key->offset > + * must be a valid tree. > + */ > + if (key->objectid == BTRFS_TREE_RELOC_OBJECTID && key->offset == 0) { > + generic_err(leaf, slot, "invalid root id 0 for reloc tree"); > + return -EUCLEAN; > + } > + > + if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) { > + generic_err(leaf, slot, > + "invalid root item size, have %u expect %lu", > + btrfs_item_size_nr(leaf, slot), sizeof(ri)); sizeof needs %zu > + } > + > + read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot), > + sizeof(ri)); > + > + /* Generateion related */ typo here and a few more times below > + if (btrfs_root_generation(&ri) > > + btrfs_super_generation(fs_info->super_copy) + 1) { > + generic_err(leaf, slot, > + "invalid root generaetion, have %llu expect (0, %llu]", > + btrfs_root_generation(&ri), > + btrfs_super_generation(fs_info->super_copy) + 1); > + return -EUCLEAN; > + } > + if (btrfs_root_generation_v2(&ri) > > + btrfs_super_generation(fs_info->super_copy) + 1) { > + generic_err(leaf, slot, > + "invalid root v2 generaetion, have %llu expect (0, %llu]", So (0, %llu] here means that it must be greater than zero, right? I'm not sure that everyone uses the same notation for open/closed notation. > + btrfs_root_generation_v2(&ri), > + btrfs_super_generation(fs_info->super_copy) + 1); > + return -EUCLEAN; > + } > + if (btrfs_root_last_snapshot(&ri) > > + btrfs_super_generation(fs_info->super_copy) + 1) { > + generic_err(leaf, slot, > + "invalid root last_snapshot, have %llu expect (0, %llu]", > + btrfs_root_last_snapshot(&ri), > + btrfs_super_generation(fs_info->super_copy) + 1); > + return -EUCLEAN; > + } > + > + /* Alignment and level check */ > + if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) { > + generic_err(leaf, slot, > + "invalid root bytenr, have %llu expect to be aligned to %u", > + btrfs_root_bytenr(&ri), fs_info->sectorsize); > + return -EUCLEAN; > + } > + if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) { > + generic_err(leaf, slot, > + "invalid root level, have %u expect [0, %u]", > + btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1); > + return -EUCLEAN; > + } > + if (ri.drop_level >= BTRFS_MAX_LEVEL) { > + generic_err(leaf, slot, > + "invalid root level, have %u expect [0, %u]", > + ri.drop_level, BTRFS_MAX_LEVEL - 1); > + return -EUCLEAN; > + } > + > + /* Flags check */ > + if (btrfs_root_flags(&ri) & ~valid_root_flags) { > + generic_err(leaf, slot, > + "invalid root flags, have 0x%llx expect mask 0x%llu", 0x%llx > + btrfs_root_flags(&ri), valid_root_flags); > + return -EUCLEAN; > + } > + return 0; > +} > + > /* > * Common point to switch the item-specific validation. > */ > @@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf, > case BTRFS_INODE_ITEM_KEY: > ret = check_inode_item(leaf, key, slot); > break; > + case BTRFS_ROOT_ITEM_KEY: > + ret = check_root_item(leaf, key, slot); > + break; > } > return ret; > } > -- > 2.22.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check 2019-07-26 15:29 ` David Sterba @ 2019-07-26 23:26 ` Qu Wenruo 2019-07-31 15:34 ` David Sterba 0 siblings, 1 reply; 9+ messages in thread From: Qu Wenruo @ 2019-07-26 23:26 UTC (permalink / raw) To: dsterba, Qu Wenruo, linux-btrfs [-- Attachment #1.1: Type: text/plain, Size: 2994 bytes --] [snip] > >> + } >> + >> + read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot), >> + sizeof(ri)); >> + >> + /* Generateion related */ > > typo here and a few more times below > >> + if (btrfs_root_generation(&ri) > >> + btrfs_super_generation(fs_info->super_copy) + 1) { >> + generic_err(leaf, slot, >> + "invalid root generaetion, have %llu expect (0, %llu]", >> + btrfs_root_generation(&ri), >> + btrfs_super_generation(fs_info->super_copy) + 1); >> + return -EUCLEAN; >> + } >> + if (btrfs_root_generation_v2(&ri) > >> + btrfs_super_generation(fs_info->super_copy) + 1) { >> + generic_err(leaf, slot, >> + "invalid root v2 generaetion, have %llu expect (0, %llu]", > > So (0, %llu] here means that it must be greater than zero, right? I'm > not sure that everyone uses the same notation for open/closed notation. AFAIK in tree checker it's all the same notation. Or any better solution for that? Thanks, Qu > >> + btrfs_root_generation_v2(&ri), >> + btrfs_super_generation(fs_info->super_copy) + 1); >> + return -EUCLEAN; >> + } >> + if (btrfs_root_last_snapshot(&ri) > >> + btrfs_super_generation(fs_info->super_copy) + 1) { >> + generic_err(leaf, slot, >> + "invalid root last_snapshot, have %llu expect (0, %llu]", >> + btrfs_root_last_snapshot(&ri), >> + btrfs_super_generation(fs_info->super_copy) + 1); >> + return -EUCLEAN; >> + } >> + >> + /* Alignment and level check */ >> + if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) { >> + generic_err(leaf, slot, >> + "invalid root bytenr, have %llu expect to be aligned to %u", >> + btrfs_root_bytenr(&ri), fs_info->sectorsize); >> + return -EUCLEAN; >> + } >> + if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) { >> + generic_err(leaf, slot, >> + "invalid root level, have %u expect [0, %u]", >> + btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1); >> + return -EUCLEAN; >> + } >> + if (ri.drop_level >= BTRFS_MAX_LEVEL) { >> + generic_err(leaf, slot, >> + "invalid root level, have %u expect [0, %u]", >> + ri.drop_level, BTRFS_MAX_LEVEL - 1); >> + return -EUCLEAN; >> + } >> + >> + /* Flags check */ >> + if (btrfs_root_flags(&ri) & ~valid_root_flags) { >> + generic_err(leaf, slot, >> + "invalid root flags, have 0x%llx expect mask 0x%llu", > > 0x%llx > >> + btrfs_root_flags(&ri), valid_root_flags); >> + return -EUCLEAN; >> + } >> + return 0; >> +} >> + >> /* >> * Common point to switch the item-specific validation. >> */ >> @@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf, >> case BTRFS_INODE_ITEM_KEY: >> ret = check_inode_item(leaf, key, slot); >> break; >> + case BTRFS_ROOT_ITEM_KEY: >> + ret = check_root_item(leaf, key, slot); >> + break; >> } >> return ret; >> } >> -- >> 2.22.0 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check 2019-07-26 23:26 ` Qu Wenruo @ 2019-07-31 15:34 ` David Sterba 0 siblings, 0 replies; 9+ messages in thread From: David Sterba @ 2019-07-31 15:34 UTC (permalink / raw) To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, linux-btrfs On Sat, Jul 27, 2019 at 07:26:28AM +0800, Qu Wenruo wrote: > [snip] > > > >> + } > >> + > >> + read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot), > >> + sizeof(ri)); > >> + > >> + /* Generateion related */ > > > > typo here and a few more times below > > > >> + if (btrfs_root_generation(&ri) > > >> + btrfs_super_generation(fs_info->super_copy) + 1) { > >> + generic_err(leaf, slot, > >> + "invalid root generaetion, have %llu expect (0, %llu]", > >> + btrfs_root_generation(&ri), > >> + btrfs_super_generation(fs_info->super_copy) + 1); > >> + return -EUCLEAN; > >> + } > >> + if (btrfs_root_generation_v2(&ri) > > >> + btrfs_super_generation(fs_info->super_copy) + 1) { > >> + generic_err(leaf, slot, > >> + "invalid root v2 generaetion, have %llu expect (0, %llu]", > > > > So (0, %llu] here means that it must be greater than zero, right? I'm > > not sure that everyone uses the same notation for open/closed notation. > > AFAIK in tree checker it's all the same notation. > > Or any better solution for that? No this one is fine, let's use it and eventualy update comments or messages where this notation is not used. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images 2019-07-16 9:00 [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images Qu Wenruo ` (2 preceding siblings ...) 2019-07-16 9:00 ` [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check Qu Wenruo @ 2019-07-31 15:58 ` David Sterba 3 siblings, 0 replies; 9+ messages in thread From: David Sterba @ 2019-07-31 15:58 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs, Jungyeon Yoon On Tue, Jul 16, 2019 at 05:00:31PM +0800, Qu Wenruo wrote: > This wave has the following features: > - Hunt down BUG_ON() in btrfs_delete_delayed_dir_index() > EEXIST can cause BUG_ON(). And all callers of this function has > already handled error by aborting transacation. > > - Only allocate extents from the same block group type > This is a very tricky bug, needs MIXED_GROUP super flag with regular > block groups (separate META and DATA) and corrupted extent tree. > > - ROOT_ITEM check for tree checker > This kills the unaligned bytenr, invalid level and incorrect reloc > tree. > > Reported-by: Jungyeon Yoon <jungyeon.yoon@gmail.com> > > Qu Wenruo (3): > btrfs: delayed-inode: Kill the BUG_ON() in > btrfs_delete_delayed_dir_index() > btrfs: extent-tree: Make sure we only allocate extents from block > groups with the same type > btrfs: tree-checker: Add ROOT_ITEM check Added to misc-next, with some minor updates, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-31 15:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-16 9:00 [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images Qu Wenruo 2019-07-16 9:00 ` [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index() Qu Wenruo 2019-07-31 15:44 ` David Sterba 2019-07-16 9:00 ` [PATCH 2/3] btrfs: extent-tree: Make sure we only allocate extents from block groups with the same type Qu Wenruo 2019-07-16 9:00 ` [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check Qu Wenruo 2019-07-26 15:29 ` David Sterba 2019-07-26 23:26 ` Qu Wenruo 2019-07-31 15:34 ` David Sterba 2019-07-31 15:58 ` [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).