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

[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@xxxxxxxxx 写道:
> > > From: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > 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@xxxxxxxxx>
> > 
> > Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
> 
> 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.

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);
> > 
>