Re: Bug in the scsi_id (0.9 version) utility

Re: Bug in the scsi_id (0.9 version) utility

[Mike Anderson]

adding a linux-scsi cc as others may have comments on checking of reserved
fields.

brooks@netgate.net <brooks@netgate.net> wrote:
>
> The utility makes the assumption that a reserved field in the 
> identification descriptor returned by a SCSI page 83 response will be 
> filled with "0x00". The Solaris iSCSI target implemenation (and possibly 
> others) instead fills the field with "0XFF". As a result the utility 
> assumes that the Solaris target is a SCSI-2 device and (incorrectly) parses 
> the response as such. The Solaris box actually uses the latest released 
> SCSI command set (SPC3):
>
>     http://www.t10.org/ftp/t10/drafts/spc3/spc3r23.pdf
>
> The fix was simple, check for 0x00 and for 0xFF instead of checking for 
> !0x00. Since this code is generally only used discovery discovery it's a 
> low risk change.
>
> As I understand it there's nothing in the SPC2/3 spec that states that 
> "0x00" should be written to the reserved field of the identification 
> descriptor so I'm not sure this is the best way to determine the device 
> type command set. 

The SAM definitions section and the SPC Keywords section provides a
definition of what should be in a reserved field.

That said we are not required to be checking the reserved fields for zero
but in this case we are doing so to work around another device issue.

Instead of adding yet another device work around to the "if" which
makes it hard to rework the code in the future as you do not know how many
vendor, models are running through certain non compliant checks (i.e. more
than the intend models may exhibit like behavior).

We possible should look at updating how we handle not compliant behavior.
scsi_id.config is available but that must not be useful in all cases as
the standard page 83 code has a check for the pre-spc3-83 case even though
it is selectable through the config file and command line. 

> But, if it's the best we have I guess all we can do is 
> accomodiate the various implementations.  Below is the diff patch made 
> against the 118 code set (0.9 version of the "scsi_id" util).
>
> Please let me know if there is a better way to reort the bug and if this 
> will be an accepted change.  This bug is currently affecting XenSource 
> (Citrix) 4.1 (still in beta).  Xen (4.1) uses the utility to determine the 
> SCSIid of the target for later use in multipath attachement.
>
> Thank you,
> Kevin
>
> ----------------------------------------------------------------
> Kevin Brooks                                    NetGate Internet
> http://www.netgate.net                          iStrata Inc
> ----------------------------------------------------------------
>
>
> --- scsi_serial.c       2008-03-13 08:50:16.000000000 -0700
> +++ scsi_serial_patched.c       2008-03-13 08:46:09.000000000 -0700
> @@ -590,9 +590,18 @@
>          * 0x006048.
>          */
>
> -       if (page_83[6] != 0)
> -               return check_fill_0x83_prespc3(dev_scsi, page_83, 
> id_search_list,
> -                                              serial, serial_short, len);
> +       /*
> +        * Kevin Brooks patch for Solaris iSCSI target devices that
> +        * return something other than 0x00 in the reserved field
> +        * of the reserved field of the identification descriptor
> +        * of the SCSI page 83 response.  Incorrectly parsing this
> +        * causes the device to be incorrectly identified as as a
> +        * SCSI-2 and (incorrectly) parses the response as such.
> +        * The Solaris iSCSI target implementation (and possibly
> +         * others) instead fills the field with 0XFF.
> +        */
> +
> +       if (page_83[6] == 0x00 || page_83[6] == 0xff) {
>
>         /*
>          * Search for a match in the prioritized id_search_list.
> @@ -623,6 +632,13 @@
>                         }
>                 }
>         }
> +
> +       } else {
> +
> +               return check_fill_0x83_prespc3(dev_scsi, page_83, 
> id_search_list,
> +                                              serial, serial_short, len);
> +       }
> +
>         return 1;
>  }

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

Re: Bug in the scsi_id (0.9 version) utility

[James Bottomley]

On Thu, 2008-03-13 at 23:08 -0700, Mike Anderson wrote:
> adding a linux-scsi cc as others may have comments on checking of reserved
> fields.
> 
> brooks@netgate.net <brooks@netgate.net> wrote:
> >
> > The utility makes the assumption that a reserved field in the 
> > identification descriptor returned by a SCSI page 83 response will be 
> > filled with "0x00". The Solaris iSCSI target implemenation (and possibly 
> > others) instead fills the field with "0XFF". As a result the utility 
> > assumes that the Solaris target is a SCSI-2 device and (incorrectly) parses 
> > the response as such. The Solaris box actually uses the latest released 
> > SCSI command set (SPC3):
> >
> >     http://www.t10.org/ftp/t10/drafts/spc3/spc3r23.pdf
> >
> > The fix was simple, check for 0x00 and for 0xFF instead of checking for 
> > !0x00. Since this code is generally only used discovery discovery it's a 
> > low risk change.
> >
> > As I understand it there's nothing in the SPC2/3 spec that states that 
> > "0x00" should be written to the reserved field of the identification 
> > descriptor so I'm not sure this is the best way to determine the device 
> > type command set. 

This is incorrect; SPC does specify zero in reserved fields.

> The SAM definitions section and the SPC Keywords section provides a
> definition of what should be in a reserved field.

And just to make it totally plain, this is what it says:

3.3.9 reserved: A keyword referring to bits, bytes, words, fields and
code values that are set aside for future standardization. A reserved
bit, byte, word or field shall be set to zero, or in accordance with a
future extension to this standard. Recipients are not required to check
reserved bits, bytes, words or fields for zero values. Receipt of
reserved code values in defined fields shall be reported as an error.

So your implementation is a spec violation.  You're just lucky SPC4
leaves it reserved.  If they ever decided to implement something there,
not only would you be in spec violation, you'd likely be claiming some
property your device doesn't support.

> That said we are not required to be checking the reserved fields for zero
> but in this case we are doing so to work around another device issue.
> 
> Instead of adding yet another device work around to the "if" which
> makes it hard to rework the code in the future as you do not know how many
> vendor, models are running through certain non compliant checks (i.e. more
> than the intend models may exhibit like behavior).
> 
> We possible should look at updating how we handle not compliant behavior.
> scsi_id.config is available but that must not be useful in all cases as
> the standard page 83 code has a check for the pre-spc3-83 case even though
> it is selectable through the config file and command line. 

Some type of blacklist might work: we're already starting to find USB
devices that aren't too happy with VPD inquiries.

James

Re: Bug in the scsi_id (0.9 version) utility

[brooks]

> This is incorrect; SPC does specify zero in reserved fields.
>
>> The SAM definitions section and the SPC Keywords section provides a
>> definition of what should be in a reserved field.

Thanks for the clarification.  I've submitted the bug to OpenSolaris.

>> That said we are not required to be checking the reserved fields for zero
>> but in this case we are doing so to work around another device issue.
>>
>> Instead of adding yet another device work around to the "if" which
>> makes it hard to rework the code in the future as you do not know how many
>> vendor, models are running through certain non compliant checks (i.e. more
>> than the intend models may exhibit like behavior).
>>
>> We possible should look at updating how we handle not compliant behavior.
>> scsi_id.config is available but that must not be useful in all cases as
>> the standard page 83 code has a check for the pre-spc3-83 case even though
>> it is selectable through the config file and command line.
>
> Some type of blacklist might work: we're already starting to find USB
> devices that aren't too happy with VPD inquiries.

That makes sense.  Please let me know what you decide, and if there's 
anything more I can do to help.

Kevin

Re: Bug in the scsi_id (0.9 version) utility

[Patrick Mansfield]

Hi all ...

On Mon, Mar 17, 2008 at 11:38:41AM -0700, brooks@netgate.net wrote:

>>> That said we are not required to be checking the reserved fields for zero
>>> but in this case we are doing so to work around another device issue.
>>>
>>> Instead of adding yet another device work around to the "if" which
>>> makes it hard to rework the code in the future as you do not know how many
>>> vendor, models are running through certain non compliant checks (i.e. more
>>> than the intend models may exhibit like behavior).
>>>
>>> We possible should look at updating how we handle not compliant behavior.
>>> scsi_id.config is available but that must not be useful in all cases as

Note Mike meant the file /etc/scsi_id.config, this file can already be
used to blacklist devices for scsi_id. But, the scsi_id blacklist doesn't
currently know about the transport or bus type (like USB, FCP, etc), and
relies mainly on SCSI vendor and product names, so USB devices can't be
easily blacklisted.

udev doesn't have problems for scsi_id and USB persistent names as it
won't call scsi_id for USB devices, see the 60-persistent-storage.rules

[btw it looks like a failure in usb_id would allow scsi_id to be called
for udev.]

>>> the standard page 83 code has a check for the pre-spc3-83 case even though
>>> it is selectable through the config file and command line.

In hindsight that sucks here :-( we probably should have (somehow) made it
use a black list. AFAIR the issue is that the vendor + product (AFAIR
it's always EMC Symmetrix ... Mike as you know that all too well!) is the
same for EMC devices that give different behaviour.

>> Some type of blacklist might work: we're already starting to find USB
>> devices that aren't too happy with VPD inquiries.

Per above comment, udev should be OK. For other users (like multipath) it
would be simplest if they do not call scsi_id at all for USB devices, and
then (hopefully not for multipath!) have white lists for properly
functioning USB devices 

> That makes sense.  Please let me know what you decide, and if there's 
> anything more I can do to help.

Best is if you can get the device fixed ;-)

If you can't get a fix, something like the following should be OK.
Actually this is probably nice anyway, in case other devices hit the
issue. [I think someone else reported this problem some time ago ...]

With this, you would have to run your device with -p 0x83 (manually for
testing, and then eventually via some blacklist rules, assuming page 0x83
works correctly for the device), and then that blacklist rule will live
on forever ...

Also note that the version id in scsi_id has not been maintained, you need
to specify the udev version for people to know what version you're using.

Untested patch (and not easy to properly test without the actual borken
device) on top of current udev git view:

--- git-udev/extras/scsi_id/scsi_serial.c	2008-03-18 09:24:46.000000000 -0700
+++ udev/extras/scsi_id/scsi_serial.c	2008-03-18 12:16:38.000000000 -0700
@@ -544,7 +544,8 @@ static int check_fill_0x83_prespc3(struc
 
 /* Get device identification VPD page */
 static int do_scsi_page83_inquiry(struct sysfs_device *dev_scsi, int fd,
-				  char *serial, char *serial_short, int len)
+				  char *serial, char *serial_short, int len,
+				  int page_code)
 {
 	int retval;
 	unsigned int id_ind, j;
@@ -590,7 +591,7 @@ static int do_scsi_page83_inquiry(struct
 	 * 0x006048.
 	 */
 	
-	if (page_83[6] != 0) 
+	if ((page_code == 0x00) && (page_83[6] != 0))
 		return check_fill_0x83_prespc3(dev_scsi, page_83, id_search_list,
 					       serial, serial_short, len);
 
@@ -793,7 +794,7 @@ int scsi_get_serial (struct sysfs_device
 			goto completed;
 		}
 	} else if (page_code == PAGE_83) {
-		if (do_scsi_page83_inquiry(dev_scsi, fd, serial, serial_short, len)) {
+		if (do_scsi_page83_inquiry(dev_scsi, fd, serial, serial_short, len, page_code)) {
 			retval = 1;
 			goto completed;
 		} else  {
@@ -809,7 +810,7 @@ int scsi_get_serial (struct sysfs_device
 			 * conform to pre-SPC3 expectations.
 			 */
 			if (retval == 2) {
-				if (do_scsi_page83_inquiry(dev_scsi, fd, serial, serial_short, len)) {
+				if (do_scsi_page83_inquiry(dev_scsi, fd, serial, serial_short, len, 0x00)) {
 					retval = 1;
 					goto completed;
 				} else  {
@@ -849,7 +850,8 @@ int scsi_get_serial (struct sysfs_device
 	for (ind = 4; ind <= page0[3] + 3; ind++)
 		if (page0[ind] == PAGE_83)
 			if (!do_scsi_page83_inquiry(dev_scsi, fd,
-						    serial, serial_short, len)) {
+						    serial, serial_short,
+						    len, 0x00)) {
 				/*
 				 * Success
 				 */