Re: [PATCH v7 1/5] scsi: core: Protect INQUIRY sysfs attributes with mutex

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: sysfs modalias and uevent missing inquiry_mutex protection
--

commit 81f6a8643e20a98783acfb264a3488a0b0ad95ac
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Protect INQUIRY sysfs attributes with mutex
    
All INQUIRY-derived sysfs attributes (type, scsi_level, vendor, model,
rev, cdl_supported, and the binary inquiry attribute) read data that
can be updated during device rescan. These reads must be protected
against concurrent updates.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fe54792ed69e4..a20707f6f1ac7 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -638,21 +638,30 @@ sdev_show_##field (struct device *dev, struct device_attribute *attr,	\
>  
>  /*
>   * sdev_rd_attr: macro to create a function and attribute variable for a
> - * read only field.
> + * read-only field. inquiry_mutex protects INQUIRY-derived fields against
> + * concurrent updates during device rescan.
>   */
>  #define sdev_rd_attr(field, format_string)				\
> -	sdev_show_function(field, format_string)			\
> -static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
> +static ssize_t								\
> +sdev_show_##field(struct device *dev, struct device_attribute *attr,	\
> +		  char *buf)						\
> +{									\
> +	struct scsi_device *sdev = to_scsi_device(dev);			\
> +									\
> +	guard(mutex)(&sdev->inquiry_mutex);				\
> +	return sysfs_emit(buf, format_string, sdev->field);		\
> +}									\
> +static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL)
>  
>  /*
>   * Create the actual show/store functions and data structures.
>   */
> -sdev_rd_attr (type, "%d\n");
> -sdev_rd_attr (scsi_level, "%d\n");
> -sdev_rd_attr (vendor, "%.8s\n");
> -sdev_rd_attr (model, "%.16s\n");
> -sdev_rd_attr (rev, "%.4s\n");
> -sdev_rd_attr (cdl_supported, "%d\n");
> +sdev_rd_attr(type, "%d\n");

[Severity: Medium]
The commit states all INQUIRY-derived sysfs attributes, including type, must
be protected by inquiry_mutex against concurrent updates.

However, shouldn't sdev_show_modalias() and scsi_bus_uevent() also be updated?
They both read sdev->type locklessly to construct the modalias string:

drivers/scsi/scsi_sysfs.c:sdev_show_modalias() {
    ...
    sdev = to_scsi_device(dev);
    return snprintf (buf, 20, SCSI_DEVICE_MODALIAS_FMT "\n", sdev->type);
}

drivers/scsi/scsi_sysfs.c:scsi_bus_uevent() {
    ...
    sdev = to_scsi_device(dev);
    add_uevent_var(env, "MODALIAS=" SCSI_DEVICE_MODALIAS_FMT, sdev->type);
    ...
}

Can concurrent device rescans cause these functions to read an inconsistent
or transient sdev->type?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014220014.94330-1-brian@purestorage.com?part=1