Re: [PATCH v2] btrfs: tree-checker: add qgroup status item check

[Qu Wenruo <wqu@suse.com>]

在 2026/9/1 07:21, Boris Burkov 写道:
> On Wed, Aug 26, 2026 at 07:23:15PM +0930, Qu Wenruo wrote:
>> This adds the following checks:
>>
>> - Key check
>>
>> - Item size check
>>    Here we do not require completely matching the older or newer qgroup
>>    status item size, this is to allow future structure expansion.
>>
>> - Version check
>>    Again it's not a strict requirement for the version to match the
>>    support one, but to catch obvious bitflip.
>>
>>    And the kernel will already reject unknown version by disabling qgroup.
>>
>>    So we allow any version that is no larger than U8_MAX, which I believe
>>    we won't reach in the next few decades.
>>
>> - Flags check
>>    Mostly to catch any unexpected RUNTIME flags.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Loosen the version check
>> - Loosen the item size check
>>    To allow older kernels to mount future newer qgroup expansions,
>>    other than completely rejecting the qgroup tree and failing the mount.
>> ---
>>   fs/btrfs/tree-checker.c | 64 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 0ce91396b517..ff268ebb22d7 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -2313,6 +2313,67 @@ static int check_free_space_bitmap(struct extent_buffer *leaf,
>>   	return 0;
>>   }
>>   
>> +static int check_qgroup_status_item(const struct extent_buffer *leaf,
>> +				    const struct btrfs_key *key, int slot)
>> +{
>> +	struct btrfs_qgroup_status_item *qsi;
>> +	const u32 item_size = btrfs_item_size(leaf, slot);
>> +	/*
>> +	 * Since the introduction of simple mode, the size of qsi has been
>> +	 * enlarged. We need to handle both the old and new sizes.
>> +	 */
>> +	const unsigned int old_qsi_size =
>> +		offsetof(struct btrfs_qgroup_status_item, enable_gen);
>> +	u64 flags;
>> +
>> +	if (unlikely(key->objectid != 0 || key->offset != 0)) {
>> +		const struct btrfs_key expected = { .type = BTRFS_QGROUP_STATUS_KEY };
>> +
>> +		generic_err(leaf, slot,
>> +	"invalid qgroup status item key, has " BTRFS_KEY_FMT " expect " BTRFS_KEY_FMT,
>> +			    BTRFS_KEY_FMT_VALUE(key),
>> +			    BTRFS_KEY_FMT_VALUE(&expected));
>> +		return -EUCLEAN;
>> +	}
>> +
>> +	if (unlikely(item_size < old_qsi_size)) {
>> +		generic_err(leaf, slot,
>> +			    "invalid qgroup status item size, has %u expect at least %u",
>> +			    item_size, old_qsi_size);
>> +		return -EUCLEAN;
>> +	}
>> +
>> +	qsi = btrfs_item_ptr(leaf, slot, struct btrfs_qgroup_status_item);
>> +
>> +	/*
>> +	 * The version (1) hasn't changed for a long time, but even if we
>> +	 * are going to support a newer version, it won't suddenly jump
>> +	 * over 255 in the foreseeable future.
>> +	 */
>> +	if (unlikely(btrfs_qgroup_status_version(leaf, qsi) > U8_MAX)) {
>> +		generic_err(leaf, slot,
>> +			    "suspicious qgroup status version, has %llu expect %u",
>> +			    btrfs_qgroup_status_version(leaf, qsi),
>> +			    BTRFS_QGROUP_STATUS_VERSION);
>> +		return -EUCLEAN;
>> +	}
>> +	flags = btrfs_qgroup_status_flags(leaf, qsi);
>> +	if (unlikely(flags & ~BTRFS_QGROUP_STATUS_FLAGS_MASK)) {
>> +		generic_err(leaf, slot,
>> +			    "unknown qgroup status flags, has 0x%llx unknown flags 0x%llx",
>> +			    flags, flags & ~BTRFS_QGROUP_STATUS_FLAGS_MASK);
>> +		return -EUCLEAN;
>> +	}
>> +	if (unlikely(flags & BTRFS_QGROUP_STATUS_FLAG_SIMPLE_MODE &&
>> +		     item_size < sizeof(*qsi))) {
> 
> I assume this SIMPLE check is because enable_gen is new after the simple
> quotas change and that old qgroup status items will be smaller still?

Yes, the reason is correct.

For those still running qgroups and have never disabled it (which I 
believe they are minority now), their qgroup status items will stay 
without the enable_gen member.

> I don't think that checking the flags of the given item makes sense,
> though? If we happen to not use squota the flag won't be set right? but
> if the size is tiny we pass this check anyway? Or is the idea to look
> for specifically squota status items missing enable_gen? Why not look
> for normal ones missing rescan or flags or whatever?

This is the common practice for tree-checker, remember that tree-checker 
is the "2nd best memory tester" to catch weird problems, not only from 
btrfs, but also bad hardware.

So the principle is not "this should not happen so we don't need to 
check" but "every member should be verified when possible".

And this is part of the common path for any variable sized structure, we 
need to make sure the added member is not beyond the item boundary.


Finally for the "why not look for normal ones missing rescan or flags or 
whatever" part, that's because for regular qgroups, it's completely 
valid to not have RESCAN/INCONSISTENT flag set, so we do not need to 
check that.

But this inspired me that, for simple mode we should not have RESCAN 
flag set.
I'll add this check into the next version.

Thanks for the review,
Qu

> 
>> +		generic_err(leaf, slot,
>> +			    "invalid qgroup status item size, has %u expect at least %zu",
>> +			    item_size, sizeof(*qsi));
>> +		return -EUCLEAN;
>> +	}
>> +	return 0;
>> +}
>> +
>>   /*
>>    * Common point to switch the item-specific validation.
>>    */
>> @@ -2394,6 +2455,9 @@ static enum btrfs_tree_block_status check_leaf_item(struct extent_buffer *leaf,
>>   	case BTRFS_REMAP_BACKREF_KEY:
>>   		ret = check_remap_key(leaf, key, slot);
>>   		break;
>> +	case BTRFS_QGROUP_STATUS_KEY:
>> +		ret = check_qgroup_status_item(leaf, key, slot);
>> +		break;
>>   	}
>>   
>>   	if (unlikely(ret))
>> -- 
>> 2.55.0
>>