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

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 <[email protected]>