Re: [PATCH v4 15/28] scsi-multipath: support iopolicy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Use-after-free on requeue_work during device teardown after sysfs store.
- [Low] Data race on global iopolicy module parameter.

Pre-existing issues:
- [High] TOCTOU race exposing uninitialized sysfs attributes to userspace.
--

--- Patch [15]: [PATCH v4 15/28] scsi-multipath: support iopolicy ---
commit d1b32244eafe032033fe9c1b3cb04bb7a990a08a
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: support iopolicy

This commit adds support for configuring the multipath I/O policy. It
introduces a new iopolicy member to the scsi_mpath_head structure and adds a
module parameter to set the default policy.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 59d1c0b2d3441..6228ff4be4ff7 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -61,6 +61,23 @@ static const struct kernel_param_ops multipath_param_ops = {
>  module_param_cb(multipath, &multipath_param_ops, &scsi_multipath, 0444);
>  MODULE_PARM_DESC(multipath, "turn on native multipath support, options: on, off, always");
>  
> +static enum mpath_iopolicy_e iopolicy = MPATH_IOPOLICY_NUMA;
> +
> +static int scsi_mpath_set_iopolicy_param(const char *val, const struct kernel_param *kp)
> +{
> +	return mpath_set_iopolicy(val, &iopolicy);
> +}

[Severity: Low]
Does this introduce a data race on the global module parameter?

The global iopolicy variable is updated locklessly here, and read locklessly
when assigning it later in scsi_mpath_alloc_head(). Should this use
READ_ONCE() and WRITE_ONCE() to prevent KCSAN warnings and ensure proper
concurrency hygiene?

> +
> +static int scsi_mpath_get_iopolicy_param(char *buf, const struct kernel_param *kp)
> +{
> +	return mpath_get_iopolicy(buf, iopolicy);
> +}
> +
> +module_param_call(multipath_iopolicy, scsi_mpath_set_iopolicy_param,
> +		scsi_mpath_get_iopolicy_param, &iopolicy, 0644);
> +MODULE_PARM_DESC(multipath_iopolicy,
> +	"Default multipath I/O policy; 'numa' (default), 'round-robin' or 'queue-depth'");
> +

[ ... ]

> @@ -96,8 +113,36 @@ 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);
> +	mpath_schedule_requeue_work(mpath_head);

[Severity: High]
Can mpath_schedule_requeue_work() trigger a use-after-free during device
teardown?

If a sysfs write occurs concurrently with device removal, the asynchronous
requeue work is scheduled here. The teardown path in scsi_mpath_head_release()
frees the scsi_mpath_head structure without calling cancel_work_sync() to
cancel this pending work.

Could the workqueue access freed memory when it executes the queued function?

> +	return count;
> +}
> +
> +static ssize_t scsi_mpath_device_iopolicy_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{

[ ... ]

> @@ -201,6 +246,8 @@ static struct scsi_mpath_head *scsi_mpath_alloc_head(void)
>  	if (mpath_head_init(&scsi_mpath_head->mpath_head))
>  		goto out_free;
>  	scsi_mpath_head->mpath_head.mpdt = &smpdt;
> +	scsi_mpath_head->iopolicy = iopolicy;
> +	scsi_mpath_head->mpath_head.iopolicy = &scsi_mpath_head->iopolicy;
>  
>  	scsi_mpath_head->index = ida_alloc(&scsi_multipath_dev_ida, GFP_KERNEL);
>  	if (scsi_mpath_head->index < 0)

[Severity: High]
This is a pre-existing issue, but does this allocation path contain a race
condition exposing uninitialized sysfs attributes to userspace?

Later in scsi_mpath_alloc_head(), device_add() is called which registers the
device and fires a KOBJ_ADD uevent. However, the caller scsi_mpath_dev_alloc()
only populates the scsi_mpath_head->vpd_id data after scsi_mpath_alloc_head()
returns.

Could userspace udev rules react to the new device and read an empty vpd_id
string before it is fully initialized?

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