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

On 27/07/2026 16:15, [email protected] wrote:
> 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.

hmmm ... in checking scsi_vpd_lun_id(), I don't see how the length could 
ever be zero

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

used later

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

they are added later

> 
>> +
>> +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;
>