Quota enable adds qgroups to fs_info->qgroup_tree before the operation is
guaranteed to succeed.  On failure, the current error path removes only the
sysfs entries, leaving the qgroups and their relations in memory.

Use btrfs_free_qgroup_config() instead.  It is the existing qgroup teardown
helper and removes the in-memory qgroups, their relations and the sysfs
entries.

This is safe because the failure cleanup runs with qgroup_ioctl_lock held
and before quota_root is published or quota accounting is enabled.  Qgroups
already added to the tree are freed by btrfs_free_qgroup_config(), while an
untransferred preallocation and quota_root are still released by the
existing cleanup code.  Transaction handling is unchanged.

Signed-off-by: Guanjie Chen <jk.chen1095@gmail.com>
---
 fs/btrfs/qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1292,7 +1292,7 @@ out_free_root:
 		btrfs_put_root(quota_root);
 out:
 	if (ret)
-		btrfs_sysfs_del_qgroups(fs_info);
+		btrfs_free_qgroup_config(fs_info);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (ret && trans)
 		btrfs_end_transaction(trans);
-- 
2.43.0

在 2026/8/14 20:52, Guanjie Chen 写道:
> Quota enable adds qgroups to fs_info->qgroup_tree before the operation is
> guaranteed to succeed.  On failure, the current error path removes only the
> sysfs entries, leaving the qgroups and their relations in memory.
> 
> Use btrfs_free_qgroup_config() instead.  It is the existing qgroup teardown
> helper and removes the in-memory qgroups, their relations and the sysfs
> entries.
> 
> This is safe because the failure cleanup runs with qgroup_ioctl_lock held
> and before quota_root is published or quota accounting is enabled.  Qgroups
> already added to the tree are freed by btrfs_free_qgroup_config(), while an
> untransferred preallocation and quota_root are still released by the
> existing cleanup code.  Transaction handling is unchanged.
> 
> Signed-off-by: Guanjie Chen <jk.chen1095@gmail.com>

LLM disclosure.

In fact, for anyone who doesn't have any previous btrfs involvement, I 
have a strong feeling such error handling bug is fully exposed by LLM, 
not yourself.

> ---
>   fs/btrfs/qgroup.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1292,7 +1292,7 @@ out_free_root:
>   		btrfs_put_root(quota_root);
>   out:
>   	if (ret)
> -		btrfs_sysfs_del_qgroups(fs_info);
> +		btrfs_free_qgroup_config(fs_info);

And the error handling, no matter the older or the newer one, is 
incorrect in the first place.

In fact, the newer one is worse.

We can reach this out branch if btrfs_start_transaction() failed, but 
the fs_info->quota_root is already setup by another process.

In that case, the older code only removes the sysfs files, but keeps the 
qgroups in the rb tree.

But now all qgroups are removed unexpectedly.

>   	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>   	if (ret && trans)
>   		btrfs_end_transaction(trans);

Hi Qu,

> We can reach this out branch if btrfs_start_transaction() failed, but
> the fs_info->quota_root is already setup by another process.

Do you mean the window where qgroup_ioctl_lock is dropped before
btrfs_start_transaction()?

I don't think another task can set fs_info->quota_root in that window.
The quota ioctl holds fs_info->subvol_sem for writing across the whole
btrfs_quota_enable() call, including while qgroup_ioctl_lock is dropped.

As far as I can see, quota_root is only set in two places:
btrfs_quota_enable(), which is serialized by subvol_sem, and
btrfs_read_roots(), which runs during mount initialization.

The disable path also calls btrfs_free_qgroup_config() and relies on the
same subvol_sem and qgroup_ioctl_lock serialization for the qgroup
configuration. If subvol_sem did not prevent another quota operation
here, the disable path would have a similar lifetime problem.

So if quota_root was NULL at the first check, I believe it must still be
NULL when btrfs_start_transaction() fails. I agree that the changelog
should have mentioned subvol_sem as part of the protection.

Did I miss another runtime path that can set quota_root without holding
subvol_sem?

Also, just to clarify, this is not my first btrfs contribution, although
my previous change was only a small one.

Thanks,
Guanjie

在 2026/8/14 22:00, Guanjie Chen 写道:
> Hi Qu,
> 
>> We can reach this out branch if btrfs_start_transaction() failed, but
>> the fs_info->quota_root is already setup by another process.
> 
> Do you mean the window where qgroup_ioctl_lock is dropped before
> btrfs_start_transaction()?
> 
> I don't think another task can set fs_info->quota_root in that window.
> The quota ioctl holds fs_info->subvol_sem for writing across the whole
> btrfs_quota_enable() call, including while qgroup_ioctl_lock is dropped.
> 
> As far as I can see, quota_root is only set in two places:
> btrfs_quota_enable(), which is serialized by subvol_sem, and
> btrfs_read_roots(), which runs during mount initialization.

Right, I missed that rwsem.

But still, for error handling one should be in the reverse order.

And for out: label, there are cases we're going out label without 
creating any qgroup. In that case, qgroups removing should be done only 
for cases where a qgroup is created.

> 
> The disable path also calls btrfs_free_qgroup_config() and relies on the
> same subvol_sem and qgroup_ioctl_lock serialization for the qgroup
> configuration. If subvol_sem did not prevent another quota operation
> here, the disable path would have a similar lifetime problem.
> 
> So if quota_root was NULL at the first check, I believe it must still be
> NULL when btrfs_start_transaction() fails. I agree that the changelog
> should have mentioned subvol_sem as part of the protection.
> 
> Did I miss another runtime path that can set quota_root without holding
> subvol_sem?
> 
> Also, just to clarify, this is not my first btrfs contribution, although
> my previous change was only a small one.

The same, LLM disclosure if you're using it.

Nowadays I won't trust anyone who is not a regular contributor.

> 
> Thanks,
> Guanjie

Hi Qu,

Thanks for the review.

I agree the error path should be reversed, even though btrfs_free_qgroup_config() has defensive checks so it won't UAF. But this isn't a one-liner – maybe we can refactor the whole init/cleanup flow later and fix it properly.

As for contributing: I know there's been lots of LLM noise lately, and I don't want to add to it. I've been studying btrfs for about 2~3 years now. For my next patch, would it help if I clearly explain the root cause, my reasoning, and the solution in the commit message or cover letter? I'd like to make review smoother.

Thanks again for your time.

Best,
Guanjie

在 2026/8/15 16:14, Guanjie Chen 写道:
> Hi Qu,
> 
> Thanks for the review.
> 
> I agree the error path should be reversed, even though btrfs_free_qgroup_config() has defensive checks so it won't UAF. But this isn't a one-liner – maybe we can refactor the whole init/cleanup flow later and fix it properly.
> 
> As for contributing: I know there's been lots of LLM noise lately, and I don't want to add to it. I've been studying btrfs for about 2~3 years now. For my next patch, would it help if I clearly explain the root cause, my reasoning, and the solution in the commit message or cover letter? I'd like to make review smoother.

I don't care, the policy is there, and all active developers are 
following it:

https://docs.kernel.org/process/coding-assistants.html


I see no point why you want to be special and do not disclose whether 
you have used LLM, unless you have something to hide and then I have all 
reasons to question your motivation.

Even well known developers are using LLM actively, e.g:

https://lore.kernel.org/linux-btrfs/20260811-btrfs-enomem-v3-0-46a993fc3fe5@kernel.org/

https://lore.kernel.org/linux-btrfs/69d0043e0f6a3d17048dfde857127ab0bf331154.1785190866.git.boris@bur.io/

And of course myself:

https://lore.kernel.org/linux-btrfs/61fd5ab4b532ba3a7915dc629339db230cccd52f.1786007917.git.wqu@suse.com/

It's very simple, if you used LLM to analyze/fix the bug, then just 
disclose it.

> 
> Thanks again for your time.
> 
> Best,
> Guanjie