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

[Brian Bunker <brian@purestorage.com>]

On 5/3/26 8:44 AM, Bart Van Assche wrote:
> scsi: core: Convert inquiry information
>
> Convert these fields to fixed-size character arrays within struct
> scsi_device and remove trailing white space at initialization time.
>
> This patch fixes a bug in the qla2xxx driver.

Thanks for the patch - picking up the qla2xxx fix as a side effect of
the conversion is a nice win, and the call-site cleanup (no more
%.Ns / strncmp) is genuinely more readable.

> +static void strip_trailing_spaces(char *s)
> +{
> +	size_t size;
> +	char *end;
> +
> +	size = strlen(s);
> +	if (!size)
> +		return;
> +
> +	end = s + size - 1;
> +	while (end >= s && isspace(*end))
> +		end--;
> +	*(end + 1) = '\0';
> +}

This changes the sysfs ABI for /sys/.../vendor, /sys/.../model and
/sys/.../rev. Today these files contain the raw 8/16/4 bytes from
the INQUIRY response, space-padded as the SCSI spec requires for
short identifiers, followed by a newline. For example, on a Pure
array today:

  $ od -x /sys/.../vendor
  0000000 5550 4552 2020 2020 000a
  0000011

i.e. "PURE    \n" - 9 bytes. After the patch the same file would
contain "PURE\n" - 5 bytes. That's spec-correct (per SPC, trailing
bytes ARE padding, not data) but it's a userspace-visible change
that's been stable for some time. The places I'd worry about are
udev rules using ATTRS{vendor}=="PURE    " (literal match, no
whitespace handling) and shell scripts comparing against the padded
form. None of those are in-tree.

> -	if ((type == TYPE_ROM) && (strncmp(sdev->vendor, "HP ", 3) == 0))
> +	if (type == TYPE_ROM && strcmp(sdev->vendor, "HP") == 0)

A few of the strncmp -> strcmp conversions narrow the match in ways
that are worth a second look even after the strip - this one matches
only literal "HP" post-strip, where the original would have matched
"HP COMPAQ" too.

> -	const char * vendor;		/* [back_compat] point into 'inquiry' ... */
> -	const char * model;		/* ... after scan; point to static string */
> -	const char * rev;		/* ... "nullnullnullnull" before scan */
> +	char vendor[9];
> +	char model[17];
> +	char rev[5];

Going back to Hannes' original suggestion - dropping
sdev->vendor / sdev->model / sdev->rev from struct scsi_device
entirely - is another way to get the qla2xxx bug out of the tree
without touching the sysfs output. The shape I had in mind is small
helpers against the (still raw, still space-padded) inquiry buffer:

  bool   scsi_device_vendor_match(struct scsi_device *sdev,
                                  const char *prefix);
  bool   scsi_device_model_match(struct scsi_device *sdev,
                                 const char *prefix);
  bool   scsi_device_rev_match(struct scsi_device *sdev,
                               const char *prefix);

  size_t scsi_device_vendor(struct scsi_device *sdev,
                            char *buf, size_t len);
  size_t scsi_device_model(struct scsi_device *sdev,
                           char *buf, size_t len);
  size_t scsi_device_rev(struct scsi_device *sdev,
                         char *buf, size_t len);

Match helpers do a bounded comparison against the inquiry bytes
under inquiry_mutex; copy helpers copy and NUL-terminate into a
caller buffer. Sysfs reads stay byte-exact (no strip); qla2xxx
becomes a small stack buffer + scsi_device_vendor() and the
over-read goes away. Scope is comparable to your conversion -
~18 call sites across ~8 files, each migrating separately - but
no userspace ABI delta and no cached pointers left in
struct scsi_device.

Happy either way - just wanted to surface the ABI concern and
remind you of the other direction before you spend more time on the
conversion patch.

Thanks,
Brian