properly validate the nvme uniqueue identifiers are unique

properly validate the nvme uniqueue identifiers are unique

[Christoph Hellwig]

Hi all,

this series fixed how the nvme driver check for unique identifier
and then also applied that logic across subsystems.

[PATCH 1/3] nvme: cleanup __nvme_check_ids

[Christoph Hellwig]

Pass the actual nvme_ns_ids used for the comparism instead of the
ns_head that isn't needed and use a more descriptive function name.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9cffc4770e737..076a03b801b7e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3673,16 +3673,15 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_subsystem *subsys,
 	return NULL;
 }
 
-static int __nvme_check_ids(struct nvme_subsystem *subsys,
-		struct nvme_ns_head *new)
+static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
+		struct nvme_ns_ids *ids)
 {
 	struct nvme_ns_head *h;
 
 	lockdep_assert_held(&subsys->lock);
 
 	list_for_each_entry(h, &subsys->nsheads, entry) {
-		if (nvme_ns_ids_valid(&new->ids) &&
-		    nvme_ns_ids_equal(&new->ids, &h->ids))
+		if (nvme_ns_ids_valid(ids) && nvme_ns_ids_equal(ids, &h->ids))
 			return -EINVAL;
 	}
 
@@ -3781,7 +3780,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	head->ids = *ids;
 	kref_init(&head->ref);
 
-	ret = __nvme_check_ids(ctrl->subsys, head);
+	ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &head->ids);
 	if (ret) {
 		dev_err(ctrl->device,
 			"duplicate IDs for nsid %d\n", nsid);
-- 
2.30.2

Re: [PATCH 1/3] nvme: cleanup __nvme_check_ids

[Kanchan Joshi]

On Thu, Feb 24, 2022 at 4:33 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Pass the actual nvme_ns_ids used for the comparism instead of the

s/comparism/comparison

> ns_head that isn't needed and use a more descriptive function name.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9cffc4770e737..076a03b801b7e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3673,16 +3673,15 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_subsystem *subsys,
>         return NULL;
>  }
>
> -static int __nvme_check_ids(struct nvme_subsystem *subsys,
> -               struct nvme_ns_head *new)
> +static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
> +               struct nvme_ns_ids *ids)
>  {
>         struct nvme_ns_head *h;
>
>         lockdep_assert_held(&subsys->lock);
>
>         list_for_each_entry(h, &subsys->nsheads, entry) {
> -               if (nvme_ns_ids_valid(&new->ids) &&
> -                   nvme_ns_ids_equal(&new->ids, &h->ids))
> +               if (nvme_ns_ids_valid(ids) && nvme_ns_ids_equal(ids, &h->ids))

Why the first check is inside the loop? Seems we can move this outside
of the loop.

Re: [PATCH 1/3] nvme: cleanup __nvme_check_ids

[Keith Busch]

On Thu, Feb 24, 2022 at 11:58:50AM +0100, Christoph Hellwig wrote:
> Pass the actual nvme_ns_ids used for the comparism instead of the
> ns_head that isn't needed and use a more descriptive function name.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9cffc4770e737..076a03b801b7e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3673,16 +3673,15 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_subsystem *subsys,
>  	return NULL;
>  }
>  
> -static int __nvme_check_ids(struct nvme_subsystem *subsys,
> -		struct nvme_ns_head *new)
> +static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
> +		struct nvme_ns_ids *ids)
>  {
>  	struct nvme_ns_head *h;
>  
>  	lockdep_assert_held(&subsys->lock);
>  
>  	list_for_each_entry(h, &subsys->nsheads, entry) {
> -		if (nvme_ns_ids_valid(&new->ids) &&
> -		    nvme_ns_ids_equal(&new->ids, &h->ids))
> +		if (nvme_ns_ids_valid(ids) && nvme_ns_ids_equal(ids, &h->ids))
>  			return -EINVAL;
>  	}
>  
> @@ -3781,7 +3780,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>  	head->ids = *ids;
>  	kref_init(&head->ref);
>  
> -	ret = __nvme_check_ids(ctrl->subsys, head);
> +	ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &head->ids);

Looks good.

As an afterthought, now that we don't need the "head", a further cleanup
can move this check further up in this function prior allocating the
head. That simplifies the error handling since there's nothing to
unwind.

[PATCH 2/3] nvme: fix the IDs equality check

[Christoph Hellwig]

In nvme_subsys_check_duplicate_ids we care if any of the IDs is the
same, not just if all of them match.  This also requires not comparing
the CSI, which is rather irrelevant here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 076a03b801b7e..ac4749f257439 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1716,13 +1716,6 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
 		blk_queue_max_write_zeroes_sectors(queue, UINT_MAX);
 }
 
-static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
-{
-	return !uuid_is_null(&ids->uuid) ||
-		memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) ||
-		memchr_inv(ids->eui64, 0, sizeof(ids->eui64));
-}
-
 static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
 {
 	return uuid_equal(&a->uuid, &b->uuid) &&
@@ -3681,7 +3674,14 @@ 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 (nvme_ns_ids_valid(ids) && nvme_ns_ids_equal(ids, &h->ids))
+		if (!uuid_is_null(&ids->uuid) &&
+		    uuid_equal(&ids->uuid, &h->ids.uuid))
+			return -EINVAL;
+		if (memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) &&
+		    memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0)
+			return -EINVAL;
+		if (memchr_inv(ids->eui64, 0, sizeof(ids->eui64)) &&
+		    memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0)
 			return -EINVAL;
 	}
 
-- 
2.30.2

Re: [PATCH 2/3] nvme: fix the IDs equality check

[Kanchan Joshi]

On Thu, Feb 24, 2022 at 4:33 PM Christoph Hellwig <hch@lst.de> wrote:
>
> In nvme_subsys_check_duplicate_ids we care if any of the IDs is the
> same, not just if all of them match.  This also requires not comparing
> the CSI, which is rather irrelevant here.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 076a03b801b7e..ac4749f257439 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1716,13 +1716,6 @@ static void nvme_config_discard(struct gendisk *disk, struct nvme_ns *ns)
>                 blk_queue_max_write_zeroes_sectors(queue, UINT_MAX);
>  }
>
> -static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
> -{
> -       return !uuid_is_null(&ids->uuid) ||
> -               memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) ||
> -               memchr_inv(ids->eui64, 0, sizeof(ids->eui64));
> -}
> -
>  static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
>  {
>         return uuid_equal(&a->uuid, &b->uuid) &&
> @@ -3681,7 +3674,14 @@ 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 (nvme_ns_ids_valid(ids) && nvme_ns_ids_equal(ids, &h->ids))
> +               if (!uuid_is_null(&ids->uuid) &&
> +                   uuid_equal(&ids->uuid, &h->ids.uuid))
> +                       return -EINVAL;
> +               if (memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) &&
> +                   memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0)
> +                       return -EINVAL;
> +               if (memchr_inv(ids->eui64, 0, sizeof(ids->eui64)) &&
> +                   memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0)

All the "ids" only checks can be consolidated out, and the loop can
keep only comparisons involving "h->ids".

[PATCH 3/3] nvme: check that EUI/GUID/UUID are globally unique

[Christoph Hellwig]

Add a check to verify that the unique identifiers are unique globally
in addition to the existing check that verifies that they are unique
inside a single subsystem.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 ac4749f257439..4f9623d9f7ed9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3783,7 +3783,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &head->ids);
 	if (ret) {
 		dev_err(ctrl->device,
-			"duplicate IDs for nsid %d\n", nsid);
+			"duplicate IDs in subsystem for nsid %d\n", nsid);
 		goto out_cleanup_srcu;
 	}
 
@@ -3815,12 +3815,40 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	return ERR_PTR(ret);
 }
 
+static int nvme_global_check_ids(struct nvme_subsystem *this,
+		struct nvme_ns_ids *ids)
+{
+	struct nvme_subsystem *s;
+	int ret = 0;
+
+	mutex_lock(&nvme_subsystems_lock);
+	list_for_each_entry(s, &nvme_subsystems, entry) {
+		if (s == this)
+			continue;
+		mutex_lock(&s->lock);
+		ret = nvme_subsys_check_duplicate_ids(s, ids);
+		mutex_unlock(&s->lock);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&nvme_subsystems_lock);
+
+	return ret;
+}
+
 static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 		struct nvme_ns_ids *ids, bool is_shared)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	struct nvme_ns_head *head = NULL;
-	int ret = 0;
+	int ret;
+
+	ret = nvme_global_check_ids(ctrl->subsys, ids);
+	if (ret) {
+		dev_err(ctrl->device,
+			"globally duplicate IDs for nsid %d\n", nsid);
+		return ret;
+	}
 
 	mutex_lock(&ctrl->subsys->lock);
 	head = nvme_find_ns_head(ctrl->subsys, nsid);
-- 
2.30.2

Re: [PATCH 3/3] nvme: check that EUI/GUID/UUID are globally unique

[Keith Busch]

On Thu, Feb 24, 2022 at 11:58:52AM +0100, Christoph Hellwig wrote:
> Add a check to verify that the unique identifiers are unique globally
> in addition to the existing check that verifies that they are unique
> inside a single subsystem.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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 ac4749f257439..4f9623d9f7ed9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3783,7 +3783,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>  	ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, &head->ids);
>  	if (ret) {
>  		dev_err(ctrl->device,
> -			"duplicate IDs for nsid %d\n", nsid);
> +			"duplicate IDs in subsystem for nsid %d\n", nsid);
>  		goto out_cleanup_srcu;
>  	}
>  
> @@ -3815,12 +3815,40 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>  	return ERR_PTR(ret);
>  }
>  
> +static int nvme_global_check_ids(struct nvme_subsystem *this,
> +		struct nvme_ns_ids *ids)
> +{
> +	struct nvme_subsystem *s;
> +	int ret = 0;
> +
> +	mutex_lock(&nvme_subsystems_lock);
> +	list_for_each_entry(s, &nvme_subsystems, entry) {
> +		if (s == this)
> +			continue;
> +		mutex_lock(&s->lock);
> +		ret = nvme_subsys_check_duplicate_ids(s, ids);
> +		mutex_unlock(&s->lock);
> +		if (ret)
> +			break;
> +	}
> +	mutex_unlock(&nvme_subsystems_lock);
> +
> +	return ret;
> +}
> +
>  static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
>  		struct nvme_ns_ids *ids, bool is_shared)
>  {
>  	struct nvme_ctrl *ctrl = ns->ctrl;
>  	struct nvme_ns_head *head = NULL;
> -	int ret = 0;
> +	int ret;
> +
> +	ret = nvme_global_check_ids(ctrl->subsys, ids);
> +	if (ret) {
> +		dev_err(ctrl->device,
> +			"globally duplicate IDs for nsid %d\n", nsid);
> +		return ret;
> +	}

It looks like a race condition exists here such that two namespaces with
duplicate IDs in different subsystems being concurrently added may not
see the duplicate.

Re: [PATCH 3/3] nvme: check that EUI/GUID/UUID are globally unique

[Christoph Hellwig]

On Thu, Feb 24, 2022 at 07:43:04AM -0800, Keith Busch wrote:
> It looks like a race condition exists here such that two namespaces with
> duplicate IDs in different subsystems being concurrently added may not
> see the duplicate.

Yes.  But do we care?  This is a sanity check.  The alternative would be
to hold nvme_subsystems_lock for the whole ns_head probing, which is
a little annoying.

Re: [PATCH 3/3] nvme: check that EUI/GUID/UUID are globally unique

[Keith Busch]

On Thu, Feb 24, 2022 at 05:51:38PM +0100, Christoph Hellwig wrote:
> On Thu, Feb 24, 2022 at 07:43:04AM -0800, Keith Busch wrote:
> > It looks like a race condition exists here such that two namespaces with
> > duplicate IDs in different subsystems being concurrently added may not
> > see the duplicate.
> 
> Yes.  But do we care?  This is a sanity check.  The alternative would be
> to hold nvme_subsystems_lock for the whole ns_head probing, which is
> a little annoying.

I suppose whether we care or not depends on how many broken or dispersed
subsystems exist in the field. If it's too common, we may receive bug
reports for inconsistent namespace enumeration.

Holding the subsystem lock for the entire scan is annoying, though... I
guess just go with this patch and we'll see what happens.