Received-SPF: pass (google.com: domain of linux-kernel+bounces-117213-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 25 Mar 2024 15:04:52 +0100
From: David Sterba <dsterba@suse.cz>
To: Tavian Barnes <tavianator@tavianator.com>
Cc: linux-btrfs@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Qu Wenruo <wqu@suse.com>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>, David Sterba <dsterba@suse.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] btrfs: Fix race in read_extent_buffer_pages()
Message-ID: <20240325140452.GM14596@suse.cz>
Reply-To: dsterba@suse.cz
References: <1ca6e688950ee82b1526bb3098852af99b75e6ba.1710551459.git.tavianator@tavianator.com>
 <20240322192108.GK14596@twin.jikos.cz>
 <CABg4E-n=iWjM3K0dvrrwQ9BOEiqTawJ4YUVL6RPWaq2DT2AKvQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CABg4E-n=iWjM3K0dvrrwQ9BOEiqTawJ4YUVL6RPWaq2DT2AKvQ@mail.gmail.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)

On Fri, Mar 22, 2024 at 06:50:07PM -0400, Tavian Barnes wrote:
> On Fri, Mar 22, 2024 at 3:28 PM David Sterba <dsterba@suse.cz> wrote:
> > 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@tavianator.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.
> 
> Thank you!
> 
> Sorry to nitpick, but the paragraph you added to the commit message
> [1] has a typo:
> 
> > 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 is can be missed.
> 
> s/is can/can/

I will fix that, thanks.