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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <62e78a2f-998a-b340-9dcd-7d5c12c96fe9@grimberg.me>
Date:   Thu, 1 Sep 2022 13:49:32 +0300
From:   Sagi Grimberg <sagi@...mberg.me>
To:     sungup.moon@...sung.com, "kbusch@...nel.org" <kbusch@...nel.org>,
        "axboe@...com" <axboe@...com>, "hch@....de" <hch@....de>
Cc:     "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drivers/nvme/host: Fix namespace duplication check
 rule



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@...sung.com>
> ---
>   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...