Re: [PATCH v7 14/27] scsi-multipath: support iopolicy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] A TOCTOU race between updating the I/O policy and lockless path resolution can cause a path evaluated under the old policy to be permanently cached.
--

commit 23d70dc129a3932ccedd2e25833214d5d4fedd20
Author: John Garry <john.garry@linux.dev>

scsi-multipath: support iopolicy

Add support to set the multipath iopolicy. The default policy can be set via
a module parameter, and it can be dynamically updated via a sysfs attribute.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 1c04947d906ea..7164bc61e65e1 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c

[ ... ]

> @@ -99,8 +116,41 @@ static ssize_t scsi_mpath_device_vpd_id_show(struct device *dev,
>  }
>  static DEVICE_ATTR(vpd_id, S_IRUGO, scsi_mpath_device_vpd_id_show, NULL);
>  
> +static ssize_t scsi_mpath_device_iopolicy_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head =
> +		container_of(dev, struct scsi_mpath_head, dev);
> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> +
> +	if (!mpath_iopolicy_store(&scsi_mpath_head->iopolicy, buf))
> +		return -EINVAL;
> +
> +	mpath_clear_paths(mpath_head);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Is there a race condition here between updating the I/O policy and lockless
path resolution?

If an administrator writes to the iopolicy sysfs attribute while an I/O
request is concurrently finding a path, this sequence appears possible:

Thread 1 (I/O) evaluates the path under the current policy (e.g.,
round-robin) and selects a path.

Thread 2 (sysfs) updates the policy to NUMA here in
scsi_mpath_device_iopolicy_store(), and calls mpath_clear_paths(), setting
current_path[node] = NULL.

Thread 1 finishes path selection and executes this in __mpath_find_path()
located in lib/multipath.c:

	if (found)
		rcu_assign_pointer(mpath_head->current_path[node], found);

This would overwrite the NULL with the path chosen under the old policy.
Subsequent I/Os calling mpath_numa_path() would see a non-NULL
current_path[node] and use it directly, permanently caching a stale path
under the new policy.

Does mpath_clear_paths() need synchronization against concurrent path
resolutions to prevent caching paths evaluated under the old policy?

> +	mpath_schedule_requeue_work(mpath_head);
> +	/*
> +	 * Ensure requeue work completes, as this work could run later when
> +	 * the mpath_head is gone.
> +	 */
> +	flush_work(&mpath_head->requeue_work);
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901104526.3850621-1-john.g.garry@oracle.com?part=14