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

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

[Qu Wenruo]

In the function btrfs_dev_replace_start(), we have several error paths
that assigns replace_start without reverting it back to NULL.

There are two involved error paths:

- There is already a running dev-replace
  Then replace_task is over-written to the current task.
  This is the one with long running effect.

- The btrfs_start_transaction() call failed
  This is much harder to hit though.

This can result the replace_task check inside btrfs_map_block() to be
incorrectly triggered, not taking dev_replace->rwsem.

Normally that replace_task check is to protect regular IOs from racing
with dev-replace, which will modify the device list.

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.

Fix the problem by:

- Moving the replace_task assignment after replace_state check

- Reset replace_task to NULL if btrfs_start_transaction() failed

This is reported by Sashiko, which found the existing bug during review
of another patch, and since the bug is an existing one, it's not shown in
the summary, but only in the detail page.

Link: https://sashiko.dev/#/patchset/tencent_853134544C3CE88A219EEB21346E2510D308%40qq.com
Fixes: 8cca35cb29f8 ("btrfs: don't take dev_replace rwsem on task already holding it")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/dev-replace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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;
 	}
-- 
2.54.0

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

[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?

> 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?

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?

-- 
Jeff Layton <jlayton@kernel.org>

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

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


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

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

[Qu Wenruo]

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