Re: [PATCH v2] drivers/nvme/host: Fix namespace duplication check rule

[Sagi Grimberg]

On 9/1/22 03:49, Sungup Moon wrote:
> Some NVMe device, use EUI64 and NGUID, has fixed value EUI64 on a
> sub-system because of the bit size of ID. Current kernel check the
> all IDs should have unique value in a sub-system and globally.
> However, if an namespace has duplicate IDs (not all) in a sub-system,
> current kernel raise "duplicate IDs in subsystem for nsid" error. But
> NVMe Specification defines the namespace unique identifier like this:
>
> When creating a namespace, the controller shall indicate a globally
> unique value in one or more of the following:
> a) the EUI64 field;
> b) the NGUID field; or
> c) a Namespace Identification Descriptor with the Namespace Identifier
> Type field set to 3h
> (reference: 7.11 Unique Identifier; NVM Express 1.4c spec)
>
> So, I suggest the modified nvme_subsys_check_duplicate_ids function
> checking uniqueness from all IDS to one more IDs.
>
> I missed the initializing of "duplicated" variable, so I reissue the
> version2 patch.
>
> Signed-off-by: Sungup Moon <sungup.moon@xxxxxxxxxxx>
> ---
>   drivers/nvme/host/core.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index af367b22871b..8c2faa2881a4 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3902,24 +3902,35 @@ static struct nvme_ns_head *nvme_find_ns_head(struct nvme_ctrl *ctrl,
>          return NULL;
>   }
>
> +#define IDS_EQUAL(A, B) (memcmp(&(A), &(B), sizeof(A)) == 0)

No need for this macro IMO.

> +
>   static int nvme_subsys_check_duplicate_ids(struct nvme_subsystem *subsys,
>                  struct nvme_ns_ids *ids)
>   {
>          bool has_uuid = !uuid_is_null(&ids->uuid);
>          bool has_nguid = memchr_inv(ids->nguid, 0, sizeof(ids->nguid));
>          bool has_eui64 = memchr_inv(ids->eui64, 0, sizeof(ids->eui64));
> +       bool duplicated;
>          struct nvme_ns_head *h;
>
>          lockdep_assert_held(&subsys->lock);
>
>          list_for_each_entry(h, &subsys->nsheads, entry) {
> -               if (has_uuid && uuid_equal(&ids->uuid, &h->ids.uuid))
> -                       return -EINVAL;
> -               if (has_nguid &&
> -                   memcmp(&ids->nguid, &h->ids.nguid, sizeof(ids->nguid)) == 0)
> -                       return -EINVAL;
> -               if (has_eui64 &&
> -                   memcmp(&ids->eui64, &h->ids.eui64, sizeof(ids->eui64)) == 0)
> +               duplicated = has_uuid || has_nguid || has_eui64;
> +
> +               if (has_uuid)
> +                       duplicated = duplicated &&
> +                               uuid_equal(&ids->uuid, &h->ids.uuid);
> +
> +               if (has_nguid)
> +                       duplicated = duplicated &&
> +                               IDS_EQUAL(ids->nguid, h->ids.nguid);
> +
> +               if (has_eui64)
> +                       duplicated = duplicated &&
> +                               IDS_EQUAL(ids->eui64, h->ids.eui64);
> +
> +               if (duplicated)
>                          return -EINVAL;

That is very confusing.

So a ns can have an identifier that does not have to be unique? or that
every identifier that exist should be unique but at least one identifier
should exist?

The former seems weird...