linux-kernel - Re: [PATCH] btrfs: Fix race in read_extent_buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 22 Mar 2024 20:21:08 +0100
From: David Sterba <dsterba@...e.cz>
To: Tavian Barnes <tavianator@...ianator.com>
Cc: linux-btrfs@...r.kernel.org, Christoph Hellwig <hch@....de>,
	Qu Wenruo <wqu@...e.com>, Chris Mason <clm@...com>,
	Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: Fix race in read_extent_buffer_pages()

On Fri, Mar 15, 2024 at 09:14:29PM -0400, Tavian Barnes wrote:
> 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-0U3H2+PS18p+Pzw@mail.gmail.com/
> Link: https://lore.kernel.org/linux-btrfs/f51a6d5d7432455a6a858d51b49ecac183e0bbc9.1706312914.git.wqu@suse.com/
> Link: https://lore.kernel.org/linux-btrfs/c7241ea4-fcc6-48d2-98c8-b5ea790d6c89@gmx.com/
> Signed-off-by: Tavian Barnes <tavianator@...ianator.com>

Thank you very much for taking the time to debug the issue and for the
fix. It is a rare occurrence that a tough bug is followed by a fix from
the same person (outside of the developer group) and is certainly
appreciated.