| 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
| ||
|
Message-ID: <a3051d85-c08e-a273-c7cc-2eb3e21df717@grimberg.me>
Date: Tue, 15 Mar 2022 10:18:30 +0200
From: Sagi Grimberg <sagi@...mberg.me>
To: "hch@....de" <hch@....de>, Sungup Moon <sungup.moon@...sung.com>
Cc: "kbusch@...nel.org" <kbusch@...nel.org>,
"axboe@...com" <axboe@...com>,
"linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] driver/nvme/host: Support duplicated nsid for the
private ns
On 3/15/22 09:19, hch@....de wrote:
> On Tue, Mar 15, 2022 at 08:12:30AM +0100, hch@....de wrote:
>> I looked at this a bit more and found two issues:
>>
>> - nvme_init_ns_head will now leak the ns_head for the private namespaces
>> with potentially duplicate IDs case.
>> - nvme_mpath_set_disk_name still needs to use the subsystem-wide IDA
>> for the nvme instance name as the subsystem and controller ones
>> could otherwise clash.
>>
>> Let me know what you think of this version:
>
> Except that this had the parts to actually make it compile uncommited,
> so here is the proper one:
>
> ---
> From c6deed0b18d66460b090d22ee18f37d631d0fd12 Mon Sep 17 00:00:00 2001
> From: Sungup Moon <sungup.moon@...sung.com>
> Date: Mon, 14 Mar 2022 20:05:45 +0900
> Subject: nvme: allow duplicated NSIDs for the private namespaces
>
> A NVMe subsystem with multiple controller can have private namespaces
> that use the same NSID under some conditions:
>
> "If Namespace Management, ANA Reporting, or NVM Sets are supported, the
> NSIDs shall be unique within the NVM subsystem. If the Namespace
> Management, ANA Reporting, and NVM Sets are not supported, then NSIDs:
> a) for shared namespace shall be unique; and
> b) for private namespace are not required to be unique."
>
> Reference: Section 6.1.6 NSID and Namespace Usage; NVM Express 1.4c spec.
>
> Make sure this specific setup is supported in Linux.
>
> Signed-off-by: Sungup Moon <sungup.moon@...sung.com>
> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
> drivers/nvme/host/core.c | 12 +++++++++---
> drivers/nvme/host/multipath.c | 7 ++++---
> drivers/nvme/host/nvme.h | 19 +++++++++++++++++++
> include/linux/nvme.h | 1 +
> 4 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f8084ded69e50..31f7a479fa08d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3649,15 +3649,21 @@ static const struct attribute_group *nvme_dev_attr_groups[] = {
> NULL,
> };
>
> -static struct nvme_ns_head *nvme_find_ns_head(struct nvme_subsystem *subsys,
> +static struct nvme_ns_head *nvme_find_ns_head(struct nvme_ctrl *ctrl,
> unsigned nsid)
> {
> + struct nvme_subsystem *subsys = ctrl->subsys;
> struct nvme_ns_head *h;
>
> lockdep_assert_held(&subsys->lock);
IMO it is a bit strange that we now don't pass in the subsystem but
require that the subsys->lock is taken...
>
> list_for_each_entry(h, &subsys->nsheads, entry) {
> - if (h->ns_id != nsid)
> + /*
> + * Private namespaces can share NSIDs under some conditions.
> + * In that case we can't use the same ns_head for namespaces
> + * with the same NSID.
> + */
> + if (h->ns_id != nsid || !nvme_is_uniqueue_nsid(ctrl, h))
> continue;
> if (!list_empty(&h->list) && nvme_tryget_ns_head(h))
> return h;
> @@ -3851,7 +3857,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
> }
>
> mutex_lock(&ctrl->subsys->lock);
> - head = nvme_find_ns_head(ctrl->subsys, nsid);
> + head = nvme_find_ns_head(ctrl, nsid);
> if (!head) {
> ret = nvme_subsys_check_duplicate_ids(ctrl->subsys, ids);
> if (ret) {
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index d13b81cd6225c..6b6df1016cb91 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -504,10 +504,11 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
>
> /*
> * Add a multipath node if the subsystems supports multiple controllers.
> - * We also do this for private namespaces as the namespace sharing data could
> - * change after a rescan.
> + * We also do this for private namespaces as the namespace sharing flag
> + * could change after a rescan.
What happens in this case? we now have non-unique shared namespaces?
> */
> - if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) || !multipath)
> + if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) ||
> + !nvme_is_uniqueue_nsid(ctrl, head) || !multipath)
> return 0;
>
> head->disk = blk_alloc_disk(ctrl->numa_node);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 587d92df118b7..9add586434929 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -718,6 +718,25 @@ static inline bool nvme_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
> return queue_live;
> return __nvme_check_ready(ctrl, rq, queue_live);
> }
> +
> +/*
> + * NSID shall be unique for all shared namespaces, or if at least one of the
> + * following conditions is met:
> + * 1. Namespace Management is supported by the controller
> + * 2. ANA is supported by the controller
> + * 3. NVM Set are supported by the controller
> + *
> + * In other case, private namespace are not required to report a unique NSID.
> + */
> +static inline bool nvme_is_uniqueue_nsid(struct nvme_ctrl *ctrl,
> + struct nvme_ns_head *head)
> +{
> + return head->shared ||
> + (ctrl->oacs & NVME_CTRL_OACS_NS_MNGT_SUPP) ||
> + (ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA) ||
> + (ctrl->ctratt & NVME_CTRL_CTRATT_NVM_SETS);
> +}
> +
> int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> void *buf, unsigned bufflen);
> int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 9dbc3ef4daf7c..2dcee34d467d6 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -345,6 +345,7 @@ enum {
> NVME_CTRL_ONCS_TIMESTAMP = 1 << 6,
> NVME_CTRL_VWC_PRESENT = 1 << 0,
> NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
> + NVME_CTRL_OACS_NS_MNGT_SUPP = 1 << 3,
> NVME_CTRL_OACS_DIRECTIVES = 1 << 5,
> NVME_CTRL_OACS_DBBUF_SUPP = 1 << 8,
> NVME_CTRL_LPA_CMD_EFFECTS_LOG = 1 << 1,
Powered by blists - more mailing lists