From: David Sterba <dsterba@suse.cz>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Wang Yugui <wangyugui@e16-tech.com>, Qu Wenruo <wqu@suse.com>,
linux-btrfs@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] btrfs: fix wrong block_start calculation for btrfs_drop_extent_map_range()
Date: Tue, 9 Apr 2024 14:36:28 +0200 [thread overview]
Message-ID: <20240409123628.GD3492@twin.jikos.cz> (raw)
In-Reply-To: <2024040851-uptown-splashing-951c@gregkh>
On Mon, Apr 08, 2024 at 06:57:56AM +0200, Greg KH wrote:
> On Mon, Apr 08, 2024 at 08:00:15AM +0800, Wang Yugui wrote:
> > Hi,
> >
> > > [BUG]
> > > During my extent_map cleanup/refactor, with more than too strict sanity
> > > checks, extent-map-tests::test_case_7() would crash my extent_map sanity
> > > checks.
> > >
> > > The problem is, after btrfs_drop_extent_map_range(), the resulted
> > > extent_map has a @block_start way too large.
> > > Meanwhile my btrfs_file_extent_item based members are returning a
> > > correct @disk_bytenr along with correct @offset.
> > >
> > > The extent map layout looks like this:
> > >
> > > 0 16K 32K 48K
> > > | PINNED | | Regular |
> > >
> > > The regular em at [32K, 48K) also has 32K @block_start.
> > >
> > > Then drop range [0, 36K), which should shrink the regular one to be
> > > [36K, 48K).
> > > However the @block_start is incorrect, we expect 32K + 4K, but got 52K.
> > >
> > > [CAUSE]
> > > Inside btrfs_drop_extent_map_range() function, if we hit an extent_map
> > > that covers the target range but is still beyond it, we need to split
> > > that extent map into half:
> > >
> > > |<-- drop range -->|
> > > |<----- existing extent_map --->|
> > >
> > > And if the extent map is not compressed, we need to forward
> > > extent_map::block_start by the difference between the end of drop range
> > > and the extent map start.
> > >
> > > However in that particular case, the difference is calculated using
> > > (start + len - em->start).
> > >
> > > The problem is @start can be modified if the drop range covers any
> > > pinned extent.
> > >
> > > This leads to wrong calculation, and would be caught by my later
> > > extent_map sanity checks, which checks the em::block_start against
> > > btrfs_file_extent_item::disk_bytenr + btrfs_file_extent_item::offset.
> > >
> > > And unfortunately this is going to cause data corruption, as the
> > > splitted em is pointing an incorrect location, can cause either
> > > unexpected read error or wild writes.
> > >
> > > [FIX]
> > > Fix it by avoiding using @start completely, and use @end - em->start
> > > instead, which @end is exclusive bytenr number.
> > >
> > > And update the test case to verify the @block_start to prevent such
> > > problem from happening.
> > >
> > > CC: stable@vger.kernel.org # 6.7+
> > > Fixes: c962098ca4af ("btrfs: fix incorrect splitting in btrfs_drop_extent_map_range")
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> >
> > $ git describe --contains c962098ca4af
> > v6.5-rc7~4^2
> >
> > so it should be
> > CC: stable@vger.kernel.org # 6.5+
>
> As the "Fixes:" commit was backported to the following kernel releases:
> 6.1.47 6.4.12
> it should go back to 6.1+ as well :)
Determining all the versions requires one extra step to scan the
stable-queue.git for the commit. I think everybody does 'git describe
--contains COMMITID' based on Fixes: and then pick the version for CC:.
This is "best" we can promise for an average developer.
> But we can handle that when it hits Linus's tree.
I think it's easier for you to queue the patch to other versions at the
time you pick it from mails or Linus' tree, but from what I've seen so
far this is how it works.
next prev parent reply other threads:[~2024-04-09 12:43 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-07 1:18 [PATCH] btrfs: fix wrong block_start calculation for btrfs_drop_extent_map_range() Qu Wenruo
2024-04-08 0:00 ` Wang Yugui
2024-04-08 4:57 ` Greg KH
2024-04-09 12:36 ` David Sterba [this message]
2024-04-08 10:48 ` Filipe Manana
2024-04-08 21:53 ` Qu Wenruo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240409123628.GD3492@twin.jikos.cz \
--to=dsterba@suse.cz \
--cc=gregkh@linuxfoundation.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=wangyugui@e16-tech.com \
--cc=wqu@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).