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

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?

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14