| Message ID | 20201029170846.14786-1-mwilck@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] scsi: scsi_vpd_lun_id(): fix designator priorities | expand |
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
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
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
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
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, > 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
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
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;