Re: [PATCH] btrfs: fix the length of reserved qgroup to free

[hs wang]

Boris Burkov <boris@xxxxxx> 于2024年10月9日周三 00:12写道：
>
> On Tue, Oct 08, 2024 at 02:48:46PM +0800, iamhswang@xxxxxxxxx wrote:
> > From: Haisu Wang <haisuwang@xxxxxxxxxxx>
> >
> > The dealloc flag may be cleared and the extent won't reach the disk
> > in cow_file_range when errors path. The reserved qgroup space is
> > freed in commit 30479f31d44d ("btrfs: fix qgroup reserve leaks in
> > cow_file_range"). However, the length of untouched region to free
> > need to be adjusted with the region size.
> >
> > Fixes: 30479f31d44d ("btrfs: fix qgroup reserve leaks in cow_file_range")
> > Signed-off-by: Haisu Wang <haisuwang@xxxxxxxxxxx>
>
> Good catch and fix, thank you!
> Reviewed-by: Boris Burkov <boris@xxxxxx>
>
Thanks for the review.

> Can you please share more information about how you reproduced and
> tested this issue for the fix? In one of the other emails in the chain,
> you also mentioned a CVE, so explaining the specific impact of the bug
> is helpful too.
>

Instead of hitting this in the real world, I get this while backporting the
CVE-2024-46733 fixes. I need to understand the full story and the extent
reservation/clean up context, found the free data region mismatch to the
dealloc region and the potential risky. So i write the fix of the inconsistent
size.

> As far as I can tell, we risk freeing too much space past the real
> desired range if start gets bumped before this free, which could lead to
> prematurely freeing some other rsv marked data past end. This naturally
> leads to incorrect accounting, And I think would allow us to reserve
> this same range again. Though perhaps delalloc extent range stuff would
> prevent that. Between that, and the changesets gating most of the qgroup
> freeing, it's hard to actually see what happens :)
>
> Long ramble short: do you have a reproducer?
>

Sadly, i don't have a reproducer yet.

In another mail of the chain, Wenruo suggested it is possible to
polish the usage
of @startand @extent_reserved to make it more clear/safe. I will check more to
finish this in another patch, together with generic fstest at least.

Thanks,
Haisu Wang

> > ---
> >  fs/btrfs/inode.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index b0ad46b734c3..5eefa2318fa8 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -1592,7 +1592,7 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> >               clear_bits |= EXTENT_CLEAR_DATA_RESV;
> >               extent_clear_unlock_delalloc(inode, start, end, locked_folio,
> >                                            &cached, clear_bits, page_ops);
> > -             btrfs_qgroup_free_data(inode, NULL, start, cur_alloc_size, NULL);
> > +             btrfs_qgroup_free_data(inode, NULL, start, end - start + 1, NULL);
> >       }
> >       return ret;
> >  }
> > --
> > 2.39.3
> >