* [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use
@ 2018-07-17 7:46 Qu Wenruo
2018-07-17 8:01 ` Nikolay Borisov
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2018-07-17 7:46 UTC (permalink / raw)
To: linux-btrfs
[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.xu@gatech.edu>
Signed-off-by: Qu Wenruo <wqu@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;
+ }
+
buf = btrfs_init_new_buffer(trans, root, ins.objectid, level);
if (IS_ERR(buf)) {
ret = PTR_ERR(buf);
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use
2018-07-17 7:46 [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use Qu Wenruo
@ 2018-07-17 8:01 ` Nikolay Borisov
2018-07-17 8:11 ` Su Yue
2018-07-17 8:24 ` Qu Wenruo
0 siblings, 2 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-07-17 8:01 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
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.xu@gatech.edu>
> Signed-off-by: Qu Wenruo <wqu@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.
> +
> buf = btrfs_init_new_buffer(trans, root, ins.objectid, level);
> if (IS_ERR(buf)) {
> ret = PTR_ERR(buf);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use
2018-07-17 8:01 ` Nikolay Borisov
@ 2018-07-17 8:11 ` Su Yue
2018-07-17 8:24 ` Qu Wenruo
1 sibling, 0 replies; 6+ messages in thread
From: Su Yue @ 2018-07-17 8:11 UTC (permalink / raw)
To: Nikolay Borisov, Qu Wenruo, linux-btrfs
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.xu@gatech.edu>
>> Signed-off-by: Qu Wenruo <wqu@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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use
2018-07-17 8:01 ` Nikolay Borisov
2018-07-17 8:11 ` Su Yue
@ 2018-07-17 8:24 ` Qu Wenruo
2018-07-17 8:28 ` Nikolay Borisov
1 sibling, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2018-07-17 8:24 UTC (permalink / raw)
To: Nikolay Borisov, Qu Wenruo, linux-btrfs
On 2018年07月17日 16:01, 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.xu@gatech.edu>
>> Signed-off-by: Qu Wenruo <wqu@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.
And it's causing problem for certain test cases.
Please ignore this (at least for now).
But on the other hand, we indeed have a lot of reports on corrupted
extent tree, it's possible to hit some corrupted extent tree (Su is
already exhausted by the corrupted tree reported by Marc)
So I'm not completely fine with current extent tree error handling.
I'll try to find some balance in next version.
Thanks,
Qu
>
>> +
>> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use
2018-07-17 8:24 ` Qu Wenruo
@ 2018-07-17 8:28 ` Nikolay Borisov
2018-07-17 8:33 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-07-17 8:28 UTC (permalink / raw)
To: Qu Wenruo, Qu Wenruo, linux-btrfs
On 17.07.2018 11:24, Qu Wenruo wrote:
> And it's causing problem for certain test cases.
> Please ignore this (at least for now).
>
> But on the other hand, we indeed have a lot of reports on corrupted
> extent tree, it's possible to hit some corrupted extent tree (Su is
> already exhausted by the corrupted tree reported by Marc)
>
> So I'm not completely fine with current extent tree error handling.
> I'll try to find some balance in next version.
I agree we need a better OVERALL error handling/detection. Your
tree-checker work IMO is a step in the right direction. What I want is
to prevent ad-hoc checks being sprinkled in the code. Sorry, but that's
not fine. The thing with working on a lot of corruption reports is the
fact each one of them is looked at in isolation so it produces isolated
fixes. Whereas if a step back is taken and the overall error
handling/detection is considered it might turn out a whole class of
corruption could be detected by a single change, otherwise checks upon
checks will be added which just add technical debt.
Considering this, I'm more in favor of extending the tree-checker to be
the central place where errors are detected (of course this is easier
said than done).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use
2018-07-17 8:28 ` Nikolay Borisov
@ 2018-07-17 8:33 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2018-07-17 8:33 UTC (permalink / raw)
To: Nikolay Borisov, Qu Wenruo, linux-btrfs
On 2018年07月17日 16:28, Nikolay Borisov wrote:
>
>
> On 17.07.2018 11:24, Qu Wenruo wrote:
>> And it's causing problem for certain test cases.
>> Please ignore this (at least for now).
>>
>> But on the other hand, we indeed have a lot of reports on corrupted
>> extent tree, it's possible to hit some corrupted extent tree (Su is
>> already exhausted by the corrupted tree reported by Marc)
>>
>> So I'm not completely fine with current extent tree error handling.
>> I'll try to find some balance in next version.
>
>
> I agree we need a better OVERALL error handling/detection. Your
> tree-checker work IMO is a step in the right direction. What I want is
> to prevent ad-hoc checks being sprinkled in the code.
Yep, I also don't like what I'm doing.
But the problem and the limit of tree-checker is, it's static check.
For things doing tons of cross-check like extent tree, it's not really
as useful.
> Sorry, but that's
> not fine. The thing with working on a lot of corruption reports is the
> fact each one of them is looked at in isolation so it produces isolated
> fixes. Whereas if a step back is taken and the overall error
> handling/detection is considered it might turn out a whole class of
> corruption could be detected by a single change, otherwise checks upon
> checks will be added which just add technical debt.
>
> Considering this, I'm more in favor of extending the tree-checker to be
> the central place where errors are detected (of course this is easier
> said than done).
For this report itself, tree checker can detect it indirectly by reject
the leaf for the unknown key type.
But one can easily create a valid image by just removing the valid
METADATA_ITEM, and tree-checker can't do anything to detect the problem.
So unfortunately, we will eventually need some runtime check anyway.
Thanks,
Qu
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-17 9:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-17 7:46 [PATCH] btrfs: extent-tree: Check if the newly reserved tree block is already in use Qu Wenruo
2018-07-17 8:01 ` Nikolay Borisov
2018-07-17 8:11 ` Su Yue
2018-07-17 8:24 ` Qu Wenruo
2018-07-17 8:28 ` Nikolay Borisov
2018-07-17 8:33 ` Qu Wenruo
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).