[PATCHv2 0/4] Fixup blacklist handling

[PATCHv2 0/4] Fixup blacklist handling

[Hannes Reinecke]

Hi all,

the SCSI blacklist handling seems to be rather tricky issue;
everytime a fix is included it tends to break other devices.
This patchset attempt to simplify the devlist handling yet again,
but this time implementing the framework for regression testing, too.
A patch adding a regression test to the blktest suite will follow.

As usual, comments and reviews are welcome.

Changes to v1:
- Implement exact match for vendor string as suggested by Bart
- Straigten out issues pointed out by Alan Stern
- Reshuffle patches

Hannes Reinecke (4):
  scsi_debug: allow to specify inquiry vendor and model
  scsi: Export blacklist flags to sysfs
  scsi: whitespace fixes in scsi_devinfo.c
  scsi_devinfo: fixup string compare

 drivers/scsi/scsi_debug.c   | 25 +++++++++-----
 drivers/scsi/scsi_devinfo.c | 84 ++++++++++++++++++++++++++++-----------------
 drivers/scsi/scsi_scan.c    |  1 +
 drivers/scsi/scsi_sysfs.c   | 11 ++++++
 4 files changed, 81 insertions(+), 40 deletions(-)

-- 
1.8.5.6

[PATCHv2 1/4] scsi_debug: allow to specify inquiry vendor and model

[Hannes Reinecke]

For testing purposes we need to be able to pass in the inquiry
vendor and model.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_debug.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index dc095a2..c8a3f8d 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -953,9 +953,9 @@ static int fetch_to_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
 }
 
 
-static const char * inq_vendor_id = "Linux   ";
-static const char * inq_product_id = "scsi_debug      ";
-static const char *inq_product_rev = "0186";	/* version less '.' */
+static char sdebug_inq_vendor_id[9] = "Linux   ";
+static char sdebug_inq_product_id[17] = "scsi_debug      ";
+static char sdebug_inq_product_rev[5] = "0186";	/* version less '.' */
 /* Use some locally assigned NAAs for SAS addresses. */
 static const u64 naa3_comp_a = 0x3222222000000000ULL;
 static const u64 naa3_comp_b = 0x3333333000000000ULL;
@@ -975,8 +975,8 @@ static int inquiry_vpd_83(unsigned char *arr, int port_group_id,
 	arr[0] = 0x2;	/* ASCII */
 	arr[1] = 0x1;
 	arr[2] = 0x0;
-	memcpy(&arr[4], inq_vendor_id, 8);
-	memcpy(&arr[12], inq_product_id, 16);
+	memcpy(&arr[4], sdebug_inq_vendor_id, 8);
+	memcpy(&arr[12], sdebug_inq_product_id, 16);
 	memcpy(&arr[28], dev_id_str, dev_id_str_len);
 	num = 8 + 16 + dev_id_str_len;
 	arr[3] = num;
@@ -1408,9 +1408,9 @@ static int resp_inquiry(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	arr[6] = 0x10; /* claim: MultiP */
 	/* arr[6] |= 0x40; ... claim: EncServ (enclosure services) */
 	arr[7] = 0xa; /* claim: LINKED + CMDQUE */
-	memcpy(&arr[8], inq_vendor_id, 8);
-	memcpy(&arr[16], inq_product_id, 16);
-	memcpy(&arr[32], inq_product_rev, 4);
+	memcpy(&arr[8], sdebug_inq_vendor_id, 8);
+	memcpy(&arr[16], sdebug_inq_product_id, 16);
+	memcpy(&arr[32], sdebug_inq_product_rev, 4);
 	/* version descriptors (2 bytes each) follow */
 	put_unaligned_be16(0xc0, arr + 58);   /* SAM-6 no version claimed */
 	put_unaligned_be16(0x5c0, arr + 60);  /* SPC-5 no version claimed */
@@ -4152,6 +4152,12 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 module_param_named(fake_rw, sdebug_fake_rw, int, S_IRUGO | S_IWUSR);
 module_param_named(guard, sdebug_guard, uint, S_IRUGO);
 module_param_named(host_lock, sdebug_host_lock, bool, S_IRUGO | S_IWUSR);
+module_param_string(inq_vendor, sdebug_inq_vendor_id,
+		    sizeof(sdebug_inq_vendor_id), S_IRUGO|S_IWUSR);
+module_param_string(inq_product, sdebug_inq_product_id,
+		    sizeof(sdebug_inq_product_id), S_IRUGO|S_IWUSR);
+module_param_string(inq_rev, sdebug_inq_product_rev,
+		    sizeof(sdebug_inq_product_rev), S_IRUGO|S_IWUSR);
 module_param_named(lbpu, sdebug_lbpu, int, S_IRUGO);
 module_param_named(lbpws, sdebug_lbpws, int, S_IRUGO);
 module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
@@ -4203,6 +4209,9 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 MODULE_PARM_DESC(fake_rw, "fake reads/writes instead of copying (def=0)");
 MODULE_PARM_DESC(guard, "protection checksum: 0=crc, 1=ip (def=0)");
 MODULE_PARM_DESC(host_lock, "host_lock is ignored (def=0)");
+MODULE_PARM_DESC(inq_vendor, "SCSI INQUIRY vendor string (def=\"Linux\")");
+MODULE_PARM_DESC(inq_product, "SCSI INQUIRY product string (def=\"scsi_debug\")");
+MODULE_PARM_DESC(inq_rev, "SCSI INQUIRY revision string (def=\"scsi_debug\")");
 MODULE_PARM_DESC(lbpu, "enable LBP, support UNMAP command (def=0)");
 MODULE_PARM_DESC(lbpws, "enable LBP, support WRITE SAME(16) with UNMAP bit (def=0)");
 MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (def=0)");
-- 
1.8.5.6

Re: [PATCHv2 1/4] scsi_debug: allow to specify inquiry vendor and model

[Martin K. Petersen]

Hannes,

> For testing purposes we need to be able to pass in the inquiry
> vendor and model.

This looks fine to me.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCHv2 2/4] scsi: Export blacklist flags to sysfs

[Hannes Reinecke]

Each scsi device is scanned according to the found blacklist flags,
but this information is never presented to sysfs.
This makes it quite hard to figure out if blacklisting worked as
expected.
With this patch we're exporting an additional attribute 'blacklist'
containing the blacklist flags for this device.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_scan.c  |  1 +
 drivers/scsi/scsi_sysfs.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3832ba5..e4e4374 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -984,6 +984,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		scsi_attach_vpd(sdev);
 
 	sdev->max_queue_depth = sdev->queue_depth;
+	sdev->sdev_bflags = *bflags;
 
 	/*
 	 * Ok, the device is now all set up, we can
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5e8ace2..37d436b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -953,6 +953,16 @@ static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
 }
 static DEVICE_ATTR(wwid, S_IRUGO, sdev_show_wwid, NULL);
 
+static ssize_t
+sdev_show_blacklist(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	return snprintf (buf, 20, "0x%x\n", sdev->sdev_bflags);
+}
+static DEVICE_ATTR(blacklist, S_IRUGO, sdev_show_blacklist, NULL);
+
 #ifdef CONFIG_SCSI_DH
 static ssize_t
 sdev_show_dh_state(struct device *dev, struct device_attribute *attr,
@@ -1138,6 +1148,7 @@ static umode_t scsi_sdev_bin_attr_is_visible(struct kobject *kobj,
 	&dev_attr_queue_depth.attr,
 	&dev_attr_queue_type.attr,
 	&dev_attr_wwid.attr,
+	&dev_attr_blacklist.attr,
 #ifdef CONFIG_SCSI_DH
 	&dev_attr_dh_state.attr,
 	&dev_attr_access_state.attr,
-- 
1.8.5.6

Re: [PATCHv2 2/4] scsi: Export blacklist flags to sysfs

[Martin K. Petersen]

Hannes,

> Each scsi device is scanned according to the found blacklist flags,
> but this information is never presented to sysfs.  This makes it quite
> hard to figure out if blacklisting worked as expected.  With this
> patch we're exporting an additional attribute 'blacklist' containing
> the blacklist flags for this device.

There have been changes to the blacklist values over the years so I'm
not so keen on exporting them as a numbers.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv2 2/4] scsi: Export blacklist flags to sysfs

[Hannes Reinecke]

On 08/11/2017 02:43 AM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> Each scsi device is scanned according to the found blacklist flags,
>> but this information is never presented to sysfs.  This makes it quite
>> hard to figure out if blacklisting worked as expected.  With this
>> patch we're exporting an additional attribute 'blacklist' containing
>> the blacklist flags for this device.
> 
> There have been changes to the blacklist values over the years so I'm
> not so keen on exporting them as a numbers.
> 
Ah, good to know.
I was thinking of exporting them as text; 'tis better for a quick check
anyway.
Would that be acceptable?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCHv2 3/4] scsi: whitespace fixes in scsi_devinfo.c

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 28fea83..776c701 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -304,8 +304,8 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
 			 */
 			to[from_length] = '\0';
 		} else {
-			/* 
-			 * space pad the string if it is short. 
+			/*
+			 * space pad the string if it is short.
 			 */
 			strncpy(&to[from_length], spaces,
 				to_length - from_length);
@@ -325,10 +325,10 @@ static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
  * @flags:	if strflags NULL, use this flag value
  *
  * Description:
- * 	Create and add one dev_info entry for @vendor, @model, @strflags or
- * 	@flag. If @compatible, add to the tail of the list, do not space
- * 	pad, and set devinfo->compatible. The scsi_static_device_list entries
- * 	are added with @compatible 1 and @clfags NULL.
+ *	Create and add one dev_info entry for @vendor, @model, @strflags or
+ *	@flag. If @compatible, add to the tail of the list, do not space
+ *	pad, and set devinfo->compatible. The scsi_static_device_list entries
+ *	are added with @compatible 1 and @clfags NULL.
  *
  * Returns: 0 OK, -error on failure.
  **/
@@ -350,11 +350,11 @@ static int scsi_dev_info_list_add(int compatible, char *vendor, char *model,
  * @key:	specify list to use
  *
  * Description:
- * 	Create and add one dev_info entry for @vendor, @model,
- * 	@strflags or @flag in list specified by @key. If @compatible,
- * 	add to the tail of the list, do not space pad, and set
- * 	devinfo->compatible. The scsi_static_device_list entries are
- * 	added with @compatible 1 and @clfags NULL.
+ *	Create and add one dev_info entry for @vendor, @model,
+ *	@strflags or @flag in list specified by @key. If @compatible,
+ *	add to the tail of the list, do not space pad, and set
+ *	devinfo->compatible. The scsi_static_device_list entries are
+ *	added with @compatible 1 and @clfags NULL.
  *
  * Returns: 0 OK, -error on failure.
  **/
@@ -405,7 +405,7 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
  *
  * Description:
  *	Finds the first dev_info entry matching @vendor, @model
- * 	in list specified by @key.
+ *	in list specified by @key.
  *
  * Returns: pointer to matching entry, or ERR_PTR on failure.
  **/
@@ -508,10 +508,10 @@ int scsi_dev_info_list_del_keyed(char *vendor, char *model, int key)
  * @dev_list:	string of device flags to add
  *
  * Description:
- * 	Parse dev_list, and add entries to the scsi_dev_info_list.
- * 	dev_list is of the form "vendor:product:flag,vendor:product:flag".
- * 	dev_list is modified via strsep. Can be called for command line
- * 	addition, for proc or mabye a sysfs interface.
+ *	Parse dev_list, and add entries to the scsi_dev_info_list.
+ *	dev_list is of the form "vendor:product:flag,vendor:product:flag".
+ *	dev_list is modified via strsep. Can be called for command line
+ *	addition, for proc or mabye a sysfs interface.
  *
  * Returns: 0 if OK, -error on failure.
  **/
@@ -701,7 +701,7 @@ static int proc_scsi_devinfo_open(struct inode *inode, struct file *file)
 	return seq_open(file, &scsi_devinfo_seq_ops);
 }
 
-/* 
+/*
  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via /proc.
  *
  * Description: Adds a black/white list entry for vendor and model with an
@@ -840,8 +840,8 @@ int scsi_dev_info_remove_list(int key)
  * scsi_init_devinfo - set up the dynamic device list.
  *
  * Description:
- * 	Add command line entries from scsi_dev_flags, then add
- * 	scsi_static_device_list entries to the scsi device info list.
+ *	Add command line entries from scsi_dev_flags, then add
+ *	scsi_static_device_list entries to the scsi device info list.
  */
 int __init scsi_init_devinfo(void)
 {
-- 
1.8.5.6

[PATCHv2 4/4] scsi_devinfo: fixup string compare

[Hannes Reinecke]

When checking the model and vendor string we need to use the
minimum value of either string, otherwise we'll miss out on
wildcard matches.
And we should take card when matching with zero size strings;
results might be unpredictable.
With this patch the rules for matching devinfo strings are
as follows:
- Vendor strings must match exactly
- Empty Model strings will only match if the devinfo model
  is also empty
- Model strings shorter than the devinfo model string will
  not match

Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_devinfo.c | 46 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 776c701..f8f5cb7 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -399,8 +399,8 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
 
 /**
  * scsi_dev_info_list_find - find a matching dev_info list entry.
- * @vendor:	vendor string
- * @model:	model (product) string
+ * @vendor:	full vendor string
+ * @model:	full model (product) string
  * @key:	specify list to use
  *
  * Description:
@@ -440,6 +440,8 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 	/* Also skip trailing spaces */
 	while (vmax > 0 && vskip[vmax - 1] == ' ')
 		--vmax;
+	if (!vmax)
+		vskip = NULL;
 
 	mmax = sizeof(devinfo->model);
 	mskip = model;
@@ -449,27 +451,45 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
 	}
 	while (mmax > 0 && mskip[mmax - 1] == ' ')
 		--mmax;
+	if (!mmax)
+		mskip = NULL;
 
 	list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
 			    dev_info_list) {
 		if (devinfo->compatible) {
 			/*
-			 * Behave like the older version of get_device_flags.
+			 * vendor strings must be an exact match
 			 */
-			if (memcmp(devinfo->vendor, vskip, vmax) ||
-					(vmax < sizeof(devinfo->vendor) &&
-						devinfo->vendor[vmax]))
+			if (vmax != strlen(devinfo->vendor))
 				continue;
-			if (memcmp(devinfo->model, mskip, mmax) ||
-					(mmax < sizeof(devinfo->model) &&
-						devinfo->model[mmax]))
+			if (vskip && vmax &&
+			    memcmp(devinfo->vendor, vskip, vmax))
 				continue;
-			return devinfo;
+			/*
+			 * Empty model strings only match if both strings
+			 * are empty.
+			 */
+			if (!mmax && !strlen(devinfo->model))
+				return devinfo;
+
+			/*
+			 * Empty @model never matches
+			 */
+			if (!mskip)
+				continue;
+
+			/*
+			 * @model specifies the full string, and
+			 * must be larger or equal to devinfo->model
+			 */
+			if (!memcmp(devinfo->model, mskip,
+				    strlen(devinfo->model)))
+				return devinfo;
 		} else {
 			if (!memcmp(devinfo->vendor, vendor,
-				     sizeof(devinfo->vendor)) &&
-			     !memcmp(devinfo->model, model,
-				      sizeof(devinfo->model)))
+				    sizeof(devinfo->vendor)) &&
+			    !memcmp(devinfo->model, model,
+				    sizeof(devinfo->model)))
 				return devinfo;
 		}
 	}
-- 
1.8.5.6

Re: [PATCHv2 4/4] scsi_devinfo: fixup string compare

[Alan Stern]

On Wed, 9 Aug 2017, Hannes Reinecke wrote:

> When checking the model and vendor string we need to use the
> minimum value of either string, otherwise we'll miss out on
> wildcard matches.

This is now true only for the model string, not the vendor string.  And 
even for the model string, you allow the lengths to differ in only one 
direction (the model string can be longer than the devinfo model 
string, but not vice versa).

> And we should take card when matching with zero size strings;
> results might be unpredictable.
> With this patch the rules for matching devinfo strings are
> as follows:
> - Vendor strings must match exactly
> - Empty Model strings will only match if the devinfo model
>   is also empty
> - Model strings shorter than the devinfo model string will
>   not match

Good, this is a lot better than before.  These rules make sense.

However, the rules and the code are both somewhat redundant.  For 
example, the second rule above is already a consequence of the third 
rule: If the model string is empty and the devinfo model string isn't, 
then the model string is shorter than the devinfo model string so they 
won't match.

> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_devinfo.c | 46 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 776c701..f8f5cb7 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -399,8 +399,8 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
>  
>  /**
>   * scsi_dev_info_list_find - find a matching dev_info list entry.
> - * @vendor:	vendor string
> - * @model:	model (product) string
> + * @vendor:	full vendor string
> + * @model:	full model (product) string
>   * @key:	specify list to use
>   *
>   * Description:
> @@ -440,6 +440,8 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
>  	/* Also skip trailing spaces */
>  	while (vmax > 0 && vskip[vmax - 1] == ' ')
>  		--vmax;
> +	if (!vmax)
> +		vskip = NULL;
>  
>  	mmax = sizeof(devinfo->model);
>  	mskip = model;
> @@ -449,27 +451,45 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
>  	}
>  	while (mmax > 0 && mskip[mmax - 1] == ' ')
>  		--mmax;
> +	if (!mmax)
> +		mskip = NULL;

Neither of these changes is needed.

>  
>  	list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
>  			    dev_info_list) {
>  		if (devinfo->compatible) {
>  			/*
> -			 * Behave like the older version of get_device_flags.
> +			 * vendor strings must be an exact match
>  			 */
> -			if (memcmp(devinfo->vendor, vskip, vmax) ||
> -					(vmax < sizeof(devinfo->vendor) &&
> -						devinfo->vendor[vmax]))
> +			if (vmax != strlen(devinfo->vendor))
>  				continue;
> -			if (memcmp(devinfo->model, mskip, mmax) ||
> -					(mmax < sizeof(devinfo->model) &&
> -						devinfo->model[mmax]))
> +			if (vskip && vmax &&
> +			    memcmp(devinfo->vendor, vskip, vmax))
>  				continue;

There's no need to test vskip and vmax.  The memcmp test alone is 
sufficient, since you know the lengths are equal and you are looking 
for an exact match.  So in the end, you could just do this:

			if (vmax != strlen(devinfo->vendor) ||
			    memcmp(devinfo->vendor, vskip, vmax))
				continue;

> -			return devinfo;
> +			/*
> +			 * Empty model strings only match if both strings
> +			 * are empty.
> +			 */
> +			if (!mmax && !strlen(devinfo->model))
> +				return devinfo;

As mentioned above, this is not necessary.

> +
> +			/*
> +			 * Empty @model never matches
> +			 */
> +			if (!mskip)
> +				continue;

Nor is this.

> +
> +			/*
> +			 * @model specifies the full string, and
> +			 * must be larger or equal to devinfo->model
> +			 */
> +			if (!memcmp(devinfo->model, mskip,
> +				    strlen(devinfo->model)))
> +				return devinfo;

It would be safer to do it this way:

			n = strlen(devinfo->model);
			if (mmax < n || memcmp(devinfo->model, mskip, n))
				continue;
			return devinfo;

Otherwise you run the risk of comparing part of the devinfo model 
string to bytes beyond the end of the model string.

Alan Stern

>  		} else {
>  			if (!memcmp(devinfo->vendor, vendor,
> -				     sizeof(devinfo->vendor)) &&
> -			     !memcmp(devinfo->model, model,
> -				      sizeof(devinfo->model)))
> +				    sizeof(devinfo->vendor)) &&
> +			    !memcmp(devinfo->model, model,
> +				    sizeof(devinfo->model)))
>  				return devinfo;
>  		}
>  	}
> 

Re: [PATCHv2 4/4] scsi_devinfo: fixup string compare

[Hannes Reinecke]

On 08/09/2017 07:43 PM, Alan Stern wrote:
> On Wed, 9 Aug 2017, Hannes Reinecke wrote:
> 
>> When checking the model and vendor string we need to use the
>> minimum value of either string, otherwise we'll miss out on
>> wildcard matches.
> 
> This is now true only for the model string, not the vendor string.  And 
> even for the model string, you allow the lengths to differ in only one 
> direction (the model string can be longer than the devinfo model 
> string, but not vice versa).
> 
>> And we should take card when matching with zero size strings;
>> results might be unpredictable.
>> With this patch the rules for matching devinfo strings are
>> as follows:
>> - Vendor strings must match exactly
>> - Empty Model strings will only match if the devinfo model
>>   is also empty
>> - Model strings shorter than the devinfo model string will
>>   not match
> 
> Good, this is a lot better than before.  These rules make sense.
> 
> However, the rules and the code are both somewhat redundant.  For 
> example, the second rule above is already a consequence of the third 
> rule: If the model string is empty and the devinfo model string isn't, 
> then the model string is shorter than the devinfo model string so they 
> won't match.
> 
>> Fixes: 5e7ff2c ("SCSI: fix new bug in scsi_dev_info_list string matching")
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/scsi_devinfo.c | 46 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 33 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
>> index 776c701..f8f5cb7 100644
>> --- a/drivers/scsi/scsi_devinfo.c
>> +++ b/drivers/scsi/scsi_devinfo.c
>> @@ -399,8 +399,8 @@ int scsi_dev_info_list_add_keyed(int compatible, char *vendor, char *model,
>>  
>>  /**
>>   * scsi_dev_info_list_find - find a matching dev_info list entry.
>> - * @vendor:	vendor string
>> - * @model:	model (product) string
>> + * @vendor:	full vendor string
>> + * @model:	full model (product) string
>>   * @key:	specify list to use
>>   *
>>   * Description:
>> @@ -440,6 +440,8 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
>>  	/* Also skip trailing spaces */
>>  	while (vmax > 0 && vskip[vmax - 1] == ' ')
>>  		--vmax;
>> +	if (!vmax)
>> +		vskip = NULL;
>>  
>>  	mmax = sizeof(devinfo->model);
>>  	mskip = model;
>> @@ -449,27 +451,45 @@ static struct scsi_dev_info_list *scsi_dev_info_list_find(const char *vendor,
>>  	}
>>  	while (mmax > 0 && mskip[mmax - 1] == ' ')
>>  		--mmax;
>> +	if (!mmax)
>> +		mskip = NULL;
> 
> Neither of these changes is needed.
> 
>>  
>>  	list_for_each_entry(devinfo, &devinfo_table->scsi_dev_info_list,
>>  			    dev_info_list) {
>>  		if (devinfo->compatible) {
>>  			/*
>> -			 * Behave like the older version of get_device_flags.
>> +			 * vendor strings must be an exact match
>>  			 */
>> -			if (memcmp(devinfo->vendor, vskip, vmax) ||
>> -					(vmax < sizeof(devinfo->vendor) &&
>> -						devinfo->vendor[vmax]))
>> +			if (vmax != strlen(devinfo->vendor))
>>  				continue;
>> -			if (memcmp(devinfo->model, mskip, mmax) ||
>> -					(mmax < sizeof(devinfo->model) &&
>> -						devinfo->model[mmax]))
>> +			if (vskip && vmax &&
>> +			    memcmp(devinfo->vendor, vskip, vmax))
>>  				continue;
> 
> There's no need to test vskip and vmax.  The memcmp test alone is 
> sufficient, since you know the lengths are equal and you are looking 
> for an exact match.  So in the end, you could just do this:
> 
> 			if (vmax != strlen(devinfo->vendor) ||
> 			    memcmp(devinfo->vendor, vskip, vmax))
> 				continue;
> 
Hmm. Let's see; I'll have the testsuite retest this.

>> -			return devinfo;
>> +			/*
>> +			 * Empty model strings only match if both strings
>> +			 * are empty.
>> +			 */
>> +			if (!mmax && !strlen(devinfo->model))
>> +				return devinfo;
> 
> As mentioned above, this is not necessary.
> 
>> +
>> +			/*
>> +			 * Empty @model never matches
>> +			 */
>> +			if (!mskip)
>> +				continue;
> 
> Nor is this.
> 
>> +
>> +			/*
>> +			 * @model specifies the full string, and
>> +			 * must be larger or equal to devinfo->model
>> +			 */
>> +			if (!memcmp(devinfo->model, mskip,
>> +				    strlen(devinfo->model)))
>> +				return devinfo;
> 
> It would be safer to do it this way:
> 
> 			n = strlen(devinfo->model);
> 			if (mmax < n || memcmp(devinfo->model, mskip, n))
> 				continue;
> 			return devinfo;
> 
> Otherwise you run the risk of comparing part of the devinfo model 
> string to bytes beyond the end of the model string.
> 
I was assuming that we're being passed in the full model string (ie the
full 16 bytes). But as we don't have a way of checking your way will be
safer.

Will be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)