[PATCH] btrfs: fix use-after-free in move_existing_remap()

[PATCH] btrfs: fix use-after-free in move_existing_remap()

[Mark Harmstone]

Fix a potential use-after-free in move_existing_remap(): we're calling
btrfs_put_block_group() on dest_bg, then passing it to
btrfs_add_block_group_free_space() a few lines later.

Link: https://lore.kernel.org/linux-btrfs/20260125123908.2096548-1-clm@meta.com/
Reported-by: Chris Mason <clm@fb.com>
Fixes: bbea42dfb91f ("btrfs: move existing remaps before relocating block group")
Signed-off-by: Mark Harmstone <mark@harmstone.com>
---
 fs/btrfs/relocation.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index cc80760ba5e7..5a8644667620 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4295,14 +4295,17 @@ static int move_existing_remap(struct btrfs_fs_info *fs_info,
 	bg_needs_free_space = test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
 				       &dest_bg->runtime_flags);
 	mutex_unlock(&dest_bg->free_space_lock);
-	btrfs_put_block_group(dest_bg);
 
 	if (bg_needs_free_space) {
 		ret = btrfs_add_block_group_free_space(trans, dest_bg);
-		if (unlikely(ret))
+		if (unlikely(ret)) {
+			btrfs_put_block_group(dest_bg);
 			goto end;
+		}
 	}
 
+	btrfs_put_block_group(dest_bg);
+
 	ret = btrfs_remove_from_free_space_tree(trans, dest_addr, dest_length);
 	if (unlikely(ret)) {
 		btrfs_remove_from_free_space_tree(trans, new_addr, dest_length);
-- 
2.52.0

Re: [PATCH] btrfs: fix use-after-free in move_existing_remap()

[Johannes Thumshirn]

On 2/25/26 11:36 AM, Mark Harmstone wrote:
> Fix a potential use-after-free in move_existing_remap(): we're calling
> btrfs_put_block_group() on dest_bg, then passing it to
> btrfs_add_block_group_free_space() a few lines later.
>
> Link: https://lore.kernel.org/linux-btrfs/20260125123908.2096548-1-clm@meta.com/
> Reported-by: Chris Mason <clm@fb.com>
> Fixes: bbea42dfb91f ("btrfs: move existing remaps before relocating block group")
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>   fs/btrfs/relocation.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index cc80760ba5e7..5a8644667620 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4295,14 +4295,17 @@ static int move_existing_remap(struct btrfs_fs_info *fs_info,
>   	bg_needs_free_space = test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
>   				       &dest_bg->runtime_flags);
>   	mutex_unlock(&dest_bg->free_space_lock);
> -	btrfs_put_block_group(dest_bg);
>   
>   	if (bg_needs_free_space) {
>   		ret = btrfs_add_block_group_free_space(trans, dest_bg);
> -		if (unlikely(ret))
> +		if (unlikely(ret)) {
> +			btrfs_put_block_group(dest_bg);
>   			goto end;
> +		}
>   	}
>   
> +	btrfs_put_block_group(dest_bg);
> +
>   	ret = btrfs_remove_from_free_space_tree(trans, dest_addr, dest_length);
>   	if (unlikely(ret)) {
>   		btrfs_remove_from_free_space_tree(trans, new_addr, dest_length);
Why can't you move the put to the end of the function and skip the new 
lookup of the BG further down the function?