Re: [PATCH v9 12/29] scsi-multipath: introduce basic SCSI device support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] scsi: CONFIG_SCSI_MULTIPATH depends on SCSI_MOD instead of SCSI
- [Low] scsi: duplicate MODULE_DESCRIPTION and MODULE_LICENSE in
  scsi_multipath.c
- [Low] scsi-multipath: off-by-one truncation check in
  scsi_mpath_unique_lun_id()
--

commit 006413118882829d81f9e30417cb6f7346390426
Author: John Garry <john.garry@linux.dev>

scsi-multipath: introduce basic SCSI device support

This commit introduces struct scsi_mpath_device, the scsi_mod.multipath
module parameter, and per-device multipath allocation and release hooks
during SCSI LUN scanning and device teardown.

> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 90b65bebec703..2491fdf6f563e 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -76,6 +76,16 @@ config SCSI_LIB_KUNIT_TEST
>  
>  	  If unsure say N.
>  
> +config SCSI_MULTIPATH
> +	bool "SCSI multipath support (EXPERIMENTAL)"
> +	depends on SCSI_MOD
> +	select LIBMULTIPATH
> +	help
> +	  This option enables support for native SCSI multipath support for
> +	  SCSI host.
> +
> +	  If unsure say N.
> +

[Severity: Low]
Should SCSI_MULTIPATH depend on SCSI instead of SCSI_MOD?

SCSI_MOD defaults to y when SCSI=n and BLOCK=y:

drivers/scsi/Kconfig:
    config SCSI_MOD
    	tristate
    	default y if SCSI=n || SCSI=y
    	default m if SCSI=m
    	depends on BLOCK

When CONFIG_SCSI=n and CONFIG_BLOCK=y, CONFIG_SCSI_MULTIPATH=y can be
enabled, which selects CONFIG_LIBMULTIPATH=y while skipping scsi_mod.o and
drivers/scsi/scsi_multipath.o.

[ ... ]

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> new file mode 100644
> index 0000000000000..97f7513b47122
> --- /dev/null
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -0,0 +1,150 @@

[ ... ]

> +static int scsi_mpath_unique_lun_id(struct scsi_device *sdev)
> +{
> +	struct scsi_mpath_device *scsi_mpath_dev = sdev->scsi_mpath_dev;
> +	int ret;
> +
> +	ret = scsi_vpd_lun_id(sdev, scsi_mpath_dev->device_id_str,
> +				SCSI_MPATH_DEVICE_ID_LEN);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret == 0)
> +		return -EINVAL;
> +	else if (ret > SCSI_MPATH_DEVICE_ID_LEN)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: Low]
Should the truncation check test ret >= SCSI_MPATH_DEVICE_ID_LEN instead of
ret > SCSI_MPATH_DEVICE_ID_LEN?

In scsi_vpd_lun_id(), a T10 Vendor ID designator with 252 or more bytes is
clamped to id_len - 4 (252) and formatted with snprintf(id, id_len,
"t10.%*pE", ...), which truncates device_id_str at 255 characters plus the
NUL terminator and returns 256 (SCSI_MPATH_DEVICE_ID_LEN). Testing
ret > SCSI_MPATH_DEVICE_ID_LEN accepts that truncated identifier.

[ ... ]

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("scsi_multipath");

[Severity: Low]
Does linking scsi_multipath.o into scsi_mod.o emit duplicate MODULE_LICENSE
and conflicting MODULE_DESCRIPTION entries in scsi_mod's .modinfo section?
drivers/scsi/scsi.c already defines MODULE_DESCRIPTION("SCSI core") and
MODULE_LICENSE("GPL") for scsi_mod.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=12