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

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 12:10, sashiko-bot@kernel.org wrote:
> 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.

I need to check what is missing here, as this was working...

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

It would be nice to stop repeating review comments from earlier patches...

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