[PATCH 0/2] Rework the struct scsi_device inquiry information

[PATCH 0/2] Rework the struct scsi_device inquiry information

[Bart Van Assche]

Hi Martin,

The vendor, model and rev members in struct scsi_device are fixed-length
strings that are not NUL-terminated. This patch converts these members into
NUL-terminated character arrays. This makes it less error-prone to deal with
these structure members. The patches in this series have been implemented such
that the number of lines changed and the risk for regressions is minimized.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (2):
  scsi: core, target: Move three constants into <scsi/scsi_common.h>
  scsi: core: Convert inquiry information

 drivers/hwmon/drivetemp.c         |  5 +----
 drivers/scsi/scsi_scan.c          | 12 ++++++------
 include/scsi/scsi_common.h        |  4 ++++
 include/scsi/scsi_device.h        |  7 ++++---
 include/target/target_core_base.h |  5 +----
 5 files changed, 16 insertions(+), 17 deletions(-)

[PATCH 1/2] scsi: core, target: Move three constants into <scsi/scsi_common.h>

[Bart Van Assche]

Prepare for using these constants in the SCSI core.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/scsi/scsi_common.h        | 4 ++++
 include/target/target_core_base.h | 5 +----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index fb58715fac86..f242040ff9d7 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -10,6 +10,10 @@
 #include <uapi/linux/pr.h>
 #include <scsi/scsi_proto.h>
 
+#define INQUIRY_VENDOR_LEN	8
+#define INQUIRY_MODEL_LEN	16
+#define INQUIRY_REVISION_LEN	4
+
 enum scsi_pr_type {
 	SCSI_PR_WRITE_EXCLUSIVE			= 0x01,
 	SCSI_PR_EXCLUSIVE_ACCESS		= 0x03,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 9a0e9f9e1ec4..002b0fc57587 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -8,6 +8,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/semaphore.h>     /* struct semaphore */
 #include <linux/completion.h>
+#include <scsi/scsi_common.h>
 
 #define TARGET_CORE_VERSION		"v5.0"
 
@@ -46,10 +47,6 @@
 /* Used by transport_get_inquiry_vpd_device_ident() */
 #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN	254
 
-#define INQUIRY_VENDOR_LEN			8
-#define INQUIRY_MODEL_LEN			16
-#define INQUIRY_REVISION_LEN			4
-
 /* Attempts before moving from SHORT to LONG */
 #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD	3
 #define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT	3  /* In milliseconds */

[PATCH 2/2] scsi: core: Convert inquiry information

[Bart Van Assche]

Currently the vendor, model, and revision members of struct scsi_device
are pointers to fixed-length strings that are not NUL-terminated.
Fixed-precision format specifiers (e.g., "%.8s") are required whenever
they are printed and strncmp() must be used to compare these fields.
This is error-prone.

Convert these fields to fixed-size character arrays within struct
scsi_device. Remove an !sdev->model check because sdev->model is now
guaranteed not to be NULL.

This patch fixes a bug in the qla2xxx driver. It makes the following
code safe:

		if (state_flags & BIT_4)
			scmd_printk(KERN_WARNING, cp,
			    "Unsupported device '%s' found.\n",
			    cp->device->vendor);

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/hwmon/drivetemp.c  |  5 +----
 drivers/scsi/scsi_scan.c   | 12 ++++++------
 include/scsi/scsi_device.h |  7 ++++---
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
index 002e0660a0b8..efe8b229bdbe 100644
--- a/drivers/hwmon/drivetemp.c
+++ b/drivers/hwmon/drivetemp.c
@@ -306,13 +306,10 @@ static bool drivetemp_sct_avoid(struct drivetemp_data *st)
 	struct scsi_device *sdev = st->sdev;
 	unsigned int ctr;
 
-	if (!sdev->model)
-		return false;
-
 	/*
 	 * The "model" field contains just the raw SCSI INQUIRY response
 	 * "product identification" field, which has a width of 16 bytes.
-	 * This field is space-filled, but is NOT NULL-terminated.
+	 * This field is space-filled and NUL-terminated.
 	 */
 	for (ctr = 0; ctr < ARRAY_SIZE(sct_avoid_models); ctr++)
 		if (!strncmp(sdev->model, sct_avoid_models[ctr],
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index ef22a4228b85..6c3a5d451c1d 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -292,9 +292,9 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	if (!sdev)
 		goto out;
 
-	sdev->vendor = scsi_null_device_strs;
-	sdev->model = scsi_null_device_strs;
-	sdev->rev = scsi_null_device_strs;
+	strscpy(sdev->vendor, scsi_null_device_strs);
+	strscpy(sdev->model, scsi_null_device_strs);
+	strscpy(sdev->rev, scsi_null_device_strs);
 	sdev->host = shost;
 	sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
 	sdev->id = starget->id;
@@ -905,9 +905,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (sdev->inquiry == NULL)
 		return SCSI_SCAN_NO_RESPONSE;
 
-	sdev->vendor = (char *) (sdev->inquiry + 8);
-	sdev->model = (char *) (sdev->inquiry + 16);
-	sdev->rev = (char *) (sdev->inquiry + 32);
+	strscpy(sdev->vendor, sdev->inquiry + 8);
+	strscpy(sdev->model, sdev->inquiry + 16);
+	strscpy(sdev->rev, sdev->inquiry + 32);
 
 	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
 	if (sdev->is_ata) {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 9c2a7bbe5891..029f5115b2ea 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -7,6 +7,7 @@
 #include <linux/workqueue.h>
 #include <linux/blk-mq.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_common.h>
 #include <linux/atomic.h>
 #include <linux/sbitmap.h>
 
@@ -137,9 +138,9 @@ struct scsi_device {
 	struct mutex inquiry_mutex;
 	unsigned char inquiry_len;	/* valid bytes in 'inquiry' */
 	unsigned char * inquiry;	/* INQUIRY response data */
-	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[INQUIRY_VENDOR_LEN + 1];
+	char model[INQUIRY_MODEL_LEN + 1];
+	char rev[INQUIRY_REVISION_LEN + 1];
 
 #define SCSI_DEFAULT_VPD_LEN	255	/* default SCSI VPD page size (max) */
 	struct scsi_vpd __rcu *vpd_pg0;

Re: [PATCH 2/2] scsi: core: Convert inquiry information

[Damien Le Moal]

On 5/13/26 04:46, Bart Van Assche wrote:
> Currently the vendor, model, and revision members of struct scsi_device
> are pointers to fixed-length strings that are not NUL-terminated.

s/NUL/NULL

> Fixed-precision format specifiers (e.g., "%.8s") are required whenever
> they are printed and strncmp() must be used to compare these fields.
> This is error-prone.
> 
> Convert these fields to fixed-size character arrays within struct
> scsi_device. Remove an !sdev->model check because sdev->model is now
> guaranteed not to be NULL.
> 
> This patch fixes a bug in the qla2xxx driver. It makes the following
> code safe:
> 
> 		if (state_flags & BIT_4)
> 			scmd_printk(KERN_WARNING, cp,
> 			    "Unsupported device '%s' found.\n",
> 			    cp->device->vendor);
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/hwmon/drivetemp.c  |  5 +----
>  drivers/scsi/scsi_scan.c   | 12 ++++++------
>  include/scsi/scsi_device.h |  7 ++++---
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
> index 002e0660a0b8..efe8b229bdbe 100644
> --- a/drivers/hwmon/drivetemp.c
> +++ b/drivers/hwmon/drivetemp.c
> @@ -306,13 +306,10 @@ static bool drivetemp_sct_avoid(struct drivetemp_data *st)
>  	struct scsi_device *sdev = st->sdev;
>  	unsigned int ctr;
>  
> -	if (!sdev->model)
> -		return false;
> -
>  	/*
>  	 * The "model" field contains just the raw SCSI INQUIRY response
>  	 * "product identification" field, which has a width of 16 bytes.
> -	 * This field is space-filled, but is NOT NULL-terminated.
> +	 * This field is space-filled and NUL-terminated.

s/NUL/NULL

>  	 */
>  	for (ctr = 0; ctr < ARRAY_SIZE(sct_avoid_models); ctr++)
>  		if (!strncmp(sdev->model, sct_avoid_models[ctr],
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ef22a4228b85..6c3a5d451c1d 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -292,9 +292,9 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
>  	if (!sdev)
>  		goto out;
>  
> -	sdev->vendor = scsi_null_device_strs;
> -	sdev->model = scsi_null_device_strs;
> -	sdev->rev = scsi_null_device_strs;
> +	strscpy(sdev->vendor, scsi_null_device_strs);
> +	strscpy(sdev->model, scsi_null_device_strs);
> +	strscpy(sdev->rev, scsi_null_device_strs);
>  	sdev->host = shost;
>  	sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
>  	sdev->id = starget->id;
> @@ -905,9 +905,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	if (sdev->inquiry == NULL)
>  		return SCSI_SCAN_NO_RESPONSE;
>  
> -	sdev->vendor = (char *) (sdev->inquiry + 8);
> -	sdev->model = (char *) (sdev->inquiry + 16);
> -	sdev->rev = (char *) (sdev->inquiry + 32);
> +	strscpy(sdev->vendor, sdev->inquiry + 8);
> +	strscpy(sdev->model, sdev->inquiry + 16);
> +	strscpy(sdev->rev, sdev->inquiry + 32);
>  
>  	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
>  	if (sdev->is_ata) {
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 9c2a7bbe5891..029f5115b2ea 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -7,6 +7,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/blk-mq.h>
>  #include <scsi/scsi.h>
> +#include <scsi/scsi_common.h>
>  #include <linux/atomic.h>
>  #include <linux/sbitmap.h>
>  
> @@ -137,9 +138,9 @@ struct scsi_device {
>  	struct mutex inquiry_mutex;
>  	unsigned char inquiry_len;	/* valid bytes in 'inquiry' */
>  	unsigned char * inquiry;	/* INQUIRY response data */
> -	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[INQUIRY_VENDOR_LEN + 1];
> +	char model[INQUIRY_MODEL_LEN + 1];
> +	char rev[INQUIRY_REVISION_LEN + 1];
>  
>  #define SCSI_DEFAULT_VPD_LEN	255	/* default SCSI VPD page size (max) */
>  	struct scsi_vpd __rcu *vpd_pg0;


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] scsi: core, target: Move three constants into <scsi/scsi_common.h>

[Damien Le Moal]

On 5/13/26 04:46, Bart Van Assche wrote:
> Prepare for using these constants in the SCSI core.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] scsi: core, target: Move three constants into <scsi/scsi_common.h>

[Hannes Reinecke]

On 5/12/26 21:46, Bart Van Assche wrote:
> Prepare for using these constants in the SCSI core.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   include/scsi/scsi_common.h        | 4 ++++
>   include/target/target_core_base.h | 5 +----
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/2] scsi: core: Convert inquiry information

[Hannes Reinecke]

On 5/12/26 21:46, Bart Van Assche wrote:
> Currently the vendor, model, and revision members of struct scsi_device
> are pointers to fixed-length strings that are not NUL-terminated.
> Fixed-precision format specifiers (e.g., "%.8s") are required whenever
> they are printed and strncmp() must be used to compare these fields.
> This is error-prone.
> 
> Convert these fields to fixed-size character arrays within struct
> scsi_device. Remove an !sdev->model check because sdev->model is now
> guaranteed not to be NULL.
> 
> This patch fixes a bug in the qla2xxx driver. It makes the following
> code safe:
> 
> 		if (state_flags & BIT_4)
> 			scmd_printk(KERN_WARNING, cp,
> 			    "Unsupported device '%s' found.\n",
> 			    cp->device->vendor);
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/hwmon/drivetemp.c  |  5 +----
>   drivers/scsi/scsi_scan.c   | 12 ++++++------
>   include/scsi/scsi_device.h |  7 ++++---
>   3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c
> index 002e0660a0b8..efe8b229bdbe 100644
> --- a/drivers/hwmon/drivetemp.c
> +++ b/drivers/hwmon/drivetemp.c
> @@ -306,13 +306,10 @@ static bool drivetemp_sct_avoid(struct drivetemp_data *st)
>   	struct scsi_device *sdev = st->sdev;
>   	unsigned int ctr;
>   
> -	if (!sdev->model)
> -		return false;
> -
>   	/*
>   	 * The "model" field contains just the raw SCSI INQUIRY response
>   	 * "product identification" field, which has a width of 16 bytes.
> -	 * This field is space-filled, but is NOT NULL-terminated.
> +	 * This field is space-filled and NUL-terminated.
>   	 */
>   	for (ctr = 0; ctr < ARRAY_SIZE(sct_avoid_models); ctr++)
>   		if (!strncmp(sdev->model, sct_avoid_models[ctr],
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ef22a4228b85..6c3a5d451c1d 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -292,9 +292,9 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
>   	if (!sdev)
>   		goto out;
>   
> -	sdev->vendor = scsi_null_device_strs;
> -	sdev->model = scsi_null_device_strs;
> -	sdev->rev = scsi_null_device_strs;
> +	strscpy(sdev->vendor, scsi_null_device_strs);
> +	strscpy(sdev->model, scsi_null_device_strs);
> +	strscpy(sdev->rev, scsi_null_device_strs);
>   	sdev->host = shost;
>   	sdev->queue_ramp_up_period = SCSI_DEFAULT_RAMP_UP_PERIOD;
>   	sdev->id = starget->id;
> @@ -905,9 +905,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>   	if (sdev->inquiry == NULL)
>   		return SCSI_SCAN_NO_RESPONSE;
>   
> -	sdev->vendor = (char *) (sdev->inquiry + 8);
> -	sdev->model = (char *) (sdev->inquiry + 16);
> -	sdev->rev = (char *) (sdev->inquiry + 32);
> +	strscpy(sdev->vendor, sdev->inquiry + 8);
> +	strscpy(sdev->model, sdev->inquiry + 16);
> +	strscpy(sdev->rev, sdev->inquiry + 32);
>   
>   	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
>   	if (sdev->is_ata) {


Question is whether we shouldn't make this generic, ie treat 'inquiry'
as a temporary blob, copy things over to fields in 'sdev', and then
free the 'inquiry' blob again.
There are soo many things tacked onto the standard inquiry data 
(especially for storage array trying to mimic SCSI-2 inquiry data),
that we're better of copying over only fields which we _know_.
_And_ it'll save us a permanent data allocation for the scsi device...

Hmm?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/2] scsi: core: Convert inquiry information

[James Bottomley]

On Tue, 2026-05-12 at 12:46 -0700, Bart Van Assche wrote:
> @@ -905,9 +905,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
> unsigned char *inq_result,
>  	if (sdev->inquiry == NULL)
>  		return SCSI_SCAN_NO_RESPONSE;
>  
> -	sdev->vendor = (char *) (sdev->inquiry + 8);
> -	sdev->model = (char *) (sdev->inquiry + 16);
> -	sdev->rev = (char *) (sdev->inquiry + 32);
> +	strscpy(sdev->vendor, sdev->inquiry + 8);
> +	strscpy(sdev->model, sdev->inquiry + 16);
> +	strscpy(sdev->rev, sdev->inquiry + 32);
>  
>  	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
>  	if (sdev->is_ata) {

If you've gone to all the trouble to lift the lengths out as #defines,
shouldn't we do the same with the offsets?

Regards,

James

Re: [PATCH 2/2] scsi: core: Convert inquiry information

[Bart Van Assche]

On 5/13/26 1:03 AM, Damien Le Moal wrote:
> On 5/13/26 04:46, Bart Van Assche wrote:
>> Currently the vendor, model, and revision members of struct scsi_device
>> are pointers to fixed-length strings that are not NUL-terminated.
> 
> s/NUL/NULL
Really? NUL is the correct spelling according to
https://en.wikipedia.org/wiki/ASCII and also according to any other
ASCII table I have ever seen.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: core: Convert inquiry information

[Bart Van Assche]

On 5/13/26 2:33 AM, Hannes Reinecke wrote:
> Question is whether we shouldn't make this generic, ie treat 'inquiry'
> as a temporary blob, copy things over to fields in 'sdev', and then
> free the 'inquiry' blob again.
> There are soo many things tacked onto the standard inquiry data 
> (especially for storage array trying to mimic SCSI-2 inquiry data),
> that we're better of copying over only fields which we _know_.
> _And_ it'll save us a permanent data allocation for the scsi device...
> 
> Hmm?

Are you perhaps suggesting to remove the inquiry sysfs attribute? From
scsi_sysfs.c:

static ssize_t show_inquiry(struct file *filep, struct kobject *kobj,
			    const struct bin_attribute *bin_attr,
			    char *buf, loff_t off, size_t count)
{
	struct device *dev = kobj_to_dev(kobj);
	struct scsi_device *sdev = to_scsi_device(dev);

	if (!sdev->inquiry)
		return -EINVAL;

	return memory_read_from_buffer(buf, count, &off, sdev->inquiry,
				       sdev->inquiry_len);
}

Bart.

Re: [PATCH 2/2] scsi: core: Convert inquiry information

[Bart Van Assche]

On 5/13/26 5:49 AM, James Bottomley wrote:
> On Tue, 2026-05-12 at 12:46 -0700, Bart Van Assche wrote:
>> @@ -905,9 +905,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
>> unsigned char *inq_result,
>>   	if (sdev->inquiry == NULL)
>>   		return SCSI_SCAN_NO_RESPONSE;
>>   
>> -	sdev->vendor = (char *) (sdev->inquiry + 8);
>> -	sdev->model = (char *) (sdev->inquiry + 16);
>> -	sdev->rev = (char *) (sdev->inquiry + 32);
>> +	strscpy(sdev->vendor, sdev->inquiry + 8);
>> +	strscpy(sdev->model, sdev->inquiry + 16);
>> +	strscpy(sdev->rev, sdev->inquiry + 32);
>>   
>>   	sdev->is_ata = strncmp(sdev->vendor, "ATA     ", 8) == 0;
>>   	if (sdev->is_ata) {
> 
> If you've gone to all the trouble to lift the lengths out as #defines,
> shouldn't we do the same with the offsets?

Hi James,

I will introduce symbolic names for the offsets in the next version of
this patch series.

Thanks,

Bart.