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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/10/8 21:48, hs wang 写道:
> Qu Wenruo <quwenruo.btrfs@gmx.com> 于2024年10月8日周二 15:56写道：
>>
>>
>>
>> 在 2024/10/8 17:18, iamhswang@gmail.com 写道:
>>> From: Haisu Wang <haisuwang@tencent.com>
>>>
>>> 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@tencent.com>
>>
>> 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@suse.com>
>>
>> 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.

This snippet looks good to me.

Thanks,
Qu
>
> 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;
>>>    }
>>