quirk broken namespace identifiers

quirk broken namespace identifiers

[Christoph Hellwig]

Hi all,

this series adds a quirk to ignore namespace identifiers on controllers
where they are completely broken and then quirks two controllers for it.

[PATCH 1/3] nvme: add a quirk to disable namespace identifiers

[Christoph Hellwig]

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

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 efb85c6d8e2d5..41314dccb2209 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1282,6 +1282,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 
 	switch (cur->nidt) {
 	case NVME_NIDT_EUI64:
+		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+			return NVME_NIDT_EUI64_LEN;
 		if (cur->nidl != NVME_NIDT_EUI64_LEN) {
 			dev_warn(ctrl->device, "%s %d for NVME_NIDT_EUI64\n",
 				 warn_str, cur->nidl);
@@ -1290,6 +1292,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 		memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN);
 		return NVME_NIDT_EUI64_LEN;
 	case NVME_NIDT_NGUID:
+		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+			return NVME_NIDT_NGUID_LEN;
 		if (cur->nidl != NVME_NIDT_NGUID_LEN) {
 			dev_warn(ctrl->device, "%s %d for NVME_NIDT_NGUID\n",
 				 warn_str, cur->nidl);
@@ -1298,6 +1302,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 		memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN);
 		return NVME_NIDT_NGUID_LEN;
 	case NVME_NIDT_UUID:
+		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+			return NVME_NIDT_UUID_LEN;
 		if (cur->nidl != NVME_NIDT_UUID_LEN) {
 			dev_warn(ctrl->device, "%s %d for NVME_NIDT_UUID\n",
 				 warn_str, cur->nidl);
@@ -1399,12 +1405,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 1393bbf82d71e..a2b53ca633359 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -144,6 +144,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.30.2

Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers

[Chaitanya Kulkarni]

On 4/11/22 23:11, Christoph Hellwig wrote:
> Add a quirk to disable using and exporting namespace identifiers for
> controllers where they are broken beyond repair.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---


if possible take that quirk check out see [1] it is but more
code but look much clean, either way looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1] totally untested :-


+static int nvme_id_check_quirk(u64 nidt)
+{
+       switch (cur->nidt) {
+       case NVME_NIDT_EUI64:
+               if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+                       return NVME_NIDT_EUI64_LEN;
+               return 0;
+       case NVME_NIDT_NGUID:
+               if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+                       return NVME_NIDT_NGUID_LEN;
+               return 0;
+       case NVME_NIDT_UUID:
+               if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
+                       return NVME_NIDT_UUID_LEN;
+               return 0;
+       default:
+               return 0;
+       }
+}
+
  static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct 
nvme_ns_ids *ids,
                 struct nvme_ns_id_desc *cur, bool *csi_seen)
  {
         const char *warn_str = "ctrl returned bogus length:";
         void *data = cur;
+       int ret;
+
+       ret = nvme_id_check_quirk(cur->nidt);
+       if (ret)
+               return ret;

         switch (cur->nidt) {
         case NVME_NIDT_EUI64:

Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers

[Christoph Hellwig]

On Tue, Apr 12, 2022 at 06:56:07AM +0000, Chaitanya Kulkarni wrote:
> +       ret = nvme_id_check_quirk(cur->nidt);
> +       if (ret)
> +               return ret;

We still need to process the list to find the CSI.  This is actually
relevant for modern qemu, which supports multiple command sets.

Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/3] nvme: add a quirk to disable namespace identifiers

[Keith Busch]

On Tue, Apr 12, 2022 at 08:11:24AM +0200, Christoph Hellwig wrote:
> @@ -1282,6 +1282,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
>  
>  	switch (cur->nidt) {
>  	case NVME_NIDT_EUI64:
> +		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
> +			return NVME_NIDT_EUI64_LEN;
>  		if (cur->nidl != NVME_NIDT_EUI64_LEN) {
>  			dev_warn(ctrl->device, "%s %d for NVME_NIDT_EUI64\n",
>  				 warn_str, cur->nidl);

Shouldn't we still verify that cur->nidl is accurate rather than assume the
length? If it is the wrong length, then we have a corrupt descriptor list and
should abort the process.

[PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202

[Christoph Hellwig]

The MAXIO MAP1202 controllers reports completely bogus Namespace
identifiers that even change after suspend cycles.  Disable using
the Identifiers entirely.

Reported-by: 金韬 <me@kingtous.cn>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: 金韬 <me@kingtous.cn>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d817ca17463ed..c386c91483505 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3447,6 +3447,8 @@ static const struct pci_device_id nvme_id_table[] = {
 		.driver_data = NVME_QUIRK_NO_DEEPEST_PS, },
 	{ PCI_DEVICE(0x2646, 0x2263),   /* KINGSTON A2000 NVMe SSD  */
 		.driver_data = NVME_QUIRK_NO_DEEPEST_PS, },
+	{ PCI_DEVICE(0x1e4B, 0x1202),   /* MAXIO MAP1202 */
+		.driver_data = NVME_QUIRK_BOGUS_NID, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMAZON, 0x0061),
 		.driver_data = NVME_QUIRK_DMA_ADDRESS_BITS_48, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMAZON, 0x0065),
-- 
2.30.2

Re: [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202

[Chaitanya Kulkarni]

On 4/11/22 23:11, Christoph Hellwig wrote:
> The MAXIO MAP1202 controllers reports completely bogus Namespace
> identifiers that even change after suspend cycles.  Disable using
> the Identifiers entirely.
> 
> Reported-by: 金韬 <me@kingtous.cn>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: 金韬 <me@kingtous.cn>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 2/3] nvme-pci: disable namespace identifiers for the MAXIO MAP1202

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers

[Christoph Hellwig]

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
corretly through manual intervention.

Reported-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c386c91483505..b191e7dcf15ca 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3410,6 +3410,8 @@ static const struct pci_device_id nvme_id_table[] = {
 	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
 		.driver_data = NVME_QUIRK_IDENTIFY_CNS |
 				NVME_QUIRK_DISABLE_WRITE_ZEROES, },
+	{ 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.30.2

Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Apr 12 08:11, Christoph Hellwig wrote:
> 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
> corretly through manual intervention.
> 
> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index c386c91483505..b191e7dcf15ca 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3410,6 +3410,8 @@ static const struct pci_device_id nvme_id_table[] = {
>  	{ PCI_VDEVICE(INTEL, 0x5845),	/* Qemu emulated controller */
>  		.driver_data = NVME_QUIRK_IDENTIFY_CNS |
>  				NVME_QUIRK_DISABLE_WRITE_ZEROES, },
> +	{ 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.30.2
> 

When I fix this in QEMU properly, can we move this quirk to the
core_quirks and match on firmware revision? That way I don't have to
request a new DID.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers

[Christoph Hellwig]

On Tue, Apr 12, 2022 at 08:33:38AM +0200, Klaus Jensen wrote:
> > +	{ 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.30.2
> > 
> 
> When I fix this in QEMU properly, can we move this quirk to the
> core_quirks and match on firmware revision? That way I don't have to
> request a new DID.

Do we known that only one firmware revision reported by Qemu is actually
broken?  For now I'd like to get the regression fixed ASAP, and I don't
think Qemu ever fully got identifiers right so far.  We can always
change it later if needed.

Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On Apr 12 13:45, Christoph Hellwig wrote:
> On Tue, Apr 12, 2022 at 08:33:38AM +0200, Klaus Jensen wrote:
> > > +	{ 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.30.2
> > > 
> > 
> > When I fix this in QEMU properly, can we move this quirk to the
> > core_quirks and match on firmware revision? That way I don't have to
> > request a new DID.
> 
> Do we known that only one firmware revision reported by Qemu is actually
> broken?  For now I'd like to get the regression fixed ASAP, and I don't
> think Qemu ever fully got identifiers right so far.  We can always
> change it later if needed.
> 

To my knowledge, QEMU has only ever used 1.0 in FR, but I will check.

In any case, I did not want you to change this *now*, so I am totally
fine with this getting changed later if required.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] nvme-pci: disable namespace identifiers for Qemu controllers

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

quirk broken namespace identifiers v2

[Christoph Hellwig]

Hi all,

this series adds a quirk to ignore namespace identifiers on controllers
where they are completely broken and then quirks two controllers for it.

Changes since v1:
 - validate namespace descriptor sizes even if ignoring them
 - also handle another MAXIO controller with the same problem
 - fix up a few commit messages

[PATCH 1/3] nvme: add a quirk to disable namespace identifiers

[Christoph Hellwig]

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: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 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 be9fc9818e650..e1846d04817f3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1288,6 +1288,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:
@@ -1296,6 +1298,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:
@@ -1304,6 +1308,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:
@@ -1400,12 +1406,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 1393bbf82d71e..a2b53ca633359 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -144,6 +144,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.30.2