Re: [PATCH] btrfs: properly cleanup replace_task when the replace failed to start

在 2026/8/13 07:11, Qu Wenruo 写道:
> 
> 
> 在 2026/8/12 21:33, Jeff Layton 写道:
>> I ran this through some LLM review and it seems to think that this fix
>> isn't complete. Pasting the review comments verbatim below:
>>
>> On Mon, 2026-08-10 at 17:21 +0930, Qu Wenruo wrote:
>>
>>> In the function btrfs_dev_replace_start(), we have several error paths
>>> that assigns replace_start without reverting it back to NULL.
>>
>> This isn't a bug, but should replace_start be replace_task here?
>>
>>> But since dev_replace->rwsem is incorrectly updated, a process
>>> triggering the update will no longer be protected from dev-replace's
>>> device list modification, thus later IO can get stale device info,
>>> triggering things like use-after-free.
>>
>> The thing left incorrectly updated is dev_replace->replace_task, not
>> dev_replace->rwsem, right?
> 
> Oh no, I updated the commit message without review it again, now it's 
> all kinds of wrong names...
> 
>>
>>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>>> index 72cba7fed942..5fc1dec88fb2 100644
>>> --- a/fs/btrfs/dev-replace.c
>>> +++ b/fs/btrfs/dev-replace.c
>>> @@ -633,7 +633,6 @@ static int btrfs_dev_replace_start(struct 
>>> btrfs_fs_info *fs_info,
>>>           goto leave;
>>>
>>>       down_write(&dev_replace->rwsem);
>>> -    dev_replace->replace_task = current;
>>>       switch (dev_replace->replace_state) {
>>>       case BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED:
>>>       case BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED:
>>> @@ -647,6 +646,7 @@ static int btrfs_dev_replace_start(struct 
>>> btrfs_fs_info *fs_info,
>>>           goto leave;
>>>       }
>>>
>>> +    dev_replace->replace_task = current;
>>>       dev_replace->cont_reading_from_srcdev_mode = read_src;
>>>       dev_replace->srcdev = src_device;
>>>       dev_replace->tgtdev = tgt_device;
>>> @@ -693,6 +693,7 @@ static int btrfs_dev_replace_start(struct 
>>> btrfs_fs_info *fs_info,
>>>               BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED;
>>>           dev_replace->srcdev = NULL;
>>>           dev_replace->tgtdev = NULL;
>>> +        dev_replace->replace_task = NULL;
>>>           up_write(&dev_replace->rwsem);
>>>           goto leave;
>>>       }
>>
>> Both of the paths handled here are inside btrfs_dev_replace_start().  Is
>> the same reset needed in btrfs_dev_replace_finishing()?  That function
>> only clears replace_task on the path that completes the swap:
>>
>> fs/btrfs/dev-replace.c:btrfs_dev_replace_finishing() {
>>     ...
>>     list_add(&tgt_device->dev_alloc_list, &fs_devices->alloc_list);
>>     fs_devices->rw_devices++;
>>
>>     dev_replace->replace_task = NULL;
>>     up_write(&dev_replace->rwsem);
>>     ...
>> }
>>
>> The scrub_ret error path returns earlier.  It clears srcdev and tgtdev 
>> and
>> moves the state to canceled, but leaves replace_task pointing at the task
>> that started the replace:
>>
>> fs/btrfs/dev-replace.c:btrfs_dev_replace_finishing() {
>>     ...
>>     down_write(&dev_replace->rwsem);
>>     dev_replace->replace_state =
>>         scrub_ret ? BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED
>>               : BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED;
>>     dev_replace->tgtdev = NULL;
>>     dev_replace->srcdev = NULL;
>>     ...
>>     } else {
>>         if (scrub_ret != -ECANCELED)
>>             btrfs_err(fs_info, ...);
>> error:
>>         up_write(&dev_replace->rwsem);
>>         ...
>>         return scrub_ret;
>>     }
>>     ...
>> }
>>
>> An ordinary "btrfs replace cancel" ends up there:
>>
>> btrfs_dev_replace_cancel()
>>      btrfs_scrub_cancel()
>>
>> btrfs_dev_replace_start()
>>      ret = btrfs_scrub_dev()            -> -ECANCELED
>>      ret = btrfs_dev_replace_finishing(fs_info, ret)
>>     -> error: ... return scrub_ret
>>
>> The ioctl then returns to userspace with dev_replace->replace_task still
>> set to the task that ran the ioctl.  Isn't that the same stale pointer 
>> the
>> commit message describes for the already-started case, only reachable
>> without a second replace or an allocation failure?
> 
> You're right! This is indeed a missing call site.
> 
>>
>> The two other early returns in btrfs_dev_replace_finishing(), the
>> btrfs_start_delalloc_roots() failure and the btrfs_start_transaction()
>> failure in the commit loop, leave it set as well.
>>
>> There is a second effect once replace_task is stale but still compares
>> equal to a live task.  btrfs_map_block() reads it twice:
>>
>> fs/btrfs/volumes.c:btrfs_map_block() {
>>     ...
>>     if (dev_replace->replace_task != current)
>>         down_read(&dev_replace->rwsem);
>>
>>     dev_replace_is_ongoing = btrfs_dev_replace_is_ongoing(dev_replace);
>>     /*
>>      * Hold the semaphore for read during the whole operation, write is
>>      * requested at commit time but must wait.
>>      */
>>     if (!dev_replace_is_ongoing && dev_replace->replace_task != current)
>>         up_read(&dev_replace->rwsem);
>>     ...
>> }
>>
>> If a later btrfs_dev_replace_start() assigns replace_task between those
>> two reads, the first test skips down_read() while the second one runs
>> up_read().  Can that release an rwsem this task never acquired?
> 
> This is another missing point, although it may be a little tricky to fix.
> 
> If we take the rwsem just to read replace_task, it will greatly reduce 
> the concurrency of btrfs_map_block(), even if there is no running replace.
> 
> I'll replace all those existing replace_task checks, and only do one 
> comparison and save the result, so as long as we take the rwsem, it will 
> always be released.

It turns out that, this is a complex false alerts.

There are several things here to ensure we won't get split lock/unlock 
behaviors:

- For tasks unrelated to replace
   They always get the replace_task mismatching current, and since the
   current task is already inside btrfs_map_block(), there is no way that
   the current task can run code to reassign replace_task.

   So even if replace_task is changed, it will never be changed to the
   current task, so we will still unlock the rwsem.

- For task which started the replace
   In that case, we are the task which called btrfs_dev_replace_start().

   However the only call site that re-assign replace_task is either
   btrfs_dev_replace_start() or btrfs_dev_replace_finishing() that I'm
   fixing.

   As long as btrfs_dev_replace_start() is fixed so that replace_task is
   not assigned to another task, we're safe.

   As btrfs_dev_replace_finishing() can only be reached by the running
   replace, which is the current task.

> 
> 
> Although I'm not sure if we should introduce one extra spinlock to 
> protect btrfs_dev_replace_is_ongoing().
> As in that function we're checking two members, is_valid and state 
> without any protection.
> 
> Thanks,
> Qu
>