namespace list locking and ioctl fixes

namespace list locking and ioctl fixes

[Christoph Hellwig]

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.

[PATCH 1/3] nvme: synchronize access to ctrl->namespaces

[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>
---
 drivers/nvme/host/core.c | 13 +++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  |  4 ++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1437ff3..eba7e96 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d88cf45..d203745 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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;
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b82bbea..e4a6868 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1907,6 +1907,7 @@ static void nvme_freeze_queues(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns;
 
+	mutex_lock(&dev->ctrl.namespaces_mutex);
 	list_for_each_entry(ns, &dev->ctrl.namespaces, list) {
 		blk_mq_freeze_queue_start(ns->queue);
 
@@ -1917,18 +1918,21 @@ static void nvme_freeze_queues(struct nvme_dev *dev)
 		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
 	}
+	mutex_unlock(&dev->ctrl.namespaces_mutex);
 }
 
 static void nvme_unfreeze_queues(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns;
 
+	mutex_lock(&dev->ctrl.namespaces_mutex);
 	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);
 	}
+	mutex_unlock(&dev->ctrl.namespaces_mutex);
 }
 
 static void nvme_dev_shutdown(struct nvme_dev *dev)
-- 
1.9.1

[PATCH 1/3] nvme: synchronize access to ctrl->namespaces

[Sagi Grimberg]

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

Ha yes, I may have seen strange behavior in this area wrt to
deletes and resets.

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index b82bbea..e4a6868 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c

Umm, I submitted a patch to move these to the core (which you reviewed
:)). I've not seen any negative feedback on it so far.

Jens, are you planning on taking it? and if you do,
Christoph, would you mind rebasing a top of it?

Otherwise,

Looks good,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

> @@ -1907,6 +1907,7 @@ static void nvme_freeze_queues(struct nvme_dev *dev)
>   {
>   	struct nvme_ns *ns;
>
> +	mutex_lock(&dev->ctrl.namespaces_mutex);
>   	list_for_each_entry(ns, &dev->ctrl.namespaces, list) {
>   		blk_mq_freeze_queue_start(ns->queue);
>
> @@ -1917,18 +1918,21 @@ static void nvme_freeze_queues(struct nvme_dev *dev)
>   		blk_mq_cancel_requeue_work(ns->queue);
>   		blk_mq_stop_hw_queues(ns->queue);
>   	}
> +	mutex_unlock(&dev->ctrl.namespaces_mutex);
>   }
>
>   static void nvme_unfreeze_queues(struct nvme_dev *dev)
>   {
>   	struct nvme_ns *ns;
>
> +	mutex_lock(&dev->ctrl.namespaces_mutex);
>   	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);
>   	}
> +	mutex_unlock(&dev->ctrl.namespaces_mutex);
>   }
>
>   static void nvme_dev_shutdown(struct nvme_dev *dev)
>

[PATCH 1/3] nvme: synchronize access to ctrl->namespaces

[Christoph Hellwig]

On Thu, Dec 24, 2015@12:36:01PM +0200, Sagi Grimberg wrote:
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index b82bbea..e4a6868 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>
> Umm, I submitted a patch to move these to the core (which you reviewed
> :)). I've not seen any negative feedback on it so far.
>
> Jens, are you planning on taking it? and if you do,
> Christoph, would you mind rebasing a top of it?

I'm fine with rebasing.  Those were the last accesses to the namespace
list outside the core driver anyway.

[PATCH 2/3] nvme: fixes for NVME_IOCTL_IO_CMD on the char device

[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>
---
 drivers/nvme/host/core.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eba7e96..19b854e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
-- 
1.9.1

[PATCH 2/3] nvme: fixes for NVME_IOCTL_IO_CMD on the char device

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

[PATCH 3/3] nvme: make SG_IO support optional

[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(-)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 002a94a..5d62373 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -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.
diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
index 3e26dc9..baf9f52 100644
--- a/drivers/nvme/host/Makefile
+++ b/drivers/nvme/host/Makefile
@@ -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
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 19b854e..570a36c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
 	}
-- 
1.9.1

[PATCH 3/3] nvme: make SG_IO support optional

[Sagi Grimberg]

> 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.

Umm, this is needed to support multipathing (at least in the multipath
daemon current form). I'm not sure this is a good idea just yet...

[PATCH 3/3] nvme: make SG_IO support optional

[Christoph Hellwig]

On Thu, Dec 24, 2015@12:39:40PM +0200, Sagi Grimberg wrote:
>
>> 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.
>
> Umm, this is needed to support multipathing (at least in the multipath
> daemon current form). I'm not sure this is a good idea just yet...

It's not required, it's just you being lazy.  I defintivively want
to avoid people relying on this before actual multi path hardware
ships.  So feel free to enable it while you're lazy for now, but
I want people to disable it normally.

[PATCH 3/3] nvme: make SG_IO support optional

[Sagi Grimberg]

>>> 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.
>>
>> Umm, this is needed to support multipathing (at least in the multipath
>> daemon current form). I'm not sure this is a good idea just yet...
>
> It's not required, it's just you being lazy.

I didn't say it's required, I said it's needed for the current
multipathd implementation... and yes, I am lazy, multipathd is
not the most friendly code...

> I defintivively want
> to avoid people relying on this before actual multi path hardware
> ships.  So feel free to enable it while you're lazy for now, but
> I want people to disable it normally.

OK, I see what you mean. We can look at having multipathd ioctl'ing
identify_ns at some point...

[PATCH 3/3] nvme: make SG_IO support optional

[Christoph Hellwig]

On Thu, Dec 24, 2015@01:31:32PM +0200, Sagi Grimberg wrote:
> OK, I see what you mean. We can look at having multipathd ioctl'ing
> identify_ns at some point...

block/for-next has the nguid available as a sysfs attribute, that's
all we should need.

[PATCH 3/3] nvme: make SG_IO support optional

[Keith Busch]

On Thu, Dec 24, 2015@12:40:40PM +0100, Christoph Hellwig wrote:
> On Thu, Dec 24, 2015@01:31:32PM +0200, Sagi Grimberg wrote:
> > OK, I see what you mean. We can look at having multipathd ioctl'ing
> > identify_ns at some point...
> 
> block/for-next has the nguid available as a sysfs attribute, that's
> all we should need.

Off the top of your head, do you happen to know how to point multipathd
to the new attribute? Is this a code change or just a configuration rule?

[PATCH 3/3] nvme: make SG_IO support optional

[Christoph Hellwig]

On Thu, Dec 24, 2015@03:29:46PM +0000, Keith Busch wrote:
> On Thu, Dec 24, 2015@12:40:40PM +0100, Christoph Hellwig wrote:
> > On Thu, Dec 24, 2015@01:31:32PM +0200, Sagi Grimberg wrote:
> > > OK, I see what you mean. We can look at having multipathd ioctl'ing
> > > identify_ns at some point...
> > 
> > block/for-next has the nguid available as a sysfs attribute, that's
> > all we should need.
> 
> Off the top of your head, do you happen to know how to point multipathd
> to the new attribute? Is this a code change or just a configuration rule?

With the most recent mutlipath-tools there should be no need to patch
mutipath-tools.  Just make sure udev sets the ID_SERIAL attribute
correctly, which I think some folks were already looking into.  An
explicit getuid callout that just reads the file should also work
with any versions, although the most recent ones call it deprectated.