6.6-stable review patch. If anyone has any objections, please let me know.
------------------
From: Tavian Barnes tavianator@tavianator.com
commit ef1e68236b9153c27cb7cf29ead0c532870d4215 upstream.
There are reports from tree-checker that detects corrupted nodes, without any obvious pattern so possibly an overwrite in memory. After some debugging it turns out there's a race when reading an extent buffer the uptodate status can be missed.
To prevent concurrent reads for the same extent buffer, read_extent_buffer_pages() performs these checks:
/* (1) */ if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags)) return 0;
/* (2) */ if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags)) goto done;
At this point, it seems safe to start the actual read operation. Once that completes, end_bbio_meta_read() does
/* (3) */ set_extent_buffer_uptodate(eb);
/* (4) */ clear_bit(EXTENT_BUFFER_READING, &eb->bflags);
Normally, this is enough to ensure only one read happens, and all other callers wait for it to finish before returning. Unfortunately, there is a racey interleaving:
Thread A | Thread B | Thread C ---------+----------+--------- (1) | | | (1) | (2) | | (3) | | (4) | | | (2) | | | (1)
When this happens, thread B kicks of an unnecessary read. Worse, thread C will see UPTODATE set and return immediately, while the read from thread B is still in progress. This race could result in tree-checker errors like this as the extent buffer is concurrently modified:
BTRFS critical (device dm-0): corrupted node, root=256 block=8550954455682405139 owner mismatch, have 11858205567642294356 expect [256, 18446744073709551360]
Fix it by testing UPTODATE again after setting the READING bit, and if it's been set, skip the unnecessary read.
Fixes: d7172f52e993 ("btrfs: use per-buffer locking for extent_buffer reading") Link: https://lore.kernel.org/linux-btrfs/CAHk-=whNdMaN9ntZ47XRKP6DBes2E5w7fi-0U3H... Link: https://lore.kernel.org/linux-btrfs/f51a6d5d7432455a6a858d51b49ecac183e0bbc9... Link: https://lore.kernel.org/linux-btrfs/c7241ea4-fcc6-48d2-98c8-b5ea790d6c89@gmx... CC: stable@vger.kernel.org # 6.5+ Reviewed-by: Qu Wenruo wqu@suse.com Reviewed-by: Christoph Hellwig hch@lst.de Signed-off-by: Tavian Barnes tavianator@tavianator.com Reviewed-by: David Sterba dsterba@suse.com [ minor update of changelog ] Signed-off-by: David Sterba dsterba@suse.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- fs/btrfs/extent_io.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
--- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -4047,6 +4047,19 @@ int read_extent_buffer_pages(struct exte if (test_and_set_bit(EXTENT_BUFFER_READING, &eb->bflags)) goto done;
+ /* + * Between the initial test_bit(EXTENT_BUFFER_UPTODATE) and the above + * test_and_set_bit(EXTENT_BUFFER_READING), someone else could have + * started and finished reading the same eb. In this case, UPTODATE + * will now be set, and we shouldn't read it in again. + */ + if (unlikely(test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))) { + clear_bit(EXTENT_BUFFER_READING, &eb->bflags); + smp_mb__after_atomic(); + wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING); + return 0; + } + clear_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags); eb->read_mirror = 0; check_buffer_tree_ref(eb);