[PATCH AUTOSEL 5.10 17/18] nvme: add a quirk to disable namespace identifiers

[PATCH AUTOSEL 5.10 17/18] nvme: add a quirk to disable namespace identifiers

[Sasha Levin]

From: Christoph Hellwig <hch@lst.de>

[ Upstream commit 00ff400e6deee00f7b15e200205b2708b63b8cf6 ]

Add a quirk to disable using and exporting namespace identifiers for
controllers where they are broken beyond repair.

The most directly visible problem with non-unique namespace identifiers
is that they break the /dev/disk/by-id/ links, with the link for a
supposedly unique identifier now pointing to one of multiple possible
namespaces that share the same ID, and a somewhat random selection of
which one actually shows up.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/nvme/host/core.c | 24 ++++++++++++++++++------
 drivers/nvme/host/nvme.h |  5 +++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 853b9a24f744..ad4f1cfbad2e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1270,6 +1270,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)
+			return NVME_NIDT_EUI64_LEN;
 		memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN);
 		return NVME_NIDT_EUI64_LEN;
 	case NVME_NIDT_NGUID:
@@ -1278,6 +1280,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)
+			return NVME_NIDT_NGUID_LEN;
 		memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN);
 		return NVME_NIDT_NGUID_LEN;
 	case NVME_NIDT_UUID:
@@ -1286,6 +1290,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)
+			return NVME_NIDT_UUID_LEN;
 		uuid_copy(&ids->uuid, data + sizeof(*cur));
 		return NVME_NIDT_UUID_LEN;
 	case NVME_NIDT_CSI:
@@ -1381,12 +1387,18 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if ((*id)->ncap == 0) /* namespace not allocated or attached */
 		goto out_free_id;
 
-	if (ctrl->vs >= NVME_VS(1, 1, 0) &&
-	    !memchr_inv(ids->eui64, 0, sizeof(ids->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)))
-		memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
+
+	if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
+		dev_info(ctrl->device,
+			 "Ignoring bogus Namespace Identifiers\n");
+	} else {
+		if (ctrl->vs >= NVME_VS(1, 1, 0) &&
+		    !memchr_inv(ids->eui64, 0, sizeof(ids->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)))
+			memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
+	}
 
 	return 0;
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5dd1dd8021ba..10e5ae3a8c0d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -150,6 +150,11 @@ enum nvme_quirks {
 	 * encoding the generation sequence number.
 	 */
 	NVME_QUIRK_SKIP_CID_GEN			= (1 << 17),
+
+	/*
+	 * Reports garbage in the namespace identifiers (eui64, nguid, uuid).
+	 */
+	NVME_QUIRK_BOGUS_NID			= (1 << 18),
 };
 
 /*
-- 
2.35.1

[PATCH AUTOSEL 5.10 18/18] nvme-pci: disable namespace identifiers for Qemu controllers

[Sasha Levin]

From: Christoph Hellwig <hch@lst.de>

[ Upstream commit 66dd346b84d79fde20832ed691a54f4881eac20d ]

Qemu unconditionally reports a UUID, which depending on the qemu version
is either all-null (which is incorrect but harmless) or contains a single
bit set for all controllers.  In addition it can also optionally report
a eui64 which needs to be manually set.  Disable namespace identifiers
for Qemu controlles entirely even if in some cases they could be set
correctly through manual intervention.

Reported-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/nvme/host/pci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 97afeb898b25..6939b03a16c5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3212,7 +3212,10 @@ static const struct pci_device_id nvme_id_table[] = {
 		.driver_data = NVME_QUIRK_IGNORE_DEV_SUBNQN, },
 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
 		.driver_data = NVME_QUIRK_IDENTIFY_CNS |
-				NVME_QUIRK_DISABLE_WRITE_ZEROES, },
+				NVME_QUIRK_DISABLE_WRITE_ZEROES |
+				NVME_QUIRK_BOGUS_NID, },
+	{ PCI_VDEVICE(REDHAT, 0x0010),	/* Qemu emulated controller */
+		.driver_data = NVME_QUIRK_BOGUS_NID, },
 	{ PCI_DEVICE(0x126f, 0x2263),	/* Silicon Motion unidentified */
 		.driver_data = NVME_QUIRK_NO_NS_DESC_LIST, },
 	{ PCI_DEVICE(0x1bb1, 0x0100),   /* Seagate Nytro Flash Storage */
-- 
2.35.1

Re: [PATCH AUTOSEL 5.10 18/18] nvme-pci: disable namespace identifiers for Qemu controllers

[Luis Chamberlain]

On Tue, Apr 19, 2022 at 02:13:52PM -0400, Sasha Levin wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> [ Upstream commit 66dd346b84d79fde20832ed691a54f4881eac20d ]
> 
> Qemu unconditionally reports a UUID, which depending on the qemu version
> is either all-null (which is incorrect but harmless) or contains a single
> bit set for all controllers.  In addition it can also optionally report
> a eui64 which needs to be manually set.  Disable namespace identifiers
> for Qemu controlles entirely even if in some cases they could be set
> correctly through manual intervention.
> 
> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

Huh? The NVME_QUIRK_BOGUS_NID is a new define and the code which uses
this quirk is also new, and so I'm curious *how and why* the auto-sel
stuff for stable can decide to merge this and this should not even
compile? I see this was backported to v5.15  and v5.17 as well.

I didn't get Cc'd on perhaps some other patches, but this immediately
caught my attention as not applicable, unless of course the patch
"nvme: add a quirk to disable namespace identifiers" was also sent
as part of this series to stable kernels. And if that was done, well
holy crap, really?

Cc'ing Klaus on the qemu side of things so he's aware.

  Luis

> ---
>  drivers/nvme/host/pci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 97afeb898b25..6939b03a16c5 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3212,7 +3212,10 @@ static const struct pci_device_id nvme_id_table[] = {
>  		.driver_data = NVME_QUIRK_IGNORE_DEV_SUBNQN, },
>  	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
>  		.driver_data = NVME_QUIRK_IDENTIFY_CNS |
> -				NVME_QUIRK_DISABLE_WRITE_ZEROES, },
> +				NVME_QUIRK_DISABLE_WRITE_ZEROES |
> +				NVME_QUIRK_BOGUS_NID, },
> +	{ PCI_VDEVICE(REDHAT, 0x0010),	/* Qemu emulated controller */
> +		.driver_data = NVME_QUIRK_BOGUS_NID, },
>  	{ PCI_DEVICE(0x126f, 0x2263),	/* Silicon Motion unidentified */
>  		.driver_data = NVME_QUIRK_NO_NS_DESC_LIST, },
>  	{ PCI_DEVICE(0x1bb1, 0x0100),   /* Seagate Nytro Flash Storage */
> -- 
> 2.35.1
> 

Re: [PATCH AUTOSEL 5.10 18/18] nvme-pci: disable namespace identifiers for Qemu controllers

[Sasha Levin]

On Wed, Apr 20, 2022 at 11:52:36AM -0700, Luis Chamberlain wrote:
>On Tue, Apr 19, 2022 at 02:13:52PM -0400, Sasha Levin wrote:
>> From: Christoph Hellwig <hch@lst.de>
>>
>> [ Upstream commit 66dd346b84d79fde20832ed691a54f4881eac20d ]
>>
>> Qemu unconditionally reports a UUID, which depending on the qemu version
>> is either all-null (which is incorrect but harmless) or contains a single
>> bit set for all controllers.  In addition it can also optionally report
>> a eui64 which needs to be manually set.  Disable namespace identifiers
>> for Qemu controlles entirely even if in some cases they could be set
>> correctly through manual intervention.
>>
>> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Keith Busch <kbusch@kernel.org>
>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>
>Huh? The NVME_QUIRK_BOGUS_NID is a new define and the code which uses
>this quirk is also new, and so I'm curious *how and why* the auto-sel
>stuff for stable can decide to merge this and this should not even
>compile? I see this was backported to v5.15  and v5.17 as well.

Because we take quirks for -stable?

It does compile apparently... At least with the configs we test.

>I didn't get Cc'd on perhaps some other patches, but this immediately
>caught my attention as not applicable, unless of course the patch
>"nvme: add a quirk to disable namespace identifiers" was also sent
>as part of this series to stable kernels. And if that was done, well
>holy crap, really?

Yes, it was.

I'm not sure why we shouldn't be picking it up?

-- 
Thanks,
Sasha