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

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

[Qu Wenruo]

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))) {
+		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

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

[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? 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?

> +		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
> 

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

[Qu Wenruo]

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

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

[David Sterba]

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.

I don't see this patch in for-next but it seems OK for merge.

> +	/*
> +	 * 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)) {

Why is it U8_MAX instead of 255 as said in the comment? I understand
that it is for one byte, although the structure has __le64 for it.

I also don't expect the version to reach any high number, the version
for such item is an outlier in the data structure format, we usually
guess the version from the raw item size. Keeping the version inside
without any reserved/unused filelds also make it pointless. The only
meaningful way I see is to split the u64 to 1+3 bytes with 1st byte to
be the version.

Actually, looking to the history, commit bd7c1ea3a302ab ("btrfs: qgroup:
check generation when recording simple quota delta") already increased
the structure size, so we don't even make use of the version.

As it is now it's basically wasting the 4 bytes and we won't increase it
anyway so can think about how to scrap it completely and make the test
stricter.

> +		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))) {
> +		generic_err(leaf, slot,
> +			    "invalid qgroup status item size, has %u expect at least %zu",
> +			    item_size, sizeof(*qsi));
> +		return -EUCLEAN;
> +	}
> +	return 0;
> +}

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

[Qu Wenruo]

在 2026/9/5 02:44, David Sterba 写道:
> 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.
> 
> I don't see this patch in for-next but it seems OK for merge.

Mostly because Sashiko still has some complain about the over-strict 
checks on the flags.
AKA, any newly introduced flag will be rejected by this check, causing 
mount failure.

> 
>> +	/*
>> +	 * 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)) {
> 
> Why is it U8_MAX instead of 255 as said in the comment?

U8_MAX is exactly 255. Although for the consistency I'll change the 
comment to use the same U8_MAX.

> I understand
> that it is for one byte, although the structure has __le64 for it.
> 
> I also don't expect the version to reach any high number, the version
> for such item is an outlier in the data structure format, we usually
> guess the version from the raw item size. Keeping the version inside
> without any reserved/unused filelds also make it pointless. The only
> meaningful way I see is to split the u64 to 1+3 bytes with 1st byte to
> be the version.
> 
> Actually, looking to the history, commit bd7c1ea3a302ab ("btrfs: qgroup:
> check generation when recording simple quota delta") already increased
> the structure size, so we don't even make use of the version.
> 
> As it is now it's basically wasting the 4 bytes and we won't increase it
> anyway so can think about how to scrap it completely and make the test
> stricter.

I think the version split (1 bytes for version, 3 bytes for future 
expansion) is the best solution.

Then I'm totally fine to drop this check.

> 
>> +		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))) {
>> +		generic_err(leaf, slot,
>> +			    "invalid qgroup status item size, has %u expect at least %zu",
>> +			    item_size, sizeof(*qsi));
>> +		return -EUCLEAN;
>> +	}
>> +	return 0;
>> +}
>