On 2018年08月01日 18:08, Nikolay Borisov wrote:
>
>
> On 1.08.2018 11:08, Qu Wenruo wrote:
>> [BUG]
>> When mounting certain crafted image, btrfs will trigger kernel BUG_ON()
>> when try to recover balance:
>> ------
>> ------------[ cut here ]------------
>> kernel BUG at fs/btrfs/extent-tree.c:8956!
>> invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
>> CPU: 1 PID: 662 Comm: mount Not tainted 4.18.0-rc1-custom+ #10
>> RIP: 0010:walk_up_proc+0x336/0x480 [btrfs]
>> RSP: 0018:ffffb53540c9b890 EFLAGS: 00010202
>> Call Trace:
>> walk_up_tree+0x172/0x1f0 [btrfs]
>> btrfs_drop_snapshot+0x3a4/0x830 [btrfs]
>> merge_reloc_roots+0xe1/0x1d0 [btrfs]
>> btrfs_recover_relocation+0x3ea/0x420 [btrfs]
>> open_ctree+0x1af3/0x1dd0 [btrfs]
>> btrfs_mount_root+0x66b/0x740 [btrfs]
>> mount_fs+0x3b/0x16a
>> vfs_kern_mount.part.9+0x54/0x140
>> btrfs_mount+0x16d/0x890 [btrfs]
>> mount_fs+0x3b/0x16a
>> vfs_kern_mount.part.9+0x54/0x140
>> do_mount+0x1fd/0xda0
>> ksys_mount+0xba/0xd0
>> __x64_sys_mount+0x21/0x30
>> do_syscall_64+0x60/0x210
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> ---[ end trace d4344e4deee03435 ]---
>> ------
>>
>> [CAUSE]
>> Another extent tree corruption.
>>
>> In this particular case, tree reloc root's owner is
>> DATA_RELOC_TREE (should be TREE_RELOC_TREE), thus its backref is
>> corrupted and we failed the owner check in walk_up_tree().
>>
>> [FIX]
>> It's pretty hard to take care of every extent tree corruption, but at
>> least we can remove such BUG_ON() and exit more gracefully.
>>
>> And since in this particular image, DATA_RELOC_TREE and TREE_RELOC_TREE
>> shares the same root (which is obviously invalid), we needs to make
>> __del_reloc_root() more robust to detect such invalid share to avoid
>> possible NULL dereference as root->node can be NULL in this case.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200411
>> Reported-by: Xu Wen <wen...@gatech.edu>
>> Signed-off-by: Qu Wenruo <w...@suse.com>
>> ---
>> As always, the patch is also pushed to my github repo, along with other
>> fuzzed images related fixes:
>> https://github.com/adam900710/linux/tree/tree_checker_enhance
>> (BTW, is it correct to indicate a branch like above?)
>> ---
>> fs/btrfs/extent-tree.c | 27 +++++++++++++++++++--------
>> fs/btrfs/relocation.c | 2 +-
>> 2 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index da615ebc072e..5f4ca61348b5 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -8949,17 +8949,26 @@ static noinline int walk_up_proc(struct
>> btrfs_trans_handle *trans,
>> }
>>
>> if (eb == root->node) {
>> - if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
>> + if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
>> parent = eb->start;
>> - else
>> - BUG_ON(root->root_key.objectid !=
>> - btrfs_header_owner(eb));
>> + } else if (root->root_key.objectid != btrfs_header_owner(eb)) {
>> + btrfs_err_rl(fs_info,
>> + "unexpected tree owner, have %llu expect %llu",
>> + btrfs_header_owner(eb),
>> + root->root_key.objectid);
>> + return -EINVAL;
>
> EINVAL or ECLEANUP?
Yep, also my concern here.
I have no bias here, and both makes its sense here.
EUCLEAN means it's something unexpected, but normally it's used in
static check, no sure if it suits for runtime check.
Although EINVAL looks more suitable for runtime error, it is not a
perfect errno either, as it's not something invalid from user, but the
fs has something unexpected.
I'm all ears on this errno issue.
Thanks,
Qu
>
>> + }
>> } else {
>> - if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
>> + if (wc->flags[level + 1] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
>> parent = path->nodes[level + 1]->start;
>> - else
>> - BUG_ON(root->root_key.objectid !=
>> - btrfs_header_owner(path->nodes[level + 1]));
>> + } else if (root->root_key.objectid !=
>> + btrfs_header_owner(path->nodes[level + 1])) {
>> + btrfs_err_rl(fs_info,
>> + "unexpected tree owner, have %llu expect %llu",
>> + btrfs_header_owner(eb),
>> + root->root_key.objectid);
>> + return -EINVAL;
> ditto
>> + }
>> }
>>
>> btrfs_free_tree_block(trans, root, eb, parent, wc->refs[level] == 1);
>> @@ -9020,6 +9029,8 @@ static noinline int walk_up_tree(struct
>> btrfs_trans_handle *trans,
>> ret = walk_up_proc(trans, root, path, wc);
>> if (ret > 0)
>> return 0;
>> + if (ret < 0)
>> + return ret;
>>
>> if (path->locks[level]) {
>> btrfs_tree_unlock_rw(path->nodes[level],
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index a2fc0bd83a40..c64051d33d05 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -1321,7 +1321,7 @@ static void __del_reloc_root(struct btrfs_root *root)
>> struct mapping_node *node = NULL;
>> struct reloc_control *rc = fs_info->reloc_ctl;
>>
>> - if (rc) {
>> + if (rc && root->node) {
>> spin_lock(&rc->reloc_root_tree.lock);
>> rb_node = tree_search(&rc->reloc_root_tree.rb_root,
>> root->node->start);
>>
> --
> 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
