Re: [PATCH v5 12/27] scsi-multipath: introduce scsi_device head structure

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `smpdt` (mpath_head_template) is defined without implementing logically required callbacks, which will lead to a NULL pointer dereference.
- [High] scsi_mpath_unique_lun_id treats a 0-length Unique LUN ID as success, leading to unintended grouping of devices with empty IDs.
- [Low] The function `scsi_multipath_sysfs_group_visible` and the macro invocation `DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(scsi_multipath_sysfs)` are defined but never used.
--

--- Patch [12]: [PATCH v5 12/27] scsi-multipath: introduce scsi_device head structure ---
commit 0296ab446bccfc5a00926fcca72b8b18f69d4e30
Author: John Garry <[email protected]>
Subject: scsi-multipath: introduce scsi_device head structure

Introduce a scsi_device head structure - scsi_mpath_head - to manage
multipathing for a scsi_device. This is similar to nvme_ns_head structure.
A list of scsi_mpath_head structures is managed to lookup for matching
multipathed scsi_device's.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 947f3e2103e6b..5d19ab6a89eb5 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -70,6 +74,55 @@ static int scsi_mpath_unique_lun_id(struct scsi_device *sdev)

[Severity: High]
Does this function mistakenly treat a 0-length Unique LUN ID as success?

Looking at scsi_mpath_unique_lun_id() in drivers/scsi/scsi_multipath.c:

	ret = scsi_vpd_lun_id(sdev, scsi_mpath_dev->device_id_str,
				SCSI_MPATH_DEVICE_ID_LEN);
	if (ret < 0)
		return ret;

If a SCSI device responds with a valid VPD page 0x83 but a 0-length SCSI name
string, scsi_vpd_lun_id() returns 0. Since this code only checks if (ret < 0),
it will accept 0 as success.

This could cause the multipath subsystem to incorrectly group distinct devices
with empty IDs together, as strncmp("", "", 256) will match, routing I/O
intended for one disk to another entirely separate disk.

>  	return 0;
>  }
>  
> +static void scsi_mpath_head_release(struct device *dev)
[ ... ]
> +static bool scsi_multipath_sysfs_group_visible(struct kobject *kobj)
> +{
> +	return true;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(scsi_multipath_sysfs)

[Severity: Low]
Are this function and macro invocation needed?

They appear to be defined but never used to gate an attribute group in this
patch.

> +
> +static const struct attribute_group *scsi_mpath_device_groups[] = {
[ ... ]
> @@ -89,6 +142,74 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +static struct mpath_head_template smpdt = {
> +};

[Severity: High]
Can this empty template cause a NULL pointer dereference?

Because smpdt is defined as an empty struct, all its function pointers are
NULL. The multipath core unconditionally invokes template callbacks (such as
is_disabled and is_optimized) without checking for NULL.

When a multipath SCSI device is configured and processed, invoking these
callbacks could trigger a kernel panic.

> +
> +static struct scsi_mpath_head *scsi_mpath_alloc_head(char *vpd_id)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head;
[ ... ]
> +	if (mpath_head_init(&scsi_mpath_head->mpath_head))
> +		goto out_free;
> +	scsi_mpath_head->mpath_head.mpdt = &smpdt;

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