Re: [PATCH v3 04/17] scsi-multipath: support iopolicy

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Newly allocated multipath heads ignore the module parameter default for `multipath_iopolicy`.
- [Medium] Lack of synchronization in `scsi_mpath_device_iopolicy_store()` allows concurrent writes to race and corrupt state.
- [Low] The patch calls several undefined core `mpath_` functions and uses undefined types, causing build failures.

Pre-existing issues:
- [Critical] The error path in `scsi_mpath_alloc_head()` performs manual cleanup after `put_device()`, leading to double-free bugs.
- [High] If `device_add()` fails in `scsi_mpath_dev_alloc()`, the error cleanup path crashes due to uninitialized list management and invalid device unregistration.
--

commit dcaa6fb2d7264e46f58295ab1af4b1021442b793
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: support iopolicy

Add support to set the multipath iopolicy. The iopolicy member is per 
scsi_mpath_head structure. A module param is added so that the default 
iopolicy may be set.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index cb433a028dbff..6159803d4cbb8 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -95,8 +112,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, count))
> +		return -EINVAL;

[Severity: Low]
Are the functions mpath_iopolicy_store(), mpath_clear_paths(), and
mpath_schedule_requeue_work() defined anywhere in the kernel tree?
It looks like calling these will cause build failures.

> +
> +	mpath_clear_paths(mpath_head);
> +	mpath_schedule_requeue_work(mpath_head);

[Severity: Medium]
Are these sequential operations safe from race conditions when called
without holding any locks?

If multiple threads write to the sysfs attribute concurrently, could they
race and potentially duplicate path clearing or corrupt the I/O policy
state?

> +	return count;
> +}
[ ... ]
> @@ -201,7 +246,9 @@ 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->mpath_head.iopolicy = &scsi_mpath_head->iopolicy;

[Severity: Medium]
Will newly allocated multipath heads ignore the module parameter default
for multipath_iopolicy?

Since scsi_mpath_head is kzalloc'd, iopolicy defaults to zero
(MPATH_IOPOLICY_NUMA). This assignment hooks up the pointer but doesn't
initialize it with the global iopolicy module parameter.

[Severity: Critical]
This is a pre-existing issue, but does the error path in this function
cause a double-free?

If dev_set_name() fails further down in scsi_mpath_alloc_head():

	if (ret) {
		put_device(&scsi_mpath_head->dev);
		goto out_free_ida;
	}

The put_device() call drops the refcount to 0 and synchronously invokes
scsi_mpath_head_release(), which frees the IDA slot, uninitializes the
mpath head, and calls kfree(scsi_mpath_head).

The code then jumps to out_free_ida:

out_free_ida:
	ida_free(&scsi_multipath_dev_ida, scsi_mpath_head->index);
out_put_head:
	mpath_put_head(&scsi_mpath_head->mpath_head);
out_free:
	kfree(scsi_mpath_head);

This manually repeats all of these steps, leading to a double-free and
use-after-free memory corruption.

[Severity: High]
This is a pre-existing issue, but can the device addition error path lead
to a NULL pointer dereference?

If device_add() fails in scsi_mpath_dev_alloc(), it jumps to out_put_head
and calls scsi_mpath_put_head(), which invokes scsi_mpath_free_head():

static void scsi_mpath_free_head(struct kref *ref)
{
	...
	mutex_lock(&scsi_mpath_heads_lock);
	list_del_init(&scsi_mpath_head->entry);

Because device_add() failed, the object was never added to the list via
list_add_tail(). Since scsi_mpath_head was allocated with kzalloc(),
entry.next and entry.prev are NULL, and list_del_init() will dereference
these NULL pointers.

Additionally, device_unregister() is called on a device that failed
device_add(), which violates driver core API expectations.

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