| 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: <CALv5hoQwjE=4mqEst1ay5YF3eAj2TNdjtLmHbBNCwxsfDXJQTA@mail.gmail.com>
Date: Tue, 8 Oct 2024 19:18:14 +0800
From: hs wang <iamhswang@...il.com>
To: Qu Wenruo <quwenruo.btrfs@....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
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.
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