[PATCHv3 0/2] nvme: validate CNTLID

[PATCHv3 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.

Changes to v1:
- split cntlid validation into a separate helper and moved to
  nvme_init_subsystem()

Changes to v2:
- move nvme_validate_cntlid() into critical section when adding
  nvme controller to the list as suggested by hch

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

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

-- 
2.16.4

[PATCHv3 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 5c9429d41120..499acf07d61a 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

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

[Keith Busch]

On Fri, May 03, 2019@03:37:35PM +0200, 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>

Looks good

Reviewed-by: Keith Busch <keith.busch at intel.com>

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

[Christoph Hellwig]

Thanks,

applied to nvme-5.2.

[PATCHv3 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 | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cd16d98d1f1a..dc74f7ba6f4a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2358,6 +2358,23 @@ static int nvme_active_ctrls(struct nvme_subsystem *subsys)
 	return count;
 }
 
+static bool nvme_duplicate_cntlid(struct nvme_subsystem *subsys,
+				  struct nvme_ctrl *ctrl)
+{
+	struct nvme_ctrl *tmp;
+	bool ret = false;
+
+	list_for_each_entry(tmp, &subsys->ctrls, subsys_entry) {
+		if (tmp == ctrl)
+			continue;
+		if (tmp->cntlid == ctrl->cntlid) {
+			ret = true;
+			break;
+		}
+	}
+	return ret;
+}
+
 static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
 	struct nvme_subsystem *subsys, *found;
@@ -2411,6 +2428,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 
 		__nvme_release_subsystem(subsys);
 		subsys = found;
+		ret = 0;
 	} else {
 		ret = device_add(&subsys->dev);
 		if (ret) {
@@ -2434,10 +2452,20 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	}
 
 	mutex_lock(&subsys->lock);
-	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
+	if (!nvme_duplicate_cntlid(subsys, ctrl))
+		list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
+	else {
+		dev_err(ctrl->device,
+			"Duplicate cntlid %u, rejecting\n",
+			ctrl->cntlid);
+		ctrl->subsys = NULL;
+		sysfs_remove_link(&subsys->dev.kobj, dev_name(ctrl->device));
+		nvme_put_subsystem(subsys);
+		ret = -EINVAL;
+	}
 	mutex_unlock(&subsys->lock);
 
-	return 0;
+	return ret;
 
 out_unlock:
 	mutex_unlock(&nvme_subsystems_lock);
-- 
2.16.4

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

[Keith Busch]

On Fri, May 03, 2019@03:37:36PM +0200, Hannes Reinecke wrote:
> 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.

Looks good. 

Reviewed-by: Keith Busch <keith.busch at intel.com>

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

[Max Gurtovoy]

On 5/3/2019 4:37 PM, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare at 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 | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cd16d98d1f1a..dc74f7ba6f4a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2358,6 +2358,23 @@ static int nvme_active_ctrls(struct nvme_subsystem *subsys)
>   	return count;
>   }
>   
> +static bool nvme_duplicate_cntlid(struct nvme_subsystem *subsys,
> +				  struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_ctrl *tmp;
> +	bool ret = false;
> +
> +	list_for_each_entry(tmp, &subsys->ctrls, subsys_entry) {
> +		if (tmp == ctrl)
> +			continue;
> +		if (tmp->cntlid == ctrl->cntlid) {
> +			ret = true;
> +			break;

can be done in 1 liner (without local variable ret):

if (tmp != ctrl && tmp->cntlid == ctrl->cntlid)

 ??? return true;


return false in the end of the function.


Otherwise,

looks good.

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>


> +		}
> +	}
> +	return ret;
> +}
> +
>   static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   {
>   	struct nvme_subsystem *subsys, *found;
> @@ -2411,6 +2428,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   
>   		__nvme_release_subsystem(subsys);
>   		subsys = found;
> +		ret = 0;
>   	} else {
>   		ret = device_add(&subsys->dev);
>   		if (ret) {
> @@ -2434,10 +2452,20 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	}
>   
>   	mutex_lock(&subsys->lock);
> -	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	if (!nvme_duplicate_cntlid(subsys, ctrl))
> +		list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	else {
> +		dev_err(ctrl->device,
> +			"Duplicate cntlid %u, rejecting\n",
> +			ctrl->cntlid);
> +		ctrl->subsys = NULL;
> +		sysfs_remove_link(&subsys->dev.kobj, dev_name(ctrl->device));
> +		nvme_put_subsystem(subsys);
> +		ret = -EINVAL;
> +	}
>   	mutex_unlock(&subsys->lock);
>   
> -	return 0;
> +	return ret;
>   
>   out_unlock:
>   	mutex_unlock(&nvme_subsystems_lock);

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

[Minwoo Im]

Hi Hannes,

I think this patch looks good. but I have a simple query here.

On 5/3/19 10:37 PM, Hannes Reinecke wrote:
> @@ -2434,10 +2452,20 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	}
>   
>   	mutex_lock(&subsys->lock);
> -	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	if (!nvme_duplicate_cntlid(subsys, ctrl))
> +		list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	else {
> +		dev_err(ctrl->device,
> +			"Duplicate cntlid %u, rejecting\n",
> +			ctrl->cntlid);
> +		ctrl->subsys = NULL;
> +		sysfs_remove_link(&subsys->dev.kobj, dev_name(ctrl->device));
> +		nvme_put_subsystem(subsys);

Does it(nvme_put_subsystem()) really need to be here?  I think explicit
"put" for the subsystem is always good, but right above this code, we
can see the comment when sysfs link has been failed:

if (sysfs_create_link(&subsys->dev.kobj, &ctrl->device->kobj,
                 dev_name(ctrl->device))) {
         dev_err(ctrl->device,
                 "failed to create sysfs link from subsystem.\n");
         /* the transport driver will eventually put the subsystem */
         return -EINVAL;
}

I'm not pretty sure where the exactly the comment says, but I can see
the nvme_destroy_subsystem() would be invoked from the transport
drivers (e.g. pci, rdma, etc)

In case of nvme-pci, nvme_remove_dead_ctrl() will do the "put".  For
nvme-rdma, nvme_rdma_create_ctrl() will do the "put".

Did you do just explicit "put" for the subsystem OR does it really need
to be here with any other reason?

I'm just asking why there is a difference between code above it and this
patch.  If you don't mind, please let me know if I'm wrong here. :)

Thanks,

> +		ret = -EINVAL;
> +	}
>   	mutex_unlock(&subsys->lock);
>   
> -	return 0;
> +	return ret;
>   
>   out_unlock:
>   	mutex_unlock(&nvme_subsystems_lock);
> 

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

[Christoph Hellwig]

This does two a little pointless iterations over the cntl list.

I'll send out a quick untested series to show how this might be
done better.

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

[John Donnelly]

On 5/3/19 8:37 AM, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare at 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 | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cd16d98d1f1a..dc74f7ba6f4a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2358,6 +2358,23 @@ static int nvme_active_ctrls(struct nvme_subsystem *subsys)
>   	return count;
>   }
>   
> +static bool nvme_duplicate_cntlid(struct nvme_subsystem *subsys,
> +				  struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_ctrl *tmp;
> +	bool ret = false;
> +
> +	list_for_each_entry(tmp, &subsys->ctrls, subsys_entry) {
> +		if (tmp == ctrl)
> +			continue;
> +		if (tmp->cntlid == ctrl->cntlid) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +	return ret;
> +}
> +
>   static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   {
>   	struct nvme_subsystem *subsys, *found;
> @@ -2411,6 +2428,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   
>   		__nvme_release_subsystem(subsys);
>   		subsys = found;
> +		ret = 0;
>   	} else {
>   		ret = device_add(&subsys->dev);
>   		if (ret) {
> @@ -2434,10 +2452,20 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   	}
>   
>   	mutex_lock(&subsys->lock);
> -	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	if (!nvme_duplicate_cntlid(subsys, ctrl))
> +		list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	else {
> +		dev_err(ctrl->device,
> +			"Duplicate cntlid %u, rejecting\n",
> +			ctrl->cntlid);
> +		ctrl->subsys = NULL;
> +		sysfs_remove_link(&subsys->dev.kobj, dev_name(ctrl->device));
> +		nvme_put_subsystem(subsys);
> +		ret = -EINVAL;
> +	}
>   	mutex_unlock(&subsys->lock);
>   
> -	return 0;
> +	return ret;
>   
>   out_unlock:
>   	mutex_unlock(&nvme_subsystems_lock);
> 

Hello,

I am interested in testing this series when they are finalized. I 
believe I have a test scenario for it that was discovered using the 
recent nvme-cli.1.8.1 release.

-- 
Thank You,
John

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

[Chaitanya Kulkarni]

Thanks John for reply. If you are interested you can always add tests to 
blktests framework here is link of the project :-

https://github.com/osandov/blktests.

On 05/10/2019 07:52 AM, John Donnelly wrote:
> On 5/3/19 8:37 AM, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare at 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 | 32 ++++++++++++++++++++++++++++++--
>>    1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index cd16d98d1f1a..dc74f7ba6f4a 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2358,6 +2358,23 @@ static int nvme_active_ctrls(struct nvme_subsystem *subsys)
>>    	return count;
>>    }
>>
>> +static bool nvme_duplicate_cntlid(struct nvme_subsystem *subsys,
>> +				  struct nvme_ctrl *ctrl)
>> +{
>> +	struct nvme_ctrl *tmp;
>> +	bool ret = false;
>> +
>> +	list_for_each_entry(tmp, &subsys->ctrls, subsys_entry) {
>> +		if (tmp == ctrl)
>> +			continue;
>> +		if (tmp->cntlid == ctrl->cntlid) {
>> +			ret = true;
>> +			break;
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +
>>    static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>    {
>>    	struct nvme_subsystem *subsys, *found;
>> @@ -2411,6 +2428,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>
>>    		__nvme_release_subsystem(subsys);
>>    		subsys = found;
>> +		ret = 0;
>>    	} else {
>>    		ret = device_add(&subsys->dev);
>>    		if (ret) {
>> @@ -2434,10 +2452,20 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>    	}
>>
>>    	mutex_lock(&subsys->lock);
>> -	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
>> +	if (!nvme_duplicate_cntlid(subsys, ctrl))
>> +		list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
>> +	else {
>> +		dev_err(ctrl->device,
>> +			"Duplicate cntlid %u, rejecting\n",
>> +			ctrl->cntlid);
>> +		ctrl->subsys = NULL;
>> +		sysfs_remove_link(&subsys->dev.kobj, dev_name(ctrl->device));
>> +		nvme_put_subsystem(subsys);
>> +		ret = -EINVAL;
>> +	}
>>    	mutex_unlock(&subsys->lock);
>>
>> -	return 0;
>> +	return ret;
>>
>>    out_unlock:
>>    	mutex_unlock(&nvme_subsystems_lock);
>>
>
> Hello,
>
> I am interested in testing this series when they are finalized. I
> believe I have a test scenario for it that was discovered using the
> recent nvme-cli.1.8.1 release.
>