[PATCH 0/2] btrfs: balance item fixes

[PATCH 0/2] btrfs: balance item fixes

[Siddharth Mishra]

Patch 1 fixes a BUG_ON() in btrfs_balance() on a valid filesystem.  A
balance can leave its item in the root tree when the commit after the
insert fails without an abort, and the next balance then finds the item
and hits BUG_ON(ret == -EEXIST).  syzbot reported it, see patch 1.  The
new balance now overwrites the item that no balance owns.

Patch 2 is hardening.  Nothing checks the size of the balance item, and
btrfs_recover_balance() reads past a short item at mount.  The
tree-checker now rejects a balance item with the wrong size.

Found while looking into a fuzzing report.  An AI tool helped with the
analysis and the patches.

Tested on btrfs for-next df5b88a7914e, KASAN and non-debug kernels:
- Patch 1: on a new filesystem, one failed allocation in
  btrfs_start_dirty_block_groups() during a first balance, then a
  second balance: the BUG_ON() before; with the patch the second
  balance runs to the end, no balance item is left and btrfs check is
  clean.  The same with a failed allocation at each of the first 206
  allocation points of the first balance: no BUG, KASAN report or
  assertion.  Not reproduced without fault injection.  The other way to
  the BUG_ON(), a transaction aborted with -EEXIST, is not reproduced
  on this base: the corrupted image that hit it is now refused by the
  tree-checker at mount.
- Patch 2: a crafted image with a 400 byte balance item is refused at
  mount; valid filesystems (a paused balance resumed or cancelled after
  mount, a balance paused and resumed) are not rejected.
- Both: pause, resume, cancel and skip_balance work as before;
  fstests -g balance and btrfs/220: no new failure.

The crafted image is available on request.

Siddharth Mishra (2):
  btrfs: overwrite a leftover balance item instead of BUG_ON()
  btrfs: tree-checker: add balance item size check

 fs/btrfs/tree-checker.c | 17 +++++++++++++++++
 fs/btrfs/volumes.c      | 10 ++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)


base-commit: df5b88a7914e18488908ddf13e14e31ab39f954c
-- 
2.54.0

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

[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. 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);
-- 
2.54.0

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

[Qu Wenruo]

在 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);

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

[Siddharth Mishra]

On Fri, Nov 06, 2026 at 08:39:01AM +1030, Qu Wenruo wrote:
> >
> > 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.

Are you suggesting that the right fix is to make
btrfs_commit_transaction() abort the transaction when
btrfs_start_dirty_block_groups() fails?

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

[Qu Wenruo]

在 2026/11/6 09:10, Siddharth Mishra 写道:
> On Fri, Nov 06, 2026 at 08:39:01AM +1030, Qu Wenruo wrote:
>>>
>>> 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.
> 
> Are you suggesting that the right fix is to make
> btrfs_commit_transaction() abort the transaction when
> btrfs_start_dirty_block_groups() fails?

At least one should check if all btrfs_commit_transaction() callers can 
handle such error correctly.

[PATCH 2/2] btrfs: tree-checker: add balance item size check

[Siddharth Mishra]

[BACKGROUND]
A paused or interrupted balance leaves a balance item in the root tree,
with the key (BALANCE_OBJECTID TEMPORARY_ITEM 0) and a struct
btrfs_balance_item of 448 bytes.  At mount btrfs_recover_balance()
reads the flags and the data, meta and sys args from this item, and
the balance is resumed with them.

[CAUSE]
The tree-checker has no check for TEMPORARY_ITEM keys, and
btrfs_recover_balance() reads the item without checking its size.
With a balance item that is too short, the reads go past the end of
the item into the data of the previous item in the leaf, and the
balance is resumed with whatever those bytes hold.  Nothing is
reported.  For example this balance item is 48 bytes short:

  item 11 key (DATA_RELOC_TREE ROOT_ITEM 0) itemoff 12545 itemsize 439
  item 12 key (BALANCE TEMPORARY_ITEM 0) itemoff 12145 itemsize 400
                                                       ^^^^^^^^^^^^

The mount succeeds and the balance is resumed:

  BTRFS info (device vda): balance: resume
  BTRFS info (device vda): balance: ended with status: 0

[ENHANCEMENT]
Add check_balance_item() to reject a balance item whose size is not
the size of struct btrfs_balance_item, so the leaf is refused when it
is read:

  BTRFS critical (device vda): corrupt leaf: root=1 block=30654464
  slot=12, invalid balance item size, have 400 expect 448
  BTRFS error (device vda): read time tree block corruption detected
  on logical 30654464 mirror 1
  BTRFS warning (device vda): couldn't read tree root
  BTRFS error (device vda): open_ctree failed: -EIO

Signed-off-by: Siddharth Mishra <smish149@asu.edu>
---
An alternative is to check the size only where the item is read, in
btrfs_recover_balance(), before bctl is allocated.  That refuses only
the balance item and fails the mount with -EUCLEAN, instead of refusing
the whole root tree leaf:

  BTRFS error (device vda): invalid balance item size, have 400 expect 448
  BTRFS error (device vda): failed to recover balance: -EUCLEAN
  BTRFS error (device vda): open_ctree failed: -EUCLEAN

It was tested the same way as this patch.  The tested diff, indented so
that it is not applied with this patch, in case it is preferred:

  diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
  index 3d4ef6725..97154f59f 100644
  --- a/fs/btrfs/volumes.c
  +++ b/fs/btrfs/volumes.c
  @@ -5230,11 +5230,17 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
   		return 0;
   	}
   
  +	leaf = path->nodes[0];
  +	if (unlikely(btrfs_item_size(leaf, path->slots[0]) != sizeof(*item))) {
  +		btrfs_err(fs_info, "invalid balance item size, have %u expect %zu",
  +			  btrfs_item_size(leaf, path->slots[0]), sizeof(*item));
  +		return -EUCLEAN;
  +	}
  +
   	bctl = kzalloc_obj(*bctl, GFP_NOFS);
   	if (!bctl)
   		return -ENOMEM;
   
  -	leaf = path->nodes[0];
   	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_balance_item);
   
   	bctl->flags = btrfs_balance_flags(leaf, item);

 fs/btrfs/tree-checker.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index f021ac14b..5698845be 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -2117,6 +2117,19 @@ static int check_qgroup_status_item(const struct extent_buffer *leaf, int slot)
 	return 0;
 }
 
+static int check_balance_item(const struct extent_buffer *leaf, int slot)
+{
+	const u32 item_size = btrfs_item_size(leaf, slot);
+
+	if (unlikely(item_size != sizeof(struct btrfs_balance_item))) {
+		generic_err(leaf, slot,
+			    "invalid balance item size, have %u expect %zu",
+			    item_size, sizeof(struct btrfs_balance_item));
+		return -EUCLEAN;
+	}
+	return 0;
+}
+
 static int check_remap_key(const struct extent_buffer *leaf,
 			   const struct btrfs_key *key, int slot)
 {
@@ -2512,6 +2525,10 @@ static enum btrfs_tree_block_status check_leaf_item(struct extent_buffer *leaf,
 	case BTRFS_QGROUP_STATUS_KEY:
 		ret = check_qgroup_status_item(leaf, slot);
 		break;
+	case BTRFS_TEMPORARY_ITEM_KEY:
+		if (key->objectid == BTRFS_BALANCE_OBJECTID)
+			ret = check_balance_item(leaf, slot);
+		break;
 	case BTRFS_IDENTITY_REMAP_KEY:
 	case BTRFS_REMAP_KEY:
 	case BTRFS_REMAP_BACKREF_KEY:
-- 
2.54.0

Re: [PATCH 2/2] btrfs: tree-checker: add balance item size check

[Qu Wenruo]

在 2026/11/6 08:17, Siddharth Mishra 写道:
> [BACKGROUND]
> A paused or interrupted balance leaves a balance item in the root tree,
> with the key (BALANCE_OBJECTID TEMPORARY_ITEM 0) and a struct
> btrfs_balance_item of 448 bytes.  At mount btrfs_recover_balance()
> reads the flags and the data, meta and sys args from this item, and
> the balance is resumed with them.

What's the point? The background provides nothing useful.

You should directly say that a too small balance item can make btrfs to 
read beyond item boundary.

> 
> [CAUSE]
> The tree-checker has no check for TEMPORARY_ITEM keys, and
> btrfs_recover_balance() reads the item without checking its size.
> With a balance item that is too short, the reads go past the end of
> the item into the data of the previous item in the leaf,

Or even beyond the leaf boundary if it's at the last slot, which can be 
more problematic.

> and the
> balance is resumed with whatever those bytes hold.  Nothing is
> reported.  For example this balance item is 48 bytes short:
> 
>    item 11 key (DATA_RELOC_TREE ROOT_ITEM 0) itemoff 12545 itemsize 439
>    item 12 key (BALANCE TEMPORARY_ITEM 0) itemoff 12145 itemsize 400
>                                                         ^^^^^^^^^^^^
> 
> The mount succeeds and the balance is resumed:
> 
>    BTRFS info (device vda): balance: resume
>    BTRFS info (device vda): balance: ended with status: 0

I don't think the succeeded case adds any value.

> 
> [ENHANCEMENT]
> Add check_balance_item() to reject a balance item whose size is not
> the size of struct btrfs_balance_item, so the leaf is refused when it
> is read:
> 
>    BTRFS critical (device vda): corrupt leaf: root=1 block=30654464
>    slot=12, invalid balance item size, have 400 expect 448
>    BTRFS error (device vda): read time tree block corruption detected
>    on logical 30654464 mirror 1
>    BTRFS warning (device vda): couldn't read tree root
>    BTRFS error (device vda): open_ctree failed: -EIO
> 
> Signed-off-by: Siddharth Mishra <smish149@asu.edu>
> ---
> An alternative is to check the size only where the item is read, in
> btrfs_recover_balance(), before bctl is allocated.  That refuses only
> the balance item and fails the mount with -EUCLEAN, instead of refusing
> the whole root tree leaf:
> 
>    BTRFS error (device vda): invalid balance item size, have 400 expect 448
>    BTRFS error (device vda): failed to recover balance: -EUCLEAN
>    BTRFS error (device vda): open_ctree failed: -EUCLEAN
> 
> It was tested the same way as this patch.  The tested diff, indented so
> that it is not applied with this patch, in case it is preferred:
> 
>    diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>    index 3d4ef6725..97154f59f 100644
>    --- a/fs/btrfs/volumes.c
>    +++ b/fs/btrfs/volumes.c
>    @@ -5230,11 +5230,17 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
>     		return 0;
>     	}
>     
>    +	leaf = path->nodes[0];
>    +	if (unlikely(btrfs_item_size(leaf, path->slots[0]) != sizeof(*item))) {
>    +		btrfs_err(fs_info, "invalid balance item size, have %u expect %zu",
>    +			  btrfs_item_size(leaf, path->slots[0]), sizeof(*item));
>    +		return -EUCLEAN;
>    +	}
>    +

Don't put another diff part, even with extra spaces to break diff format.

I'm almost hitting the key to send out a comment criticize the 
duplicated code.

All you doing is just to make the whole patch hard to read, not only the 
comments after the "---" but also the cursed alternative.


If you are not sure about what is the correct way to validate such item, 
do read old codes/history of tree-checker and similar patches in the 
mailing list that got rejected.

Then you should not come up with such stupid question of using the 
alternative way.


The patch looks good, but the bad comments after "---" are so bad that 
they make me hesitate to even add my reviewed-by tag.

>     	bctl = kzalloc_obj(*bctl, GFP_NOFS);
>     	if (!bctl)
>     		return -ENOMEM;
>     
>    -	leaf = path->nodes[0];
>     	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_balance_item);
>     
>     	bctl->flags = btrfs_balance_flags(leaf, item);
> 
>   fs/btrfs/tree-checker.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index f021ac14b..5698845be 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -2117,6 +2117,19 @@ static int check_qgroup_status_item(const struct extent_buffer *leaf, int slot)
>   	return 0;
>   }
>   
> +static int check_balance_item(const struct extent_buffer *leaf, int slot)
> +{
> +	const u32 item_size = btrfs_item_size(leaf, slot);
> +
> +	if (unlikely(item_size != sizeof(struct btrfs_balance_item))) {
> +		generic_err(leaf, slot,
> +			    "invalid balance item size, have %u expect %zu",
> +			    item_size, sizeof(struct btrfs_balance_item));
> +		return -EUCLEAN;
> +	}
> +	return 0;
> +}
> +
>   static int check_remap_key(const struct extent_buffer *leaf,
>   			   const struct btrfs_key *key, int slot)
>   {
> @@ -2512,6 +2525,10 @@ static enum btrfs_tree_block_status check_leaf_item(struct extent_buffer *leaf,
>   	case BTRFS_QGROUP_STATUS_KEY:
>   		ret = check_qgroup_status_item(leaf, slot);
>   		break;
> +	case BTRFS_TEMPORARY_ITEM_KEY:
> +		if (key->objectid == BTRFS_BALANCE_OBJECTID)
> +			ret = check_balance_item(leaf, slot);
> +		break;
>   	case BTRFS_IDENTITY_REMAP_KEY:
>   	case BTRFS_REMAP_KEY:
>   	case BTRFS_REMAP_BACKREF_KEY: