Skip to contentSkip to conversation

namespace list locking and ioctl fixes V2

PATCH New Christoph Hellwig 3 participants 11 messages Dec 24, 2015 – Jan 4, 2016 NVMe

lore
Share & tools
lore

Christoph Hellwig

author maintainer
#0 lore raw

This series fixes locking for the namespace list in the nvme controller, and also sorts out two major issue with the ioctl interface:

(1) it fixes access to the namespace in the NVME_IOCTL_IO_CMD ioctl on the character device and disables it for the unsafe case of a device with multiple namepaces. It also warns about this usage in general.
(2) it makes the SG_IO emulation optional. There is no need to emulate SCSI commands for a Linux block device, and the current code actually is bigger than the rest of the NVMe core driver in source and binary form. Anyone who cares about build size or reduce attack surface should disable it. In fact I can't see any good reason to enabe it at all.

Changes since V1:
- rebased ontop of a code move patch from Sagi, which is now included as new first patch in the series.

author maintainer
#1 lore raw

PATCH 1/4 nvme: Move nvme_freeze/unfreeze_queues to nvme core

From: Sagi Grimberg <[email protected]>

Nothing pci specific about them and We'll need them exported in other transports too.

Signed-off-by Sagi Grimberg <sagig at mellanox.com>
Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 28 ++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  3 +++
 drivers/nvme/host/pci.c  | 32 ++------------------------------
 3 files changed, 33 insertions(+), 30 deletions(-)
drivers/nvme/host/core.c +28 -0
index 1437ff3..2713005 100644
@@ -1328,6 +1328,34 @@ out:
 	return ret;
 }
 
+void nvme_freeze_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		blk_mq_freeze_queue_start(ns->queue);
+
+		spin_lock_irq(ns->queue->queue_lock);
+		queue_flag_set(QUEUE_FLAG_STOPPED, ns->queue);
+		spin_unlock_irq(ns->queue->queue_lock);
+
+		blk_mq_cancel_requeue_work(ns->queue);
+		blk_mq_stop_hw_queues(ns->queue);
+	}
+}
+
+void nvme_unfreeze_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
+		blk_mq_unfreeze_queue(ns->queue);
+		blk_mq_start_stopped_hw_queues(ns->queue, true);
+		blk_mq_kick_requeue_list(ns->queue);
+	}
+}
+
 int __init nvme_core_init(void)
 {
 	int result;
drivers/nvme/host/nvme.h +3 -0
index d88cf45..0da6747 100644
@@ -237,6 +237,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl);
 void nvme_scan_namespaces(struct nvme_ctrl *ctrl);
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
+void nvme_freeze_queues(struct nvme_ctrl *ctrl);
+void nvme_unfreeze_queues(struct nvme_ctrl *ctrl);
+
 struct request *nvme_alloc_request(struct request_queue *q,
 		struct nvme_command *cmd, unsigned int flags);
 void nvme_requeue_req(struct request *req);
drivers/nvme/host/pci.c +2 -30
index b82bbea..a7e5499 100644
@@ -1903,34 +1903,6 @@ static void nvme_dev_list_remove(struct nvme_dev *dev)
 		kthread_stop(tmp);
 }
 
-static void nvme_freeze_queues(struct nvme_dev *dev)
-{
-	struct nvme_ns *ns;
-
-	list_for_each_entry(ns, &dev->ctrl.namespaces, list) {
-		blk_mq_freeze_queue_start(ns->queue);
-
-		spin_lock_irq(ns->queue->queue_lock);
-		queue_flag_set(QUEUE_FLAG_STOPPED, ns->queue);
-		spin_unlock_irq(ns->queue->queue_lock);
-
-		blk_mq_cancel_requeue_work(ns->queue);
-		blk_mq_stop_hw_queues(ns->queue);
-	}
-}
-
-static void nvme_unfreeze_queues(struct nvme_dev *dev)
-{
-	struct nvme_ns *ns;
-
-	list_for_each_entry(ns, &dev->ctrl.namespaces, list) {
-		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
-		blk_mq_unfreeze_queue(ns->queue);
-		blk_mq_start_stopped_hw_queues(ns->queue, true);
-		blk_mq_kick_requeue_list(ns->queue);
-	}
-}
-
 static void nvme_dev_shutdown(struct nvme_dev *dev)
 {
 	int i;

@@ -1940,7 +1912,7 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
 
 	mutex_lock(&dev->shutdown_lock);
 	if (dev->bar) {
-		nvme_freeze_queues(dev);
+		nvme_freeze_queues(&dev->ctrl);
 		csts = readl(dev->bar + NVME_REG_CSTS);
 	}
 	if (csts & NVME_CSTS_CFS || !(csts & NVME_CSTS_RDY)) {

@@ -2049,7 +2021,7 @@ static void nvme_reset_work(struct work_struct *work)
 		dev_warn(dev->dev, "IO queues not created\n");
 		nvme_remove_namespaces(&dev->ctrl);
 	} else {
-		nvme_unfreeze_queues(dev);
+		nvme_unfreeze_queues(&dev->ctrl);
 		nvme_dev_add(dev);
 	}
Signature · 1 line
1.9.1
#2 lore raw

PATCH 1/4 nvme: Move nvme_freeze/unfreeze_queues to nvme core

Quoted 7 lines · Christoph Hellwig

From: Sagi Grimberg <sagig at mellanox.com>

Nothing pci specific about them and We'll need them exported in other transports too.

Signed-off-by Sagi Grimberg <sagig at mellanox.com>
Signed-off-by Christoph Hellwig <hch at lst.de>
Acked-by Keith Busch <keith.busch at intel.com>
author maintainer
#3 lore raw

PATCH 2/4 nvme: synchronize access to ctrl->namespaces

In reply to Christoph Hellwig

Currently traversal and modification of ctrl->namespaces happens completely unsynchronized, which can be fixed by the addition of a simple mutex.

Note: nvme_dev_ioctl will be handled in the next patch.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Sagi Grimberg <sagig at mellanox.com>
---
 drivers/nvme/host/core.c | 17 +++++++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 18 insertions(+)
drivers/nvme/host/core.c +17 -0
index 2713005..a928ad5 100644
@@ -1034,6 +1034,8 @@ static struct nvme_ns *nvme_find_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns *ns;
 
+	lockdep_assert_held(&ctrl->namespaces_mutex);
+
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		if (ns->ns_id == nsid)
 			return ns;

@@ -1049,6 +1051,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	struct gendisk *disk;
 	int node = dev_to_node(ctrl->dev);
 
+	lockdep_assert_held(&ctrl->namespaces_mutex);
+
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)
 		return;

@@ -1118,6 +1122,8 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	bool kill = nvme_io_incapable(ns->ctrl) &&
 			!blk_queue_dying(ns->queue);
 
+	lockdep_assert_held(&ns->ctrl->namespaces_mutex);
+
 	if (kill)
 		blk_set_queue_dying(ns->queue);
 	if (ns->disk->flags & GENHD_FL_UP) {

@@ -1188,6 +1194,8 @@ static void __nvme_scan_namespaces(struct nvme_ctrl *ctrl, unsigned nn)
 	struct nvme_ns *ns, *next;
 	unsigned i;
 
+	lockdep_assert_held(&ctrl->namespaces_mutex);
+
 	for (i = 1; i <= nn; i++)
 		nvme_validate_ns(ctrl, i);
 

@@ -1205,6 +1213,7 @@ void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
 	if (nvme_identify_ctrl(ctrl, &id))
 		return;
 
+	mutex_lock(&ctrl->namespaces_mutex);
 	nn = le32_to_cpu(id->nn);
 	if (ctrl->vs >= NVME_VS(1, 1) &&
 	    !(ctrl->quirks & NVME_QUIRK_IDENTIFY_CNS)) {

@@ -1214,6 +1223,7 @@ void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
 	__nvme_scan_namespaces(ctrl, le32_to_cpup(&id->nn));
  done:
 	list_sort(NULL, &ctrl->namespaces, ns_cmp);
+	mutex_unlock(&ctrl->namespaces_mutex);
 	kfree(id);
 }
 

@@ -1221,8 +1231,10 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns, *next;
 
+	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
 		nvme_ns_remove(ns);
+	mutex_unlock(&ctrl->namespaces_mutex);
 }
 
 static DEFINE_IDA(nvme_instance_ida);

@@ -1290,6 +1302,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	int ret;
 
 	INIT_LIST_HEAD(&ctrl->namespaces);
+	mutex_init(&ctrl->namespaces_mutex);
 	kref_init(&ctrl->kref);
 	ctrl->dev = dev;
 	ctrl->ops = ops;

@@ -1332,6 +1345,7 @@ void nvme_freeze_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
 
+	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		blk_mq_freeze_queue_start(ns->queue);
 

@@ -1342,18 +1356,21 @@ void nvme_freeze_queues(struct nvme_ctrl *ctrl)
 		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
 	}
+	mutex_unlock(&ctrl->namespaces_mutex);
 }
 
 void nvme_unfreeze_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
 
+	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
 		blk_mq_unfreeze_queue(ns->queue);
 		blk_mq_start_stopped_hw_queues(ns->queue, true);
 		blk_mq_kick_requeue_list(ns->queue);
 	}
+	mutex_unlock(&ctrl->namespaces_mutex);
 }
 
 int __init nvme_core_init(void)
drivers/nvme/host/nvme.h +1 -0
index 0da6747..4437592 100644
@@ -69,6 +69,7 @@ struct nvme_ctrl {
 	int instance;
 	struct blk_mq_tag_set *tagset;
 	struct list_head namespaces;
+	struct mutex namespaces_mutex;
 	struct device *device;	/* char device */
 	struct list_head node;
Signature · 1 line
1.9.1
#4 lore raw

PATCH 2/4 nvme: synchronize access to ctrl->namespaces

Quoted 7 lines · Christoph Hellwig

Currently traversal and modification of ctrl->namespaces happens completely unsynchronized, which can be fixed by the addition of a simple mutex.

Note: nvme_dev_ioctl will be handled in the next patch.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Sagi Grimberg <sagig at mellanox.com>

Looks good.

Acked-by Keith Busch <keith.busch at intel.com>
author maintainer
#5 lore raw

PATCH 3/4 nvme: fixes for NVME_IOCTL_IO_CMD on the char device

In reply to Christoph Hellwig

Make sure we synchronize access to the namespaces list and grab a reference to the namespace before doing I/O. Make sure to reject the ioctl if multiple namespaces are present as it's entirely unsafe, and warn when using it even with a single namespace.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Sagi Grimberg <sagig at mellanox.com>
---
 drivers/nvme/host/core.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)
drivers/nvme/host/core.c +34 -5
index a928ad5..51f6fc8 100644
@@ -922,21 +922,50 @@ static int nvme_dev_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp)
+{
+	struct nvme_ns *ns;
+	int ret;
+
+	mutex_lock(&ctrl->namespaces_mutex);
+	if (list_empty(&ctrl->namespaces)) {
+		ret = -ENOTTY;
+		goto out_unlock;
+	}
+
+	ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
+	if (ns != list_last_entry(&ctrl->namespaces, struct nvme_ns, list)) {
+		dev_warn(ctrl->dev,
+			"NVME_IOCTL_IO_CMD not supported when multiple namespaces present!\n");
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	dev_warn(ctrl->dev,
+		"using deprecated NVME_IOCTL_IO_CMD ioctl on the char device!\n");
+	kref_get(&ns->kref);
+	mutex_unlock(&ctrl->namespaces_mutex);
+
+	ret = nvme_user_cmd(ctrl, ns, argp);
+	nvme_put_ns(ns);
+	return ret;
+
+out_unlock:
+	mutex_unlock(&ctrl->namespaces_mutex);
+	return ret;
+}
+
 static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 		unsigned long arg)
 {
 	struct nvme_ctrl *ctrl = file->private_data;
 	void __user *argp = (void __user *)arg;
-	struct nvme_ns *ns;
 
 	switch (cmd) {
 	case NVME_IOCTL_ADMIN_CMD:
 		return nvme_user_cmd(ctrl, NULL, argp);
 	case NVME_IOCTL_IO_CMD:
-		if (list_empty(&ctrl->namespaces))
-			return -ENOTTY;
-		ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
-		return nvme_user_cmd(ctrl, ns, argp);
+		return nvme_dev_user_cmd(ctrl, argp);
 	case NVME_IOCTL_RESET:
 		dev_warn(ctrl->dev, "resetting controller\n");
 		return ctrl->ops->reset_ctrl(ctrl);
Signature · 1 line
1.9.1
#6 lore raw

PATCH 3/4 nvme: fixes for NVME_IOCTL_IO_CMD on the char device

Quoted 7 lines · Christoph Hellwig

Make sure we synchronize access to the namespaces list and grab a reference to the namespace before doing I/O. Make sure to reject the ioctl if multiple namespaces are present as it's entirely unsafe, and warn when using it even with a single namespace.

Signed-off-by Christoph Hellwig <hch at lst.de>
Reviewed-by Sagi Grimberg <sagig at mellanox.com>

Okay, I accept the argument for allowing these to "hidden" namespaces is not sufficiently justified.

Acked-by Keith Busch <keith.busch at intel.com>
author maintainer
#7 lore raw

PATCH 4/4 nvme: make SG_IO support optional

In reply to Christoph Hellwig

Translation SCSI commands to NVMe commands is rather pointless in general as applications must not expext to be able to use SCSI commands on a generic block device.

Make the huge translation layer optional and hope no one will ever enable it in the future.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/Kconfig  | 11 +++++++++++
 drivers/nvme/host/Makefile |  3 ++-
 drivers/nvme/host/core.c   |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)
drivers/nvme/host/Kconfig +11 -0
index 002a94a..5d62373 100644
@@ -8,3 +8,14 @@ config BLK_DEV_NVME
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called nvme.
+
+config BLK_DEV_NVME_SCSI
+	bool "SCSI emulation for NVMe device nodes"
+	depends on BLK_DEV_NVME
+	---help---
+	  This adds support for the SG_IO ioctl on the NVMe character
+	  and block devices nodes, as well a a translation for a small
+	  number of selected SCSI commands to NVMe commands to the NVMe
+	  driver.  If you don't know what this means you probably want
+	  to say N here, and if you know what it means you probably
+	  want to say N as well.
drivers/nvme/host/Makefile +2 -1
index 3e26dc9..baf9f52 100644
@@ -1,4 +1,5 @@
 
 obj-$(CONFIG_BLK_DEV_NVME)     += nvme.o
 
-nvme-y		+= core.o pci.o scsi.o lightnvm.o
+nvme-y					+= core.o pci.o lightnvm.o
+nvme-$(CONFIG_BLK_DEV_NVME_SCSI)        += scsi.o
drivers/nvme/host/core.c +2 -0
index 51f6fc8..8da4a8a 100644
@@ -467,10 +467,12 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		return nvme_user_cmd(ns->ctrl, ns, (void __user *)arg);
 	case NVME_IOCTL_SUBMIT_IO:
 		return nvme_submit_io(ns, (void __user *)arg);
+#ifdef CONFIG_BLK_DEV_NVME_SCSI
 	case SG_GET_VERSION_NUM:
 		return nvme_sg_get_version_num((void __user *)arg);
 	case SG_IO:
 		return nvme_sg_io(ns, (void __user *)arg);
+#endif
 	default:
 		return -ENOTTY;
 	}
Signature · 1 line
1.9.1
#8 lore raw

PATCH 4/4 nvme: make SG_IO support optional

To complete the set, looks fine,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
#9 lore raw

PATCH 4/4 nvme: make SG_IO support optional

In reply to Christoph Hellwig

drivers/nvme/host/Kconfig config BLK_DEV_NVME · line 14

Quoted 3 lines · Christoph Hellwig
drivers/nvme/host/Kconfig +3 -0
+config BLK_DEV_NVME_SCSI
+	bool "SCSI emulation for NVMe device nodes"
+	depends on BLK_DEV_NVME

Can we make this "default y"? I strongly advocate for native NVMe over SCSI translations too, but I know of users relying on this. It's been built in for years and not aware of it breaking anything, so think it should be opt-out.

author maintainer
#10 lore raw

PATCH 4/4 nvme: make SG_IO support optional

Quoted 9 lines · Keith Busch

drivers/nvme/host/Kconfig config BLK_DEV_NVME · line 14

Quoted 3 lines · Christoph Hellwig
drivers/nvme/host/Kconfig +3 -0
+config BLK_DEV_NVME_SCSI
+	bool "SCSI emulation for NVMe device nodes"
+	depends on BLK_DEV_NVME

Can we make this "default y"? I strongly advocate for native NVMe over SCSI translations too, but I know of users relying on this. It's been built in for years and not aware of it breaking anything, so think it should be opt-out.

I'd really prefer not having to carry it around. But if any actual breakage shows I'd be happy to default it to y ASAP.