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

在 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:[email protected]
>> Signed-off-by: Hongling Zeng<[email protected]>
>> ---
>>   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.

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