Re: [PATCH v4 0/3] btrfs: fix a UAF where btrfs_root::dirty_list is freed but still referred

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

在 2026/10/10 03:00, Boris Burkov 写道:
> On Fri, Oct 09, 2026 at 08:43:37AM +1030, Qu Wenruo wrote:
>> [CHANGELOG]
>> v4:
>> - Also cleanup qgroup swapped blocks and dirty_log_pages
>>    Which are only released during switch_commit_roots().
>>
>> v3:
>> - Use Yalagada's v2 fix as the final UAF fix
>>    The delayed list_del_init() call inside btrfs_put_root() is not
>>    safe as another racing ioctl can grab the quota root, extending
>>    its lifespan.
>>
>> v2:
>> - Add extra patches to address Sashiko's comment
>>    * Make all root->dirty_list users to hold trans_lock
>>    * Release root->dirty_list from cur_trans->switch_commits during
>>      transaction cleanup
>>
>> The first patch is to make lock consistent when accessing
>> btrfs_root::dirty_list, btrfs_fs_info::dirty_cowonly_roots and
>> btrfs_transaction::switch_commits.
>>
>> Normally it's not a big deal as the existing lock-holding callers are
>> also holding a trans handle, thus they can not race with transaction
>> committing.
>> But the last commit will change the cleanup timing, and Sashiko is not
>> happy with that, so make it more consistent and shut Sashiko up.
> 
> As far as I can tell, it is possible but unlikely (rescan + squota?) for
> the new callsite to actually do the removal outside a trans handle. Is
> that your understanding?

That part (the first patch) is a little overkilled.

The main reason that patch is introduced is to prepare for calling 
"list_del_init(&root->dirty_list);" during btrfs_put_root().

As we have some call sites that doesn't hold a trans handler.

But later I switched back to the v2 fix from Yalagada, which does the 
manual list_del_init() call instead of relying on btrfs_put_root().

> 
> I would prefer to have the bar for "shutting sashiko up" to be at real
> bugs, even if they are sort of unlikely. If I misunderstood and it's
> fully a false alarm, I would sort of rather not make changes to satisfy
> its misconceptions.

I'll remove the first patch from the series.

Thanks for the review,
Qu

> 
> Fixes look good overall, thanks.
> Reviewed-by: Boris Burkov <boris@bur.io>
> 
>>
>> The second patch is an existing bug exposed by Sashiko, which also
>> affects the last UAF fix.
>>
>> The last one is the final UAF fix for the bug reported by syzbot.
>>
>> Qu Wenruo (2):
>>    btrfs: protect dirty_cowonly_roots, switch_commits and
>>      root->dirty_list
>>    btrfs: prevent use-after-free in btrfs_transaction::switch_commits
>>
>> Yalagada Pavan Kumar (1):
>>    btrfs: fix use-after-free on quota enable allocation failure
>>
>>   fs/btrfs/disk-io.c     | 14 ++++++++++++++
>>   fs/btrfs/qgroup.c      |  7 +++++++
>>   fs/btrfs/transaction.c | 30 ++++++++++++++++++++++++++----
>>   3 files changed, 47 insertions(+), 4 deletions(-)
>>
>> -- 
>> 2.55.0
>>
>