[PATCH 0/2] nvme: validate CNTLID

[PATCH 0/2] nvme: validate CNTLID

[Hannes Reinecke]

Hi all,

here are two patches to validate correct CNTLID information.
A controller might violate the constrain the each CNTLID has to
be unique within a subsystem, which then would cause the host
to crash.
So these patches prevent this situation by validate the CNTLID
and not use the cntlid as part of the device name.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  nvme-multipath: avoid crash on invalid subsystem cntlid enumeration
  nvme: validate cntlid during controller initialisation

 drivers/nvme/host/core.c      | 16 +++++++++++++++-
 drivers/nvme/host/multipath.c |  2 +-
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.16.4

[PATCH 1/2] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Hannes Reinecke]

A process holding an open reference to a removed disk prevents it
from completing deletion, so its name continues to exist. A subsequent
gendisk creation may have the same cntlid which risks collision when
using that for the name. Use the unique ctrl->instance instead.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index f0716f6ce41f..2551264ef2b5 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -31,7 +31,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 		sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
 	} else if (ns->head->disk) {
 		sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
-				ctrl->cntlid, ns->head->instance);
+				ctrl->instance, ns->head->instance);
 		*flags = GENHD_FL_HIDDEN;
 	} else {
 		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
-- 
2.16.4

[PATCH 1/2] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Hannes Reinecke]

On 4/4/19 12:41 AM, Hannes Reinecke wrote:
> A process holding an open reference to a removed disk prevents it
> from completing deletion, so its name continues to exist. A subsequent
> gendisk creation may have the same cntlid which risks collision when
> using that for the name. Use the unique ctrl->instance instead.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   drivers/nvme/host/multipath.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index f0716f6ce41f..2551264ef2b5 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -31,7 +31,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
>   		sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
>   	} else if (ns->head->disk) {
>   		sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> -				ctrl->cntlid, ns->head->instance);
> +				ctrl->instance, ns->head->instance);
>   		*flags = GENHD_FL_HIDDEN;
>   	} else {
>   		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
> 
Ping?

What's the status of this patch?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH 1/2] nvme-multipath: avoid crash on invalid subsystem cntlid enumeration

[Christoph Hellwig]

On Fri, May 03, 2019@01:04:48PM +0200, Hannes Reinecke wrote:
> What's the status of this patch?

Waiting for you to resend the series based on comments on patch 2..

[PATCH 2/2] nvme: validate cntlid during controller initialisation

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.com>

The CNTLID value is required to be unique, and we do rely on this
for correct operation. So reject any controller for which a non-unique
CNTLID has been detected.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b5939112b9b6..23000a368e1f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2485,6 +2485,7 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl)
 int nvme_init_identify(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
+	struct nvme_ctrl *tmp;
 	u64 cap;
 	int ret, page_shift;
 	u32 max_hw_sectors;
@@ -2624,7 +2625,20 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		ctrl->hmmaxd = le16_to_cpu(id->hmmaxd);
 	}
 
-	ret = nvme_mpath_init(ctrl, id);
+	ret = 0;
+	mutex_lock(&ctrl->subsys->lock);
+	list_for_each_entry(tmp, &ctrl->subsys->ctrls, subsys_entry) {
+		if (tmp->cntlid == ctrl->cntlid) {
+			dev_err(ctrl->device, "Duplicate cntlid %u, rejecting\n",
+				ctrl->cntlid);
+			ret = -EINVAL;
+			break;
+		}
+	}
+	mutex_unlock(&ctrl->subsys->lock);
+
+	if (ret == 0)
+		ret = nvme_mpath_init(ctrl, id);
 	kfree(id);
 
 	if (ret < 0)
-- 
2.16.4

[PATCH 2/2] nvme: validate cntlid during controller initialisation

[Christoph Hellwig]

> +	ret = 0;
> +	mutex_lock(&ctrl->subsys->lock);
> +	list_for_each_entry(tmp, &ctrl->subsys->ctrls, subsys_entry) {
> +		if (tmp->cntlid == ctrl->cntlid) {
> +			dev_err(ctrl->device, "Duplicate cntlid %u, rejecting\n",
> +				ctrl->cntlid);
> +			ret = -EINVAL;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&ctrl->subsys->lock);

Please split the validation loop ino a separate helper.  Also I think
the check should go into nvme_init_subsystem, under the same critical
section as actualy adding the controller to the controller list to
avoid a small race window.