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

[hs wang]

Qu Wenruo <quwenruo.btrfs@xxxxxxx> 于2024年10月8日周二 15:56写道：
>
>
>
> 在 2024/10/8 17:18, iamhswang@xxxxxxxxx 写道:
> > 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>
>
> Right, just several lines before that, we increased @start by
> @cur_alloc_size if @extent_reserved is true.
>
> So we can not directly use the old range size.

Thanks for the review.

>
> You can improve that one step further by not modifying @start just for
> the error handling path, although that should be another patch.

Indeed, modify the start value based on @extent_reserved in
error path only is tricky and ambiguous.

I agree to keep the fix as simple as possible (like the previous patch),
since commit 30479f31d44d ("btrfs: fix qgroupreserve leaks in
cow_file_range") assigned to CVE-2024-46733 already.
A simple fix is easier to port to stable branch of different versions.
Also the possible change to keep @start is more like an
enhancement instead of a fix.

>
> Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
>
> Thanks,
> Qu

To make sure we are on the same page of keeping the @start
unchanged. I write a POC below for your opinion.
(Anyway, i will think/test again before convert POC to a PATCH.)

The @start will advanced in every succeed reservation, the
@cur_alloc_size can represent the @extent_reserved state
instead of using a standalone @extent_reserved flag.
In this case, the @start region no longer need to be modified
based on @extent_reserved state in the error path.

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5eefa2318fa8..0c35292550bd 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1341,7 +1341,6 @@ static noinline int cow_file_range(struct
btrfs_inode *inode,
        struct extent_map *em;
        unsigned clear_bits;
        unsigned long page_ops;
-       bool extent_reserved = false;
        int ret = 0;

        if (btrfs_is_free_space_inode(inode)) {
@@ -1395,8 +1394,7 @@ static noinline int cow_file_range(struct
btrfs_inode *inode,
                struct btrfs_ordered_extent *ordered;
                struct btrfs_file_extent file_extent;

-               cur_alloc_size = num_bytes;
-               ret = btrfs_reserve_extent(root, cur_alloc_size, cur_alloc_size,
+               ret = btrfs_reserve_extent(root, num_bytes, num_bytes,
                                           min_alloc_size, 0, alloc_hint,
                                           &ins, 1, 1);
                if (ret == -EAGAIN) {
@@ -1427,7 +1425,6 @@ static noinline int cow_file_range(struct
btrfs_inode *inode,
                if (ret < 0)
                        goto out_unlock;
                cur_alloc_size = ins.offset;
-               extent_reserved = true;

                ram_size = ins.offset;
                file_extent.disk_bytenr = ins.objectid;
@@ -1503,7 +1500,7 @@ static noinline int cow_file_range(struct
btrfs_inode *inode,
                        num_bytes -= cur_alloc_size;
                alloc_hint = ins.objectid + ins.offset;
                start += cur_alloc_size;
-               extent_reserved = false;
+               cur_alloc_size = 0;

                /*
                 * btrfs_reloc_clone_csums() error, since start is increased
@@ -1573,13 +1570,12 @@ static noinline int cow_file_range(struct
btrfs_inode *inode,
         * to decrement again the data space_info's bytes_may_use counter,
         * therefore we do not pass it the flag EXTENT_CLEAR_DATA_RESV.
         */
-       if (extent_reserved) {
+       if (cur_alloc_size) {
                extent_clear_unlock_delalloc(inode, start,
                                             start + cur_alloc_size - 1,
                                             locked_folio, &cached, clear_bits,
                                             page_ops);
                btrfs_qgroup_free_data(inode, NULL, start,
cur_alloc_size, NULL);
-               start += cur_alloc_size;
        }

        /*
@@ -1588,11 +1584,13 @@ static noinline int cow_file_range(struct
btrfs_inode *inode,
         * space_info's bytes_may_use counter, reserved in
         * btrfs_check_data_free_space().
         */
-       if (start < end) {
+       if (start + cur_alloc_size < end) {
                clear_bits |= EXTENT_CLEAR_DATA_RESV;
-               extent_clear_unlock_delalloc(inode, start, end, locked_folio,
+               extent_clear_unlock_delalloc(inode, start + cur_alloc_size,
+                                            end, locked_folio,
                                             &cached, clear_bits, page_ops);
-               btrfs_qgroup_free_data(inode, NULL, start, end - start
+ 1, NULL);
+               btrfs_qgroup_free_data(inode, NULL, start + cur_alloc_size,
+                               end - start - cur_alloc_size + 1, NULL);
        }
        return ret;
 }


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;
> >   }
>