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

[Boris Burkov <boris@bur.io>]

On Tue, Oct 08, 2024 at 02:48:46PM +0800, iamhswang@gmail.com wrote:
> 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>

Good catch and fix, thank you!
Reviewed-by: Boris Burkov <boris@bur.io>

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.

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?

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