[PATCH] nvme: fix to find live controller based on namespace

[PATCH] nvme: fix to find live controller based on namespace

[Minwoo Im]

If application issues admin command to the namespace head block device,
it will try to find out live controller instance inside of the given
subsystem without considering namespace attachment.

For example, Let's say we have ns1 and ns2 in the nvme-subsys subsystem.
Both are attached to the controller nvme0, and nvme1 controller has
ns2 attached in multipath case:

  nvme-subsys (instance=0)
    nvme0
     nvme0n1
    nvme1
     nvme0n1 nvme0n2

if we issue an admin command to nvme0n2,

  nvme id-ns /dev/nvme0n2

It might return all-zero as spec because nvme_find_get_live_ctrl() will
find out the controller instance regardless to namespace attachment,
which means it will find out the nvme0 instance first.  Then command
will be issued to nvme0 with nsid=2 which is detached.

In this example, the following two commands should have same result:

  nvme id-ns /dev/nvme0n2
  nvme id-ns /dev/nvme1 -n 2

But, the first one is heading to nvme0 controller, not nvme1.

This patch fixed nvme_find_get_live_ctrl() function by refactoring
nvme_find_get_ns() into two parts: find and get. The name of
nvme_find_get_live_ctrl() function may sound like it will return
live controller instance from a subsystem, but we don't have more
callers other than this use case, so keep the name and make it receive
`nsid` to consider namespace attachment.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 drivers/nvme/host/core.c  | 43 +++++++++++++++++++++++++--------------
 drivers/nvme/host/ioctl.c |  3 ++-
 drivers/nvme/host/nvme.h  |  6 +++++-
 3 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b905f91f14eb..98a4a1b19b13 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2005,8 +2005,27 @@ static const struct block_device_operations nvme_bdev_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
+static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)
+{
+	struct nvme_ns *ns, *ret = NULL;
+
+	down_read(&ctrl->namespaces_rwsem);
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (ns->head->ns_id > nsid)
+			break;
+
+		if (ns->head->ns_id == nsid) {
+			ret = ns;
+			break;
+		}
+	}
+	up_read(&ctrl->namespaces_rwsem);
+	return ret;
+}
+
 #ifdef CONFIG_NVME_MULTIPATH
-struct nvme_ctrl *nvme_find_get_live_ctrl(struct nvme_subsystem *subsys)
+struct nvme_ctrl *nvme_find_get_live_ctrl(struct nvme_subsystem *subsys,
+		unsigned nsid)
 {
 	struct nvme_ctrl *ctrl;
 	int ret;
@@ -2015,7 +2034,7 @@ struct nvme_ctrl *nvme_find_get_live_ctrl(struct nvme_subsystem *subsys)
 	if (ret)
 		return ERR_PTR(ret);
 	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
-		if (ctrl->state == NVME_CTRL_LIVE)
+		if (ctrl->state == NVME_CTRL_LIVE && nvme_find_ns(ctrl, nsid))
 			goto found;
 	}
 	mutex_unlock(&nvme_subsystems_lock);
@@ -3544,21 +3563,15 @@ static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
 
 struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
-	struct nvme_ns *ns, *ret = NULL;
+	struct nvme_ns *ns;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		if (ns->head->ns_id == nsid) {
-			if (!kref_get_unless_zero(&ns->kref))
-				continue;
-			ret = ns;
-			break;
-		}
-		if (ns->head->ns_id > nsid)
-			break;
+	ns = nvme_find_ns(ctrl, nsid);
+	if (ns) {
+		if (!kref_get_unless_zero(&ns->kref))
+			return NULL;
 	}
-	up_read(&ctrl->namespaces_rwsem);
-	return ret;
+	return ns;
+
 }
 EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
 
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 8e05d65c9e93..8a829200997b 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -361,7 +361,8 @@ int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 static int nvme_ns_head_ctrl_ioctl(struct nvme_ns_head *head,
 		unsigned int cmd, void __user *argp)
 {
-	struct nvme_ctrl *ctrl = nvme_find_get_live_ctrl(head->subsys);
+	struct nvme_ctrl *ctrl =
+		nvme_find_get_live_ctrl(head->subsys, head->ns_id);
 	int ret;
 
 	if (IS_ERR(ctrl))
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 49276186d5bd..15bd2000991a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -657,7 +657,11 @@ struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk,
 void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx);
 bool nvme_tryget_ns_head(struct nvme_ns_head *head);
 void nvme_put_ns_head(struct nvme_ns_head *head);
-struct nvme_ctrl *nvme_find_get_live_ctrl(struct nvme_subsystem *subsys);
+struct nvme_ctrl *nvme_find_get_live_ctrl(struct nvme_subsystem *subsys,
+		unsigned nsid);
+int nvme_cdev_add(struct cdev *cdev, struct device *cdev_device,
+		const struct file_operations *fops, struct module *owner);
+void nvme_cdev_del(struct cdev *cdev, struct device *cdev_device);
 int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		unsigned int cmd, unsigned long arg);
 int nvme_ns_head_ioctl(struct block_device *bdev, fmode_t mode,
-- 
2.27.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: fix to find live controller based on namespace

[Christoph Hellwig]

Urgg.  But yes, if people want to issue the Identify commands that
depend on an active namespace we can't do this.  But I think the simpler
fix is a manual revert of "nvme: don't bother to look up a namespace for
controller ioctls".

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme