Re: [PATCH] btrfs: fix use-after-free on quota enable allocation failure

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/10/7 22:25, Yalagada Pavan Kumar 写道:
> On Wed, Oct 07, 2026 at 08:29:14PM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2026/10/7 19:47, Qu Wenruo 写道:
>>>
>>>
>>> 在 2026/10/7 19:36, pavankumaryalagada@gmail.com 写道:
>>>> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>>>
>>>> The quota root remains on the transaction's dirty root list when
>>>> kzalloc_obj() fails and btrfs_quota_enable() releases it without
>>>> aborting the transaction. Later add_root_to_dirty_list() then accesses
>>>> the freed dirty_list, causing a slab-use-after-free.
>>>>
>>>> Abort the transaction on allocation failure to clean up the dirty
>>>> root before releasing the quota root.
>>>>
>>>> Reported-by: syzbot+947286c775f432b073a8@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?extid=947286c775f432b073a8
>>>> Fixes: 8d54518b5e52 ("btrfs: qgroup: pre-allocate btrfs_qgroup to
>>>> reduce GFP_ATOMIC usage")
>>>> Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>>
>>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> My bad, Sashiko exposed a valid but very rare race that the temporary
>> quota_root can be added to fs_info->dirty_cowonly_roots list.
>>
>> This requires enough subvolumes to make quota_root to be higher than level 0
>> in the first place, which is not common but definitely possible.
>>
>> Although all readers of fs_info->dirty_cowonly_roots won't be reached after
>> the transaction is aborted, there is still a very small window that another
>> thread is already holding a trans handler just after the transaction is
>> aborted and the quota root is freed.
>>
>> In that case the other thread may access the already freed quota_root
>> through fs_info->dirty_cowonly_roots->next.
>>
>> I'm afraid we need to call list_del(&quota_root->dirty_list) with proper
>> trans_lock hold during error handling.
> 
> Thanks, Qu. I understand.
> 
> I'll update the error handling to remove quota_root->dirty_list under trans_lock
> before dropping the quota root reference, based on the latest for-next. I'll send v2.

I'm exploring the idea to put such list_del() into btrfs_put_root(), 
which should handle it more gracefully and cover all other situations 
where a new root is created and dirtied.

Thanks,
Qu

> 
> Thanks,
> Pavan
> 
>>>
>>>> ---
>>>>    fs/btrfs/qgroup.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index f68b696b4bf7..42be06675c90 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -1204,6 +1204,7 @@ int btrfs_quota_enable(struct btrfs_fs_info
>>>> *fs_info,
>>>>        prealloc = kzalloc_obj(*prealloc, GFP_NOFS);
>>>>        if (!prealloc) {
>>>>            ret = -ENOMEM;
>>>> +        btrfs_abort_transaction(trans, ret);
>>>>            goto out_free_path;
>>>>        }
>>>>        qgroup = add_qgroup_rb(fs_info, prealloc, BTRFS_FS_TREE_OBJECTID);
>>>
>>
>