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

On 29/07/2026 15:07, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] The struct mpath_head_template 'smpdt' is entirely empty, lacking implementations for logically required callbacks, leading to NULL pointer dereferences.
> - [Low] Missing READ_ONCE() when reading the global `iopolicy` variable, leading to a data race.
> --
> 
> --- Patch [14]: [PATCH v6 14/27] scsi-multipath: support iopolicy ---
> commit f1795dc3e5eb8bb57ed7783aab4a6563ae5c3b62
> Author: John Garry <[email protected]>
> 
> scsi-multipath: support iopolicy
> 
> Add support to set the multipath iopolicy. The iopolicy member is per
> scsi_mpath_head structure, and 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 1ec7bf1835de0..593f94aac70e9 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);
>> +}
>> +
>> +static int scsi_mpath_get_iopolicy_param(char *buf, const struct kernel_param *kp)
>> +{
>> +	return mpath_get_iopolicy(buf, iopolicy);
> 
> [Severity: Low]
> Is a READ_ONCE() missing here when reading the global iopolicy variable?
> 
> mpath_set_iopolicy() updates the variable using WRITE_ONCE(). Concurrent
> reads here could lead to a data race and trigger KCSAN warnings.
> 
>> +}
>> +
>> +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'");
>> +
> 
> [ ... ]
> 
>> @@ -204,6 +254,8 @@ static struct scsi_mpath_head *scsi_mpath_alloc_head(char *vpd_id)
>>   	if (mpath_head_init(&scsi_mpath_head->mpath_head))
>>   		goto out_free;
>>   	scsi_mpath_head->mpath_head.mpdt = &smpdt;
> 
> [Severity: Critical]
> Since the scsi_mpath_head is initialized with smpdt here, and smpdt appears
> to be an empty template, could this lead to NULL pointer dereferences in the
> core multipath library?
> 

It is filled in later. And that is not even related to this patch.

> drivers/scsi/scsi_multipath.c:
> static struct mpath_head_template smpdt = {
> };
> 
> The core code unconditionally dereferences these callbacks. For example, during
> path validation:
> 
> lib/multipath.c:mpath_path_is_disabled() {
>      ...
>      return mpath_head->mpdt->is_disabled(mpath_device);
> }
> 
>> +	scsi_mpath_head->iopolicy = iopolicy;
> 
> [Severity: Low]
> Would it be better to also use READ_ONCE() here when reading the global
> iopolicy variable during initialization?

This won't make a difference, as iopolicy is only written once at module 
init time.

> 
>> +	scsi_mpath_head->mpath_head.iopolicy = &scsi_mpath_head->iopolicy;
>>   
>>   	strscpy(scsi_mpath_head->vpd_id, vpd_id,
>>   		SCSI_MPATH_DEVICE_ID_LEN);
>