On 07/17/2018 04:01 PM, Nikolay Borisov wrote:
On 17.07.2018 10:46, Qu Wenruo wrote:[BUG] For certain fuzzed btrfs image, if we create any csum data, it would cause the following kernel warning and deadlock when trying to update csum tree: ------ [ 278.113360] WARNING: CPU: 1 PID: 41 at fs/btrfs/locking.c:230 btrfs_tree_lock+0x3e2/0x400 [ 278.113737] CPU: 1 PID: 41 Comm: kworker/u4:1 Not tainted 4.18.0-rc1+ #8 [ 278.113745] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 [ 278.113753] Workqueue: btrfs-endio-write btrfs_endio_write_helper [ 278.113761] RIP: 0010:btrfs_tree_lock+0x3e2/0x400 [ 278.113762] Code: 00 48 c7 40 08 00 00 00 00 48 8b 45 d0 65 48 33 04 25 28 00 00 00 75 20 48 81 c4 a0 00 00 00 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e9 d4 fc ff ff 0f 0b e9 61 ff ff ff e8 ab f4 87 ff 90 66 2e [ 278.113818] RSP: 0018:ffff8801f407f488 EFLAGS: 00010246 [ 278.113865] Call Trace: [ 278.113936] btrfs_alloc_tree_block+0x39f/0x770 [ 278.113988] __btrfs_cow_block+0x285/0x9e0 [ 278.114029] btrfs_cow_block+0x191/0x2e0 [ 278.114035] btrfs_search_slot+0x492/0x1160 [ 278.114146] btrfs_lookup_csum+0xec/0x280 [ 278.114182] btrfs_csum_file_blocks+0x2be/0xa60 [ 278.114232] add_pending_csums+0xaf/0xf0 [ 278.114238] btrfs_finish_ordered_io+0x74b/0xc90 [ 278.114281] finish_ordered_fn+0x15/0x20 [ 278.114285] normal_work_helper+0xf6/0x500 [ 278.114305] btrfs_endio_write_helper+0x12/0x20 [ 278.114310] process_one_work+0x302/0x770 [ 278.114315] worker_thread+0x81/0x6d0 [ 278.114321] kthread+0x180/0x1d0 [ 278.114334] ret_from_fork+0x35/0x40 [ 278.114339] ---[ end trace 2e85051acb5f6dc1 ]--- ------ [CAUSE] The fuzzed image has corrupted EXTENT_ITEM for csum tree root: ------ extent tree key (EXTENT_TREE ROOT_ITEM 0) item 4 key (29364224 METADATA_ITEM 0) itemoff 3857 itemsize 33 refs 1 gen 6 flags TREE_BLOCK tree block skinny level 0 tree block backref root UUID_TREE item 5 key (29376512 UNKNOWN.0 0) itemoff 3824 itemsize 33 ^^^^^^^^^^^^^^^^^^^^ Corrupted METADATA_ITEM item 6 key (29380608 METADATA_ITEM 0) itemoff 3791 itemsize 33 refs 1 gen 4 flags TREE_BLOCK tree block skinny level 0 tree block backref root DATA_RELOC_TREE checksum tree key (CSUM_TREE ROOT_ITEM 0) leaf 29376512 items 0 free space 3995 generation 4 owner CSUM_TREE ^^^^^^^^ bytenr matches above item. ------ So when btrfs_alloc_tree_blocks() calls btrfs_reserve_extent(), since there is not METADATA_ITEM/EXTENT_ITEM for bytenr 29376512, btrfs thinks it's free space, and reserve it. However in fact it's already been used by csum tree, and later btrfs_init_new_buffer() will try to call btrfs_tree_lock(), whose WARN_ON() detects lock nest on the same extent buffer. Finally the wait_event() on the eb->read/write_lock_wq will never exit since we're holding the lock by ourselves and deadlock. [FIX] The fix here is to ensure at least the reserved extent buffer is not cached. Any used extent buffer should be cached in the global radix tree (fs_info->buffer_radix). So before calling btrfs_init_new_buffer() in btrfs_alloc_tree_block(), we call find_extent_buffer() explicitly to verify it's not used by ourselves. Please note this is just a basic check, it is not and will never be as good as btrfs check on detecting extent tree corruption, but at least we won't dead lock so easily. Link: https://bugzilla.kernel.org/show_bug.cgi?id=200405 Reported-by: Xu Wen <wen...@gatech.edu> Signed-off-by: Qu Wenruo <w...@suse.com> --- fs/btrfs/extent-tree.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 3578fa5b30ef..782dd96b7c5e 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -8435,6 +8435,20 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans, if (ret) goto out_unuse;+ /*+ * Newly allocated tree block should never be cached in radix tree, + * Or we have a corrupted extent tree. + */ + buf = find_extent_buffer(fs_info, ins.objectid); + if (buf) { + btrfs_err_rl(fs_info, + "tree block %llu is already in use, extent tree may be corrupted", + ins.objectid); + ret = -EUCLEAN; + free_extent_buffer(buf); + goto out_unuse; + }The code makes sense but I have the feeling it needs to have some sort of assert guard because this check will likely trigger only on severly corrupted filesystemd and yet we introduce it for everyone.
Agreed, the function is called so frequently.
+ buf = btrfs_init_new_buffer(trans, root, ins.objectid, level); if (IS_ERR(buf)) { ret = PTR_ERR(buf);-- 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
