Re: [PATCH 1/2] btrfs: overwrite a leftover balance item instead of BUG_ON()

[Qu Wenruo <wqu@suse.com>]

在 2026/11/6 08:17, Siddharth Mishra 写道:
> [BUG]
> A balance ioctl can hit this BUG_ON():
> 
>    kernel BUG at fs/btrfs/volumes.c:5088!
>    Oops: invalid opcode: 0000 [#1] SMP NOPTI
>    RIP: 0010:btrfs_balance (fs/btrfs/volumes.c:5088)
>    Call Trace:
>     <TASK>
>     btrfs_ioctl (fs/btrfs/ioctl.c:3523 fs/btrfs/ioctl.c:5650)
>     __x64_sys_ioctl
>     do_syscall_64
>     entry_SYSCALL_64_after_hwframe
>     </TASK>
> 
> [CAUSE]
> btrfs_balance() expects insert_balance_item() to return -EEXIST only
> when a balance item is on disk, and a balance item to be on disk only
> when fs_info->balance_ctl is set (a paused balance, or one recovered
> at mount). So for a new balance it uses BUG_ON(ret == -EEXIST).
> 
> Both assumptions can be wrong:
> 
> - insert_balance_item() inserts the item, then commits the transaction.
>    btrfs_commit_transaction() can fail without aborting the
>    transaction, for example when btrfs_start_dirty_block_groups() fails
>    to allocate a path.

I think no call site can handle a btrfs_commit_transaction() failure 
without transaction abort.

This is a way bigger problem than what you're fixing.

> Then balance fails with -ENOMEM and
>    balance_ctl is freed, but the item stays in the running transaction.
>    The next balance ioctl finds the item and gets -EEXIST.
> 
> - When the transaction was aborted with -EEXIST, for example after
>    "failed to run delayed ref ... -17" on a corrupted extent tree,
>    btrfs_commit_transaction() returns the abort error, -EEXIST, and
>    there is no balance item at all.
> 
> [FIX]
> Mount turns any balance item on disk into a paused balance, so a new
> balance can only find an item left by a balance of the same mount that
> failed after inserting it.  No balance owns that item, so let a new
> balance overwrite it, run, and delete it at the end as usual, as Filipe
> suggested in [1].
> 
> For a resumed balance -EEXIST is the normal result.  Any other error,
> including the -EEXIST of an aborted transaction for a new balance, now
> goes to the existing error path.
> 
> The BUG_ON() checks on the return value go away, and the one on
> fs_info->balance_ctl becomes an ASSERT().
> 
> Reported-by: syzbot+dc9291cf132691aeb478@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=dc9291cf132691aeb478
> Link: https://lore.kernel.org/linux-btrfs/ZNzE6CFOzu9kDG+G@debian0.Home/ [1]
> Fixes: 1bbc621ef284 ("Btrfs: allow block group cache writeout outside critical section in commit")
> Cc: stable@vger.kernel.org # 4.1+
> Suggested-by: Filipe Manana <fdmanana@kernel.org>
> Signed-off-by: Siddharth Mishra <smish149@asu.edu>
> ---
>   fs/btrfs/volumes.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 3d4ef6725..4a8e5078b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3998,6 +3998,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>   
>   	ret = btrfs_insert_empty_item(trans, root, path, &key,
>   				      sizeof(*item));
> +	if (ret == -EEXIST && !(bctl->flags & BTRFS_BALANCE_RESUME))
> +		ret = 0;
>   	if (ret)
>   		goto out;
>   
> @@ -5081,17 +5083,17 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>   	}
>   
>   	ret = insert_balance_item(fs_info, bctl);
> -	if (ret && ret != -EEXIST)
> +	if (ret == -EEXIST && (bctl->flags & BTRFS_BALANCE_RESUME))
> +		ret = 0;
> +	if (ret)
>   		goto out;
>   
>   	if (!(bctl->flags & BTRFS_BALANCE_RESUME)) {
> -		BUG_ON(ret == -EEXIST);
> -		BUG_ON(fs_info->balance_ctl);
> +		ASSERT(!fs_info->balance_ctl);
>   		spin_lock(&fs_info->balance_lock);
>   		fs_info->balance_ctl = bctl;
>   		spin_unlock(&fs_info->balance_lock);
>   	} else {
> -		BUG_ON(ret != -EEXIST);
>   		spin_lock(&fs_info->balance_lock);
>   		update_balance_args(bctl);
>   		spin_unlock(&fs_info->balance_lock);