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

[Qu Wenruo <wqu@suse.com>]

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