Re: [PATCH] btrfs: fix use-after-free in mark_block_group_to_copy

[Hongling Zeng <zhongling0719@126.com>]

在 2026年08月17日 11:09, Qu Wenruo 写道:
>
>
> 在 2026/8/17 12:15, Hongling Zeng 写道:
>>    Thanks for the review.
>>
>>    This patch was developed with assistance from Claude AI.
>>
>>    The AI helped with:
>>    - Analyzing the code paths to understand the use-after-free scenario
>>    - Identifying potential deadlock risks with different locking 
>> approaches
>>    - Cross-referencing similar locking patterns in send.c and backref.c
>>    - Drafting the commit message
>>
>>    I modified the code to add the commit_root_sem locking myself, 
>> reviewed and
>>    tested the changes before submission.
>>
>>    Apologies for not disclosing AI assistance earlier. I will ensure 
>> to do so in
>>    future submissions.
>>
>>    Best regards,
>>    Hongling Zeng
>>
>> 在 2026年08月17日 09:46, Hongling Zeng 写道:
>>> mark_block_group_to_copy() iterates over the commit root with
>>> skip_locking=true to avoid lock contention. Without holding
>>> commit_root_sem,a concurrent transaction commit can swap and free
>>> the commit root during iteration, leading to use-after-free when
>>> accessing extent buffers.
>>>
>>> The fix adds commit_root_sem locking, but CRITICALLY must only cover 
>>> the
>>> actual search period (btrfs_for_each_slot), NOT the preceding while 
>>> loop
>>> which calls btrfs_commit_transaction(). If held during that loop, we 
>>> get
>>> self-deadlock: down_read(commit_root_sem) → btrfs_commit_transaction()
>>> → switch_commit_roots() → down_write(commit_root_sem), blocking forever
>>> on our own read lock.
>>>
>>> Lock scope:
>>> - down_read() after path setup, before btrfs_for_each_slot
>>> - up_read() immediately after btrfs_for_each_slot completes
>>> - NOT held during while loop (btrfs_commit_transaction) path
>>> - NOT held on the !path error path (goto unlock before lock
>>>    acquisition)
>>>
>>> This matches the established btrfs pattern used in send.c, 
>>> backref.c, etc.:
>>> hold commit_root_sem read lock ONLY while searching commit root, never
>>> across transaction commits.
>>>
>>> Fixes: 78ce9fc269af ("btrfs: zoned: mark block groups to copy for 
>>> device-replace")
>>> Cc:stable@vger.kernel.org
>>> Signed-off-by: Hongling Zeng<zenghongling@kylinos.cn>
>>> ---
>>>   fs/btrfs/dev-replace.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>> index dc0834f920c3..734807bf48b3 100644
>>> --- a/fs/btrfs/dev-replace.c
>>> +++ b/fs/btrfs/dev-replace.c
>>> @@ -499,6 +499,7 @@ static int mark_block_group_to_copy(struct 
>>> btrfs_fs_info *fs_info,
>>>       key.type = BTRFS_DEV_EXTENT_KEY;
>>>       key.offset = 0;
>>>   +    down_read(&fs_info->commit_root_sem);
>
> And there is already path::need_commit_sem, which does a smaller 
> critical section than holding it manually.
   Thanks for the review.

   I've updated the patch to use path->need_commit_sem as suggested. This is
   indeed a better approach than manual commit_root_sem locking.

   V2 patch is attached.

   Best regards,
   Hongling Zeng

>
>>>       btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
>>>           struct extent_buffer *leaf = path->nodes[0];
>>>   @@ -525,6 +526,8 @@ static int mark_block_group_to_copy(struct 
>>> btrfs_fs_info *fs_info,
>>>       if (iter_ret < 0)
>>>           ret = iter_ret;
>>>   +    up_read(&fs_info->commit_root_sem);
>>> +
>>>       btrfs_free_path(path);
>>>   unlock:
>>>       mutex_unlock(&fs_info->chunk_mutex);
>>