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

在 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.


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