Re: [PATCH v3 2/6] scsi: Protect INQUIRY sysfs attributes with mutex

[Bart Van Assche <bvanassche@acm.org>]

On 4/29/26 3:49 PM, Brian Bunker wrote:
> 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.
> 
> Use the existing inquiry_mutex to protect access to these sysfs
> attributes. This ensures that userspace always sees consistent INQUIRY
> data, even if a rescan is updating the buffer concurrently.
> 
> Replace the sdev_rd_attr macro with two new helpers,
> sdev_rd_inquiry_attr_int and sdev_rd_inquiry_attr_str, which generate
> the show functions for INQUIRY-derived integer and string fields and
> take the inquiry_mutex around the field access.
> 
> This is preparatory work for adding INQUIRY data update support during
> device rescan operations.
> 
> Signed-off-by: Brian Bunker <brian@purestorage.com>
> Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>

Shouldn't your signed-off-by come last since you are the person posting
this patch?

> -#define sdev_rd_attr(field, format_string)				\
> -	sdev_show_function(field, format_string)			\
> -static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL);
> +#define sdev_rd_inquiry_attr_str(field, accessor, len)			\
> +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);				\
> +	if (sdev->inquiry)						\
> +		return sysfs_emit(buf, "%.*s\n", len,			\
> +				  accessor(sdev->inquiry));		\
> +	return sysfs_emit(buf, "\n");					\
> +}									\
> +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_inquiry_attr_int(type);
> +sdev_rd_inquiry_attr_int(scsi_level);
> +sdev_rd_inquiry_attr_int(cdl_supported);
> +sdev_rd_inquiry_attr_str(vendor, scsi_inq_vendor, SCSI_INQ_VENDOR_LEN);
> +sdev_rd_inquiry_attr_str(model, scsi_inq_product, SCSI_INQ_PRODUCT_LEN);
> +sdev_rd_inquiry_attr_str(rev, scsi_inq_revision, SCSI_INQ_REVISION_LEN);

I'm not aware of any other example of "accessor" functions in the Linux
kernel for character data that is not '\0' terminated. Accessor
functions like scsi_inq_vendor() only hide the offset of the vendor data
but not the length. I prefer that all accessor functions would be 
removed for strings that are not '\0'-terminated. sdev_show_##field()
can be modified to accept an offset and a length instead of an accessor
function and a length.

Thanks,

Bart.