| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ea99632a-e873-4f53-9d47-97b1a714586b@gmx.com>
Date: Wed, 9 Oct 2024 07:20:22 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: hs wang <iamhswang@...il.com>
Cc: linux-btrfs@...r.kernel.org, clm@...com, josef@...icpanda.com,
dsterba@...e.com, wqu@...e.com, boris@....io, linux-kernel@...r.kernel.org,
Haisu Wang <haisuwang@...cent.com>
Subject: Re: [PATCH] btrfs: fix the length of reserved qgroup to free
在 2024/10/8 21:48, hs wang 写道:
> Qu Wenruo <quwenruo.btrfs@....com> 于2024年10月8日周二 15:56写道：
>>
>>
>>
>> 在 2024/10/8 17:18, iamhswang@...il.com 写道:
>>> From: Haisu Wang <haisuwang@...cent.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@...cent.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@...e.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;
>>> }
>>
Powered by blists - more mailing lists