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

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

[mwilck]

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(-)

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;
-- 
2.29.0

[PATCH 2/2] scsi: scsi_vpd_lun_id: replace while-loop by for-loop

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This makes the code slightly more readable.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_lib.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 293ee1af62c3..a20f3e4b3e9b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3057,12 +3057,13 @@ 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) {
+	for (d = vpd_pg83->data + 4;
+	     d < vpd_pg83->data + vpd_pg83->len;
+	     d += d[3] + 4) {
 		u8 prio = designator_prio(d);
 
 		if (prio == 0 || cur_id_prio > prio)
-			goto next_desig;
+			continue;
 
 		switch (d[1] & 0xf) {
 		case 0x1:
@@ -3142,8 +3143,6 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
 		default:
 			break;
 		}
-next_desig:
-		d += d[3] + 4;
 	}
 	rcu_read_unlock();
 
-- 
2.29.0

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

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/2] scsi: scsi_vpd_lun_id: replace while-loop by for-loop

[Hannes Reinecke]

On 10/29/20 6:08 PM, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This makes the code slightly more readable.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/scsi/scsi_lib.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
If you insist ...

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, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

[Martin K. Petersen]

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?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

[Martin Wilck]

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

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

[Martin K. Petersen]

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	Oracle Linux Engineering

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

[Martin K. Petersen]

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

-- 
Martin K. Petersen	Oracle Linux Engineering