[PATCH] btrfs: fix block group ref leak when scrub races with unused bg deletion

[PATCH] btrfs: fix block group ref leak when scrub races with unused bg deletion

[Boris Burkov]

btrfs_delete_unused_bgs() takes a block group off fs_info->unused_bgs
with list_del_init() and, when discard is on, later moves it onto the
transaction's deleted_bgs list and takes a reference for that list
membership. Meanwhile, it is possible for scrub to add it back to the
unused block groups list:

         CLEANER                               SCRUB
  btrfs_delete_unused_bgs()
    list_del_init(&bg->bg_list)
                                        scrub_enumerate_chunks()
                                          btrfs_mark_bg_unused(bg)
                                            if (list_empty(&bg->bg_list))
                                              btrfs_get_block_group(bg)
                                              list_add_tail(unused_bgs)
    list_move(&bg->bg_list, deleted_bgs)
    btrfs_get_block_group(bg)

Other users of list_move() for bg->bg_list (like btrfs_mark_bg_unused
itself) ensure that the list was empty under the bg spin lock to decide
whether it merits a fresh reference for being linked to the list, and
we need to apply that pattern here as well.

Note that this is not abstracted to a helper function because some of
the callers have additional logic deciding whether to link it or not and
I think a twice used, twice unused helper would be more confusing than it
would be helpful.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/bug/linux-821034ac-556f-4d72-af2c-d04468879af8
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/block-group.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 027830a53f7d..0b3b8843d221 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1830,12 +1830,15 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 			/*
 			 * A concurrent scrub might have added us to the list
 			 * fs_info->unused_bgs, so use a list_move operation
-			 * to add the block group to the deleted_bgs list.
+			 * to add the block group to the deleted_bgs list. In
+			 * that case the list reference moves along with it,
+			 * so only take a new one if we were not on any list.
 			 */
+			if (list_empty(&block_group->bg_list))
+				btrfs_get_block_group(block_group);
 			list_move(&block_group->bg_list,
 				  &trans->transaction->deleted_bgs);
 			spin_unlock(&fs_info->unused_bgs_lock);
-			btrfs_get_block_group(block_group);
 		}
 end_trans:
 		/*
-- 
2.56.0

Re: [PATCH] btrfs: fix block group ref leak when scrub races with unused bg deletion

[Qu Wenruo]

在 2026/11/12 10:38, Boris Burkov 写道:
> btrfs_delete_unused_bgs() takes a block group off fs_info->unused_bgs
> with list_del_init() and, when discard is on, later moves it onto the
> transaction's deleted_bgs list and takes a reference for that list
> membership. Meanwhile, it is possible for scrub to add it back to the
> unused block groups list:
> 
>           CLEANER                               SCRUB
>    btrfs_delete_unused_bgs()
>      list_del_init(&bg->bg_list)
>                                          scrub_enumerate_chunks()
>                                            btrfs_mark_bg_unused(bg)
>                                              if (list_empty(&bg->bg_list))
>                                                btrfs_get_block_group(bg)
>                                                list_add_tail(unused_bgs)
>      list_move(&bg->bg_list, deleted_bgs)
>      btrfs_get_block_group(bg)
> 
> Other users of list_move() for bg->bg_list (like btrfs_mark_bg_unused
> itself) ensure that the list was empty under the bg spin lock to decide
> whether it merits a fresh reference for being linked to the list, and
> we need to apply that pattern here as well.
> 
> Note that this is not abstracted to a helper function because some of
> the callers have additional logic deciding whether to link it or not and
> I think a twice used, twice unused helper would be more confusing than it
> would be helpful.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/bug/linux-821034ac-556f-4d72-af2c-d04468879af8
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/block-group.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 027830a53f7d..0b3b8843d221 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1830,12 +1830,15 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
>   			/*
>   			 * A concurrent scrub might have added us to the list
>   			 * fs_info->unused_bgs, so use a list_move operation
> -			 * to add the block group to the deleted_bgs list.
> +			 * to add the block group to the deleted_bgs list. In
> +			 * that case the list reference moves along with it,
> +			 * so only take a new one if we were not on any list.
>   			 */
> +			if (list_empty(&block_group->bg_list))
> +				btrfs_get_block_group(block_group);
>   			list_move(&block_group->bg_list,
>   				  &trans->transaction->deleted_bgs);
>   			spin_unlock(&fs_info->unused_bgs_lock);
> -			btrfs_get_block_group(block_group);
>   		}
>   end_trans:
>   		/*