diff mbox series

[1/2] scsi: scsi_vpd_lun_id(): fix designator priorities

Message ID 20201029170846.14786-1-mwilck@suse.com (mailing list archive)
State Accepted
Headers show
Series [1/2] scsi: scsi_vpd_lun_id(): fix designator priorities | expand

Commit Message

Martin Wilck Oct. 29, 2020, 5:08 p.m. UTC
From: Martin Wilck <mwilck@suse.com>

The current implementation of scsi_vpd_lun_id() uses the designator
length as an implicit measure of priority. This works most of the
time, but not always. For example, some Hitachi storage arrays return
this in VPD 0x83:

VPD INQUIRY: Device Identification page
  Designation descriptor number 1, descriptor length: 24
    designator_type: T10 vendor identification,  code_set: ASCII
    associated with the Addressed logical unit
      vendor id: HITACHI
      vendor specific: 5030C3502025
  Designation descriptor number 2, descriptor length: 6
    designator_type: vendor specific [0x0],  code_set: Binary
    associated with the Target port
      vendor specific: 08 03
  Designation descriptor number 3, descriptor length: 20
    designator_type: NAA,  code_set: Binary
    associated with the Addressed logical unit
      NAA 6, IEEE Company_id: 0x60e8
      Vendor Specific Identifier: 0x7c35000
      Vendor Specific Identifier Extension: 0x30c35000002025
      [0x60060e8007c350000030c35000002025]

The current code would use the first descriptor, because it's longer
than the NAA descriptor. But this is wrong, the kernel is supposed
to prefer NAA descriptors over T10 vendor ID. Designator length
should only be used to compare designators of the same type.

This patch addresses the issue by separating designator priority and
length.

Fixes: 9983bed3907c ("scsi: Add scsi_vpd_lun_id()")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 126 +++++++++++++++++++++++++++-------------
 1 file changed, 86 insertions(+), 40 deletions(-)

Comments

Hannes Reinecke Oct. 30, 2020, 7:16 a.m. UTC | #1
On 10/29/20 6:08 PM, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The current implementation of scsi_vpd_lun_id() uses the designator
> length as an implicit measure of priority. This works most of the
> time, but not always. For example, some Hitachi storage arrays return
> this in VPD 0x83:
> 
> VPD INQUIRY: Device Identification page
>    Designation descriptor number 1, descriptor length: 24
>      designator_type: T10 vendor identification,  code_set: ASCII
>      associated with the Addressed logical unit
>        vendor id: HITACHI
>        vendor specific: 5030C3502025
>    Designation descriptor number 2, descriptor length: 6
>      designator_type: vendor specific [0x0],  code_set: Binary
>      associated with the Target port
>        vendor specific: 08 03
>    Designation descriptor number 3, descriptor length: 20
>      designator_type: NAA,  code_set: Binary
>      associated with the Addressed logical unit
>        NAA 6, IEEE Company_id: 0x60e8
>        Vendor Specific Identifier: 0x7c35000
>        Vendor Specific Identifier Extension: 0x30c35000002025
>        [0x60060e8007c350000030c35000002025]
> 
> The current code would use the first descriptor, because it's longer
> than the NAA descriptor. But this is wrong, the kernel is supposed
> to prefer NAA descriptors over T10 vendor ID. Designator length
> should only be used to compare designators of the same type.
> 
> This patch addresses the issue by separating designator priority and
> length.
> 
> Fixes: 9983bed3907c ("scsi: Add scsi_vpd_lun_id()")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/scsi/scsi_lib.c | 126 +++++++++++++++++++++++++++-------------
>   1 file changed, 86 insertions(+), 40 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
Martin K. Petersen Nov. 11, 2020, 3:05 a.m. UTC | #2
Martin,

> The current code would use the first descriptor, because it's longer
> than the NAA descriptor. But this is wrong, the kernel is supposed to
> prefer NAA descriptors over T10 vendor ID. Designator length should
> only be used to compare designators of the same type.
>
> This patch addresses the issue by separating designator priority and
> length.

I am concerned that we're going to break existing systems since their
/dev/disk/by-* names might change as a result of this. Thoughts?
Hannes Reinecke Nov. 11, 2020, 6:54 a.m. UTC | #3
On 11/11/20 4:05 AM, Martin K. Petersen wrote:
> 
> Martin,
> 
>> The current code would use the first descriptor, because it's longer
>> than the NAA descriptor. But this is wrong, the kernel is supposed to
>> prefer NAA descriptors over T10 vendor ID. Designator length should
>> only be used to compare designators of the same type.
>>
>> This patch addresses the issue by separating designator priority and
>> length.
> 
> I am concerned that we're going to break existing systems since their
> /dev/disk/by-* names might change as a result of this. Thoughts?
> 
No, this shouldn't happen. With the standard udev rules we're creating 
symlinks for all possible VPD designators, so they don't change.
The patch really is just for multipath to handle error cases better; 
we've had this situation when reading the vpd page hit an I/O error.

Cheers,

Hannes
Martin Wilck Nov. 11, 2020, 8:17 a.m. UTC | #4
On Wed, 2020-11-11 at 07:54 +0100, Hannes Reinecke wrote:
> On 11/11/20 4:05 AM, Martin K. Petersen wrote:
> > Martin,
> > 
> > > The current code would use the first descriptor, because it's
> > > longer
> > > than the NAA descriptor. But this is wrong, the kernel is
> > > supposed to
> > > prefer NAA descriptors over T10 vendor ID. Designator length
> > > should
> > > only be used to compare designators of the same type.
> > > 
> > > This patch addresses the issue by separating designator priority
> > > and
> > > length.
> > 
> > I am concerned that we're going to break existing systems since
> > their
> > /dev/disk/by-* names might change as a result of this. Thoughts?
> > 
> No, this shouldn't happen. With the standard udev rules we're
> creating 
> symlinks for all possible VPD designators, so they don't change.

Right. On distributions using either udev's scsi_id or the standard
rules shipped with sg3_utils for determining WWIDs, nothing should
change.

With this patch, the kernel's logic would eventually match the logic of
the udev rules, which is a good thing. In the long run, we could
finally ditch the complexity of the udev rules and rely on the kernel
to get the wwid right. That would be a big step forward for device
identification, wrt both reliablity and speed.

Only distributions using non-standard udev rules (generating
/dev/disk/by-wwid from the "wwid" attribute) would be affected. I don't
know if any such distribution currently exist, I haven't seen one. Even
those would only be affected in certain cases like the one I showed in
the commit message.

If this truly worries you, we could introduce a new sysfs attribute
besides "wwid". But I suppose that would rather confuse people. I
strongly believe we should have a sysfs attribute that reliably
provides the "right" WWID to user space.

Regards,
Martin
Martin K. Petersen Nov. 17, 2020, 3:30 a.m. UTC | #5
Martin,

> The current implementation of scsi_vpd_lun_id() uses the designator
> length as an implicit measure of priority. This works most of the
> time, but not always. For example, some Hitachi storage arrays return
> this in VPD 0x83:

Applied to 5.11/scsi-staging, thanks!
Martin K. Petersen Nov. 20, 2020, 3:31 a.m. UTC | #6
On Thu, 29 Oct 2020 18:08:45 +0100, mwilck@suse.com wrote:

> The current implementation of scsi_vpd_lun_id() uses the designator
> length as an implicit measure of priority. This works most of the
> time, but not always. For example, some Hitachi storage arrays return
> this in VPD 0x83:
> 
> VPD INQUIRY: Device Identification page
>   Designation descriptor number 1, descriptor length: 24
>     designator_type: T10 vendor identification,  code_set: ASCII
>     associated with the Addressed logical unit
>       vendor id: HITACHI
>       vendor specific: 5030C3502025
>   Designation descriptor number 2, descriptor length: 6
>     designator_type: vendor specific [0x0],  code_set: Binary
>     associated with the Target port
>       vendor specific: 08 03
>   Designation descriptor number 3, descriptor length: 20
>     designator_type: NAA,  code_set: Binary
>     associated with the Addressed logical unit
>       NAA 6, IEEE Company_id: 0x60e8
>       Vendor Specific Identifier: 0x7c35000
>       Vendor Specific Identifier Extension: 0x30c35000002025
>       [0x60060e8007c350000030c35000002025]
> 
> [...]

Applied to 5.11/scsi-queue, thanks!

[1/2] scsi: core: Fix VPD LUN ID designator priorities
      https://git.kernel.org/mkp/scsi/c/2e4209b3806c
[2/2] scsi: core: Replace while-loop by for-loop in scsi_vpd_lun_id()
      https://git.kernel.org/mkp/scsi/c/16d6317ea438
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 60c7a7d74852..293ee1af62c3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2949,6 +2949,78 @@  void sdev_enable_disk_events(struct scsi_device *sdev)
 }
 EXPORT_SYMBOL(sdev_enable_disk_events);
 
+static unsigned char designator_prio(const unsigned char *d)
+{
+	if (d[1] & 0x30)
+		/* not associated with LUN */
+		return 0;
+
+	if (d[3] == 0)
+		/* invalid length */
+		return 0;
+
+	/*
+	 * Order of preference for lun descriptor:
+	 * - SCSI name string
+	 * - NAA IEEE Registered Extended
+	 * - EUI-64 based 16-byte
+	 * - EUI-64 based 12-byte
+	 * - NAA IEEE Registered
+	 * - NAA IEEE Extended
+	 * - EUI-64 based 8-byte
+	 * - SCSI name string (truncated)
+	 * - T10 Vendor ID
+	 * as longer descriptors reduce the likelyhood
+	 * of identification clashes.
+	 */
+
+	switch (d[1] & 0xf) {
+	case 8:
+		/* SCSI name string, variable-length UTF-8 */
+		return 9;
+	case 3:
+		switch (d[4] >> 4) {
+		case 6:
+			/* NAA registered extended */
+			return 8;
+		case 5:
+			/* NAA registered */
+			return 5;
+		case 4:
+			/* NAA extended */
+			return 4;
+		case 3:
+			/* NAA locally assigned */
+			return 1;
+		default:
+			break;
+		}
+		break;
+	case 2:
+		switch (d[3]) {
+		case 16:
+			/* EUI64-based, 16 byte */
+			return 7;
+		case 12:
+			/* EUI64-based, 12 byte */
+			return 6;
+		case 8:
+			/* EUI64-based, 8 byte */
+			return 3;
+		default:
+			break;
+		}
+		break;
+	case 1:
+		/* T10 vendor ID */
+		return 1;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 /**
  * scsi_vpd_lun_id - return a unique device identification
  * @sdev: SCSI device
@@ -2965,7 +3037,7 @@  EXPORT_SYMBOL(sdev_enable_disk_events);
  */
 int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 {
-	u8 cur_id_type = 0xff;
+	u8 cur_id_prio = 0;
 	u8 cur_id_size = 0;
 	const unsigned char *d, *cur_id_str;
 	const struct scsi_vpd *vpd_pg83;
@@ -2978,20 +3050,6 @@  int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 		return -ENXIO;
 	}
 
-	/*
-	 * Look for the correct descriptor.
-	 * Order of preference for lun descriptor:
-	 * - SCSI name string
-	 * - NAA IEEE Registered Extended
-	 * - EUI-64 based 16-byte
-	 * - EUI-64 based 12-byte
-	 * - NAA IEEE Registered
-	 * - NAA IEEE Extended
-	 * - T10 Vendor ID
-	 * as longer descriptors reduce the likelyhood
-	 * of identification clashes.
-	 */
-
 	/* The id string must be at least 20 bytes + terminating NULL byte */
 	if (id_len < 21) {
 		rcu_read_unlock();
@@ -3001,8 +3059,9 @@  int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 	memset(id, 0, id_len);
 	d = vpd_pg83->data + 4;
 	while (d < vpd_pg83->data + vpd_pg83->len) {
-		/* Skip designators not referring to the LUN */
-		if ((d[1] & 0x30) != 0x00)
+		u8 prio = designator_prio(d);
+
+		if (prio == 0 || cur_id_prio > prio)
 			goto next_desig;
 
 		switch (d[1] & 0xf) {
@@ -3010,28 +3069,19 @@  int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 			/* T10 Vendor ID */
 			if (cur_id_size > d[3])
 				break;
-			/* Prefer anything */
-			if (cur_id_type > 0x01 && cur_id_type != 0xff)
-				break;
+			cur_id_prio = prio;
 			cur_id_size = d[3];
 			if (cur_id_size + 4 > id_len)
 				cur_id_size = id_len - 4;
 			cur_id_str = d + 4;
-			cur_id_type = d[1] & 0xf;
 			id_size = snprintf(id, id_len, "t10.%*pE",
 					   cur_id_size, cur_id_str);
 			break;
 		case 0x2:
 			/* EUI-64 */
-			if (cur_id_size > d[3])
-				break;
-			/* Prefer NAA IEEE Registered Extended */
-			if (cur_id_type == 0x3 &&
-			    cur_id_size == d[3])
-				break;
+			cur_id_prio = prio;
 			cur_id_size = d[3];
 			cur_id_str = d + 4;
-			cur_id_type = d[1] & 0xf;
 			switch (cur_id_size) {
 			case 8:
 				id_size = snprintf(id, id_len,
@@ -3049,17 +3099,14 @@  int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 						   cur_id_str);
 				break;
 			default:
-				cur_id_size = 0;
 				break;
 			}
 			break;
 		case 0x3:
 			/* NAA */
-			if (cur_id_size > d[3])
-				break;
+			cur_id_prio = prio;
 			cur_id_size = d[3];
 			cur_id_str = d + 4;
-			cur_id_type = d[1] & 0xf;
 			switch (cur_id_size) {
 			case 8:
 				id_size = snprintf(id, id_len,
@@ -3072,26 +3119,25 @@  int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 						   cur_id_str);
 				break;
 			default:
-				cur_id_size = 0;
 				break;
 			}
 			break;
 		case 0x8:
 			/* SCSI name string */
-			if (cur_id_size + 4 > d[3])
+			if (cur_id_size > d[3])
 				break;
 			/* Prefer others for truncated descriptor */
-			if (cur_id_size && d[3] > id_len)
-				break;
+			if (d[3] > id_len) {
+				prio = 2;
+				if (cur_id_prio > prio)
+					break;
+			}
+			cur_id_prio = prio;
 			cur_id_size = id_size = d[3];
 			cur_id_str = d + 4;
-			cur_id_type = d[1] & 0xf;
 			if (cur_id_size >= id_len)
 				cur_id_size = id_len - 1;
 			memcpy(id, cur_id_str, cur_id_size);
-			/* Decrease priority for truncated descriptor */
-			if (cur_id_size != id_size)
-				cur_id_size = 6;
 			break;
 		default:
 			break;