[PATCH 1/2] nvme: check duplicate unique identifiers in order

[PATCH 1/2] nvme: check duplicate unique identifiers in order

[Bryan Gurney]

The identifier check in nvme_subsys_check_duplicate_ids returns an error
if any of the identifiers match; however, this causes problems with
PCI-Express NVMe devices that may otherwise have unique identifiers of
higher precedent (for example, unique NGUIDs, but common EUI64s).

Check for duplicate IDs in order of precedence in the NVMe standard:
UUID, then NGUID, then EUI64, in a way that ensures the highest
priority identifer that exists for the namespace will be checked for
uniquness.  If a lower-priority identifier exists and is non-unique, the
higher priority identifier will be unique, and should be used as the
identifier for the namespace.

Suggested-by: John Meneghini <jmeneghi@redhat.com>

Reviewed-by: John Meneghini <jmeneghi@redhat.com>
Signed-off-by: Bryan Gurney <bgurney@redhat.com>
---
 drivers/nvme/host/core.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eb6ea8acb3cc..49ca29c64ca9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3577,15 +3577,29 @@ static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
 
 	lockdep_assert_held(&subsys->lock);
 
-	list_for_each_entry(h, &subsys->nsheads, entry) {
-		if (has_uuid && uuid_equal(&ids->uuid, &h->ids.uuid))
-			return -EINVAL;
-		if (has_nguid &&
-		    memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0)
-			return -EINVAL;
-		if (has_eui64 &&
-		    memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0)
-			return -EINVAL;
+	if (has_uuid) {
+		list_for_each_entry(h, &subsys->nsheads, entry)
+			if (uuid_equal(&ids->uuid, &h->ids.uuid))
+				return -EINVAL;
+		return 0;
+	}
+
+	if (has_nguid) {
+		list_for_each_entry(h, &subsys->nsheads, entry)
+			if (memcmp(&ids->nguid,
+				   &h->ids.nguid,
+				   sizeof(ids->nguid)) == 0)
+				return -EINVAL;
+		return 0;
+	}
+
+	if (has_eui64) {
+		list_for_each_entry(h, &subsys->nsheads, entry)
+			if (memcmp(&ids->eui64,
+				   &h->ids.eui64,
+				   sizeof(ids->eui64)) == 0)
+				return -EINVAL;
+		return 0;
 	}
 
 	return 0;
-- 
2.49.0

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Keith Busch]

On Fri, May 09, 2025 at 11:16:27AM -0400, Bryan Gurney wrote:
> +	if (has_uuid) {
> +		list_for_each_entry(h, &subsys->nsheads, entry)
> +			if (uuid_equal(&ids->uuid, &h->ids.uuid))
> +				return -EINVAL;
> +		return 0;
> +	}

You're returning success too early here. It may have a unique uuid, but
that doesn't mean we don't want to check for nguid and eui64 too. If
those are not unique, they need to be blanked out to make sure we don't
expose these on the namespace's sysfs attributes.

> +
> +	if (has_nguid) {
> +		list_for_each_entry(h, &subsys->nsheads, entry)
> +			if (memcmp(&ids->nguid,
> +				   &h->ids.nguid,
> +				   sizeof(ids->nguid)) == 0)
> +				return -EINVAL;
> +		return 0;
> +	}
> +
> +	if (has_eui64) {
> +		list_for_each_entry(h, &subsys->nsheads, entry)
> +			if (memcmp(&ids->eui64,
> +				   &h->ids.eui64,
> +				   sizeof(ids->eui64)) == 0)
> +				return -EINVAL;
> +		return 0;
>  	}
>  
>  	return 0;

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[John Meneghini]

On 5/9/25 11:33 AM, Keith Busch wrote:
> On Fri, May 09, 2025 at 11:16:27AM -0400, Bryan Gurney wrote:
>> +	if (has_uuid) {
>> +		list_for_each_entry(h, &subsys->nsheads, entry)
>> +			if (uuid_equal(&ids->uuid, &h->ids.uuid))
>> +				return -EINVAL;
>> +		return 0;
>> +	}
> You're returning success too early here. It may have a unique uuid, but
> that doesn't mean we don't want to check for nguid and eui64 too. 

That's the point of this patch. Once we find a unique id we stop and return before checking the lower precedent IDs.

As it turns out more that one drive reports multiple IDs, but the lower precedent IDs are not unique. We've found this to be
especially true with drives that support namespace management. When you start creating more NSIDs, the drive creates new and
unique NGIUD for each new namepspace, but the EUI64 is duplicated/the same.

> If those are not unique, they need to be blanked out to make sure we don't
> expose these on the namespace's sysfs attributes.

OK, we can add another patch that blanks out the lower precedent/invalid IDs.

Will that work?

/John

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Keith Busch]

On Mon, May 12, 2025 at 11:18:58AM -0400, John Meneghini wrote:
> On 5/9/25 11:33 AM, Keith Busch wrote:
> 
> > If those are not unique, they need to be blanked out to make sure we don't
> > expose these on the namespace's sysfs attributes.
> 
> OK, we can add another patch that blanks out the lower precedent/invalid IDs.
> 
> Will that work?

I think you'd have to blank out the non-unique ones, otherwise the
driver would export them.

And while I think that might work, this proposal didn't go over so well
last time it came up:

https://lore.kernel.org/linux-nvme/20250414111916.GB13225@lst.de/

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Keith Busch]

On Mon, May 12, 2025 at 10:12:29AM -0600, Keith Busch wrote:
> On Mon, May 12, 2025 at 11:18:58AM -0400, John Meneghini wrote:
> > On 5/9/25 11:33 AM, Keith Busch wrote:
> > 
> > > If those are not unique, they need to be blanked out to make sure we don't
> > > expose these on the namespace's sysfs attributes.
> > 
> > OK, we can add another patch that blanks out the lower precedent/invalid IDs.
> > 
> > Will that work?
> 
> I think you'd have to blank out the non-unique ones, otherwise the
> driver would export them.
> 
> And while I think that might work, this proposal didn't go over so well
> last time it came up:
> 
> https://lore.kernel.org/linux-nvme/20250414111916.GB13225@lst.de/

I've recently encountered some devices with this behavior, from machines
hosted by various cloud providers. I had to pull in something
out-of-tree like the patch here just to get things going. I think we
should have the kernel discard lower priority fields.

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[John Meneghini]

On 6/24/25 11:16 AM, Keith Busch wrote:
> On Mon, May 12, 2025 at 10:12:29AM -0600, Keith Busch wrote:
>> On Mon, May 12, 2025 at 11:18:58AM -0400, John Meneghini wrote:
>>> On 5/9/25 11:33 AM, Keith Busch wrote:
>>>
>>>> If those are not unique, they need to be blanked out to make sure we don't
>>>> expose these on the namespace's sysfs attributes.
>>>
>>> OK, we can add another patch that blanks out the lower precedent/invalid IDs.
>>>
>>> Will that work?
>>
>> I think you'd have to blank out the non-unique ones, otherwise the
>> driver would export them.
>>
>> And while I think that might work, this proposal didn't go over so well
>> last time it came up:
>>
>> https://lore.kernel.org/linux-nvme/20250414111916.GB13225@lst.de/
> 
> I've recently encountered some devices with this behavior, from machines
> hosted by various cloud providers. I had to pull in something
> out-of-tree like the patch here just to get things going. I think we
> should have the kernel discard lower priority fields.

OK good I will ask Bryan to resubmit this as a version 2 patch.
  
We can add another patch that blanks out the lower precedent/invalid IDs in sysfs so that only the unique IDs are reported.

/John.

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Keith Busch]

On Tue, Jul 15, 2025 at 03:01:59PM -0400, John Meneghini wrote:
> On 6/24/25 11:16 AM, Keith Busch wrote:
> > On Mon, May 12, 2025 at 10:12:29AM -0600, Keith Busch wrote:
> > > On Mon, May 12, 2025 at 11:18:58AM -0400, John Meneghini wrote:
> > > > On 5/9/25 11:33 AM, Keith Busch wrote:
> > > > 
> > > > > If those are not unique, they need to be blanked out to make sure we don't
> > > > > expose these on the namespace's sysfs attributes.
> > > > 
> > > > OK, we can add another patch that blanks out the lower precedent/invalid IDs.
> > > > 
> > > > Will that work?
> > > 
> > > I think you'd have to blank out the non-unique ones, otherwise the
> > > driver would export them.
> > > 
> > > And while I think that might work, this proposal didn't go over so well
> > > last time it came up:
> > > 
> > > https://lore.kernel.org/linux-nvme/20250414111916.GB13225@lst.de/
> > 
> > I've recently encountered some devices with this behavior, from machines
> > hosted by various cloud providers. I had to pull in something
> > out-of-tree like the patch here just to get things going. I think we
> > should have the kernel discard lower priority fields.
> 
> OK good I will ask Bryan to resubmit this as a version 2 patch.
> We can add another patch that blanks out the lower precedent/invalid IDs in sysfs so that only the unique IDs are reported.

Note, it's not me who needs convincing.

FWIW, this is what I'm running with. There's just some extra care here
to ensure we refresh sysfs attributes as needed, as well as prevent
repeatedly spamming the kernel messages with the same error.

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 12e7ae1f99e20..13482b2fbf46a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1467,7 +1467,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 				 warn_str, cur->nidl);
 			return -1;
 		}
-		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID ||
+		    ctrl->subsys->quirks & NVME_SUBSYS_BAD_EUI64)
 			return NVME_NIDT_EUI64_LEN;
 		memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN);
 		return NVME_NIDT_EUI64_LEN;
@@ -1477,7 +1478,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 				 warn_str, cur->nidl);
 			return -1;
 		}
-		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID ||
+		    ctrl->subsys->quirks & NVME_SUBSYS_BAD_NGUID)
 			return NVME_NIDT_NGUID_LEN;
 		memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN);
 		return NVME_NIDT_NGUID_LEN;
@@ -1611,10 +1613,12 @@ static int nvme_ns_info_from_identify(struct nvme_ctrl *ctrl,
 			 "Ignoring bogus Namespace Identifiers\n");
 	} else {
 		if (ctrl->vs >= NVME_VS(1, 1, 0) &&
-		    !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
+		    !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)) &&
+		    !(ctrl->subsys->quirks & NVME_SUBSYS_BAD_EUI64))
 			memcpy(ids->eui64, id->eui64, sizeof(ids->eui64));
 		if (ctrl->vs >= NVME_VS(1, 2, 0) &&
-		    !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
+		    !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) &&
+		    !(ctrl->subsys->quirks & NVME_SUBSYS_BAD_NGUID))
 			memcpy(ids->nguid, id->nguid, sizeof(ids->nguid));
 	}
 
@@ -3554,7 +3558,7 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_ctrl *ctrl,
 }
 
 static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
-		struct nvme_ns_ids *ids)
+		struct nvme_ns_ids *ids, bool global)
 {
 	bool has_uuid = !uuid_is_null(&ids->uuid);
 	bool has_nguid = memchr_inv(ids->nguid, 0, sizeof(ids->nguid));
@@ -3564,14 +3568,39 @@ static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
 	lockdep_assert_held(&subsys->lock);
 
 	list_for_each_entry(h, &subsys->nsheads, entry) {
+		bool changed = false;
+
 		if (has_uuid && uuid_equal(&ids->uuid, &h->ids.uuid))
 			return -EINVAL;
 		if (has_nguid &&
-		    memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0)
-			return -EINVAL;
+		    memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0) {
+			if (!has_uuid || global)
+				return -EINVAL;
+
+			dev_warn(&subsys->dev, "duplicate nguid:%16phN\n",
+				ids->nguid);
+			memset(ids->nguid, 0, sizeof(ids->nguid));
+			memset(h->ids.nguid, 0, sizeof(h->ids.nguid));
+			subsys->quirks |= NVME_SUBSYS_BAD_NGUID;
+			has_nguid = false;
+			changed = true;
+		}
 		if (has_eui64 &&
-		    memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0)
-			return -EINVAL;
+		    memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0) {
+			if ((!has_uuid && !has_nguid) || global)
+				return -EINVAL;
+
+			dev_warn(&subsys->dev, "duplicate eui64:%8phN\n",
+				ids->eui64);
+			memset(ids->eui64, 0, sizeof(ids->eui64));
+			memset(h->ids.eui64, 0, sizeof(h->ids.eui64));
+			subsys->quirks |= NVME_SUBSYS_BAD_EUI64;
+			has_eui64 = false;
+			changed = true;
+		}
+
+		if (changed)
+			nvme_update_ns_attrs(h);
 	}
 
 	return 0;
@@ -3719,7 +3748,7 @@ static int nvme_global_check_duplicate_ids(struct nvme_subsystem *this,
 		if (s == this)
 			continue;
 		mutex_lock(&s->lock);
-		ret = nvme_subsys_check_duplicate_ids(s, ids);
+		ret = nvme_subsys_check_duplicate_ids(s, ids, true);
 		mutex_unlock(&s->lock);
 		if (ret)
 			break;
@@ -3776,7 +3805,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
 	mutex_lock(&ctrl->subsys->lock);
 	head = nvme_find_ns_head(ctrl, info->nsid);
 	if (!head) {
-		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &info->ids);
+		ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &info->ids, false);
 		if (ret) {
 			dev_err(ctrl->device,
 				"duplicate IDs in subsystem for nsid %d\n",
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index c4bb8dfe1a458..b81fe820280c2 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -447,6 +447,10 @@ struct nvme_subsystem {
 #ifdef CONFIG_NVME_MULTIPATH
 	enum nvme_iopolicy	iopolicy;
 #endif
+
+#define NVME_SUBSYS_BAD_NGUID	(1 << 0)
+#define NVME_SUBSYS_BAD_EUI64	(1 << 1)
+	unsigned long		quirks;
 };
 
 /*
@@ -1181,6 +1185,7 @@ struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
 struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
 bool nvme_get_ns(struct nvme_ns *ns);
 void nvme_put_ns(struct nvme_ns *ns);
+void nvme_update_ns_attrs(struct nvme_ns_head *head);
 
 static inline bool nvme_multi_css(struct nvme_ctrl *ctrl)
 {
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index b68a9e5f1ea39..ef232e0da5f72 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -299,6 +299,18 @@ static const struct attribute_group nvme_ns_attr_group = {
 	.is_visible	= nvme_ns_attrs_are_visible,
 };
 
+void nvme_update_ns_attrs(struct nvme_ns_head *head)
+{
+	struct gendisk *disk;
+
+	if (nvme_ns_head_multipath(head))
+		disk = head->disk;
+	else
+		disk = list_first_entry(&head->list, struct nvme_ns,
+					siblings)->disk;
+	sysfs_update_group(&disk_to_dev(disk)->kobj, &nvme_ns_attr_group);
+}
+
 const struct attribute_group *nvme_ns_attr_groups[] = {
 	&nvme_ns_attr_group,
 	NULL,
--

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Hannes Reinecke]

On 7/15/25 21:29, Keith Busch wrote:
> On Tue, Jul 15, 2025 at 03:01:59PM -0400, John Meneghini wrote:
>> On 6/24/25 11:16 AM, Keith Busch wrote:
>>> On Mon, May 12, 2025 at 10:12:29AM -0600, Keith Busch wrote:
>>>> On Mon, May 12, 2025 at 11:18:58AM -0400, John Meneghini wrote:
>>>>> On 5/9/25 11:33 AM, Keith Busch wrote:
>>>>>
>>>>>> If those are not unique, they need to be blanked out to make sure we don't
>>>>>> expose these on the namespace's sysfs attributes.
>>>>>
>>>>> OK, we can add another patch that blanks out the lower precedent/invalid IDs.
>>>>>
>>>>> Will that work?
>>>>
>>>> I think you'd have to blank out the non-unique ones, otherwise the
>>>> driver would export them.
>>>>
>>>> And while I think that might work, this proposal didn't go over so well
>>>> last time it came up:
>>>>
>>>> https://lore.kernel.org/linux-nvme/20250414111916.GB13225@lst.de/
>>>
>>> I've recently encountered some devices with this behavior, from machines
>>> hosted by various cloud providers. I had to pull in something
>>> out-of-tree like the patch here just to get things going. I think we
>>> should have the kernel discard lower priority fields.
>>
>> OK good I will ask Bryan to resubmit this as a version 2 patch.
>> We can add another patch that blanks out the lower precedent/invalid IDs in sysfs so that only the unique IDs are reported.
> 
> Note, it's not me who needs convincing.
> 
> FWIW, this is what I'm running with. There's just some extra care here
> to ensure we refresh sysfs attributes as needed, as well as prevent
> repeatedly spamming the kernel messages with the same error.
> 
Ah, so you got another one?

We got one, too (which is essentially that one posted on the mailing list).

If we now can get Oracle to chime in (maybe with a different patch
altogether?) we have all major vendors implementing different fixes
for the same problem.

Fun ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Martin K. Petersen]

Hannes,

> If we now can get Oracle to chime in (maybe with a different patch
> altogether?) we have all major vendors implementing different fixes
> for the same problem.

We have posted our quirk patches on several occasions.

The challenge is how to work around duplicate identifiers while reducing
the number of cases where /dev/disk/by-foo would end up changing. Either
as a result of a firmware update or as a result of updating kernels.

The issue is pervasive enough that OCP has a defined EUI64 workaround in
their SSD spec.

-- 
Martin K. Petersen

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[John Meneghini]

On 7/22/25 7:58 AM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> If we now can get Oracle to chime in (maybe with a different patch
>> altogether?) we have all major vendors implementing different fixes
>> for the same problem.
> 
> We have posted our quirk patches on several occasions.
> 
> The challenge is how to work around duplicate identifiers while reducing
> the number of cases where /dev/disk/by-foo would end up changing. Either
> as a result of a firmware update or as a result of updating kernels.

Not sure that's possible because we can only control what the Kernel does.

If a firmware update is going to change, add, or remove an EUI64 or NGUID or UUID identifier
we can't fix that.  But we can enforce a uniform set of rules in the kernel that does the
correct thing based up the spec.
  
> The issue is pervasive enough that OCP has a defined EUI64 workaround in
> their SSD spec.

https://www.opencompute.org/documents/datacenter-nvme-ssd-specification-v2-6-2-pdf

Is this the specification you are referring to?

NVMe-CFG-7
The single default namespace (see SECTOR-3) shall have a unique non-zero value in the
EUI64 (Extended Unique Identifier) field in the Identify Namespace data structure.
For each namespace that is created other than the initial namespace configured in the
factory, the device shall clear the EUI64 field in the Identify Namespace data structure to 0h.

NVMe-CFG-8
The device shall support a non-zero NGUID per Namespace that is never reused (i.e., the
UIDREUSE bit in the Common Namespace Features field of the Identify Namespace data
structure shall be set to 1b).

/John

Re: [PATCH 1/2] nvme: check duplicate unique identifiers in order

[Martin K. Petersen]

John,

>> The issue is pervasive enough that OCP has a defined EUI64 workaround
>> in their SSD spec.

[...]

> https://www.opencompute.org/documents/datacenter-nvme-ssd-specification-v2-6-2-pdf
>
> Is this the specification you are referring to?
>
> NVMe-CFG-7

> The single default namespace (see SECTOR-3) shall have a unique
> non-zero value in the EUI64 (Extended Unique Identifier) field in the
> Identify Namespace data structure. For each namespace that is created
> other than the initial namespace configured in the factory, the device
> shall clear the EUI64 field in the Identify Namespace data structure
> to 0h.

Correct.

I suspect there are a bazillion deployed drives which suffer from the
non-unique identifier problem. And thus the path of least resistance
appears to be to avoid reporting EUI64 for subsequent namespaces after a
firmware update. At least that keeps the existing EUI64 stable for the
common case where only a single namespace is provisioned.

-- 
Martin K. Petersen