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@xxxxxxxxxxxxxxx # 6.7+
> > > Fixes: c962098ca4af ("btrfs: fix incorrect splitting in btrfs_drop_extent_map_range")
> > > Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> >
> > $ git describe --contains c962098ca4af
> > v6.5-rc7~4^2
> >
> > so it should be
> > CC: stable@xxxxxxxxxxxxxxx # 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.
