[PATCH-4.6 1/3] NVMe: Use simple ida interface

[PATCH-4.6 1/3] NVMe: Use simple ida interface

[Keith Busch]

This is an easier interface to use and removes one of the overloaded
dev_list_lock uses.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
Feature was originally posted by Lee Duncan last October, but this missed
being applied.

 drivers/nvme/host/core.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9b1253f..04443b2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1334,29 +1334,17 @@ static DEFINE_IDA(nvme_instance_ida);
 
 static int nvme_set_instance(struct nvme_ctrl *ctrl)
 {
-	int instance, error;
-
-	do {
-		if (!ida_pre_get(&nvme_instance_ida, GFP_KERNEL))
-			return -ENODEV;
-
-		spin_lock(&dev_list_lock);
-		error = ida_get_new(&nvme_instance_ida, &instance);
-		spin_unlock(&dev_list_lock);
-	} while (error == -EAGAIN);
-
-	if (error)
-		return -ENODEV;
+	int instance = ida_simple_get(&nvme_instance_ida, 0, 0, GFP_KERNEL);
 
+	if (instance < 0)
+		return instance;
 	ctrl->instance = instance;
 	return 0;
 }
 
 static void nvme_release_instance(struct nvme_ctrl *ctrl)
 {
-	spin_lock(&dev_list_lock);
-	ida_remove(&nvme_instance_ida, ctrl->instance);
-	spin_unlock(&dev_list_lock);
+	ida_simple_remove(&nvme_instance_ida, ctrl->instance);
 }
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
-- 
2.6.2.307.g37023ba

[PATCH-4.6 2/3] NVMe: Code naming cleanup

[Keith Busch]

Consistency help readability. This renames all instances of struct
nvme_ctrl to "ctrl" that weren't already named that.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 04443b2..33ad10d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -248,7 +248,7 @@ int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 			result, timeout);
 }
 
-int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
+int nvme_identify_ctrl(struct nvme_ctrl *ctrl, struct nvme_id_ctrl **id)
 {
 	struct nvme_command c = { };
 	int error;
@@ -261,24 +261,24 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 	if (!*id)
 		return -ENOMEM;
 
-	error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
+	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id,
 			sizeof(struct nvme_id_ctrl));
 	if (error)
 		kfree(*id);
 	return error;
 }
 
-static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
+static int nvme_identify_ns_list(struct nvme_ctrl *ctrl, unsigned nsid, __le32 *ns_list)
 {
 	struct nvme_command c = { };
 
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.cns = cpu_to_le32(2);
 	c.identify.nsid = cpu_to_le32(nsid);
-	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list, 0x1000);
+	return nvme_submit_sync_cmd(ctrl->admin_q, &c, ns_list, 0x1000);
 }
 
-int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
+int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 		struct nvme_id_ns **id)
 {
 	struct nvme_command c = { };
@@ -292,14 +292,14 @@ int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
 	if (!*id)
 		return -ENOMEM;
 
-	error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
+	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id,
 			sizeof(struct nvme_id_ns));
 	if (error)
 		kfree(*id);
 	return error;
 }
 
-int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
+int nvme_get_features(struct nvme_ctrl *ctrl, unsigned fid, unsigned nsid,
 					dma_addr_t dma_addr, u32 *result)
 {
 	struct nvme_command c;
@@ -310,10 +310,10 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 	c.features.prp1 = cpu_to_le64(dma_addr);
 	c.features.fid = cpu_to_le32(fid);
 
-	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0);
+	return __nvme_submit_sync_cmd(ctrl->admin_q, &c, NULL, 0, result, 0);
 }
 
-int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
+int nvme_set_features(struct nvme_ctrl *ctrl, unsigned fid, unsigned dword11,
 					dma_addr_t dma_addr, u32 *result)
 {
 	struct nvme_command c;
@@ -324,10 +324,10 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 	c.features.fid = cpu_to_le32(fid);
 	c.features.dword11 = cpu_to_le32(dword11);
 
-	return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0);
+	return __nvme_submit_sync_cmd(ctrl->admin_q, &c, NULL, 0, result, 0);
 }
 
-int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log)
+int nvme_get_log_page(struct nvme_ctrl *ctrl, struct nvme_smart_log **log)
 {
 	struct nvme_command c = { };
 	int error;
@@ -342,7 +342,7 @@ int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log)
 	if (!*log)
 		return -ENOMEM;
 
-	error = nvme_submit_sync_cmd(dev->admin_q, &c, *log,
+	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *log,
 			sizeof(struct nvme_smart_log));
 	if (error)
 		kfree(*log);
-- 
2.6.2.307.g37023ba

[PATCH-4.6 3/3] NVMe: Don't allow unsupported flags

[Keith Busch]

The command flags can change the meaning of other fields in the command
that the driver is not prepared to handle. Specifically, the user could
passthrough an SGL flag, causing the controller to misinterpret the PRP
list the driver created, potentially corrupting memory or data.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
Alternatively I have a different patch that builds SGL's if the flags
has it set and the device supports SGL. Any interest?

I didn't post it since the fast path only gets PRP-able scatter lists,
and the additional logic to handle SGL's complicates handling the NVMe
IO descriptor.

 drivers/nvme/host/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 33ad10d..d8c3a55 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -398,7 +398,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 
 	memset(&c, 0, sizeof(c));
 	c.rw.opcode = io.opcode;
-	c.rw.flags = io.flags;
+	c.rw.flags = 0;
 	c.rw.nsid = cpu_to_le32(ns->ns_id);
 	c.rw.slba = cpu_to_le64(io.slba);
 	c.rw.length = cpu_to_le16(io.nblocks);
@@ -428,7 +428,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 
 	memset(&c, 0, sizeof(c));
 	c.common.opcode = cmd.opcode;
-	c.common.flags = cmd.flags;
+	c.common.flags = 0;
 	c.common.nsid = cpu_to_le32(cmd.nsid);
 	c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
 	c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
-- 
2.6.2.307.g37023ba

[PATCH-4.6 3/3] NVMe: Don't allow unsupported flags

[Derrick, Jonathan]

I haven't seen a great argument for SGL other than that the devices support it. If anyone can prove it faster, I'd like to see it.

Otherwise the set looks good

-----Original Message-----
From: Linux-nvme [mailto:linux-nvme-bounces@lists.infradead.org] On Behalf Of Keith Busch
Sent: Thursday, February 18, 2016 4:21 PM
To: linux-nvme at lists.infradead.org; Jens Axboe <axboe at fb.com>; Christoph Hellwig <hch at infradead.org>
Cc: Busch, Keith <keith.busch at intel.com>
Subject: [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags

The command flags can change the meaning of other fields in the command that the driver is not prepared to handle. Specifically, the user could passthrough an SGL flag, causing the controller to misinterpret the PRP list the driver created, potentially corrupting memory or data.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
Alternatively I have a different patch that builds SGL's if the flags has it set and the device supports SGL. Any interest?

I didn't post it since the fast path only gets PRP-able scatter lists, and the additional logic to handle SGL's complicates handling the NVMe IO descriptor.

 drivers/nvme/host/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 33ad10d..d8c3a55 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -398,7 +398,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 
 	memset(&c, 0, sizeof(c));
 	c.rw.opcode = io.opcode;
-	c.rw.flags = io.flags;
+	c.rw.flags = 0;
 	c.rw.nsid = cpu_to_le32(ns->ns_id);
 	c.rw.slba = cpu_to_le64(io.slba);
 	c.rw.length = cpu_to_le16(io.nblocks); @@ -428,7 +428,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 
 	memset(&c, 0, sizeof(c));
 	c.common.opcode = cmd.opcode;
-	c.common.flags = cmd.flags;
+	c.common.flags = 0;
 	c.common.nsid = cpu_to_le32(cmd.nsid);
 	c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
 	c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
--
2.6.2.307.g37023ba


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

[PATCH-4.6 1/3] NVMe: Use simple ida interface

[Christoph Hellwig]

On Thu, Feb 18, 2016@04:21:15PM -0700, Keith Busch wrote:
> This is an easier interface to use and removes one of the overloaded
> dev_list_lock uses.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
> Feature was originally posted by Lee Duncan last October, but this missed
> being applied.

So is Lee's patch or yours?

Either way it looks fine to me:

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH-4.6 2/3] NVMe: Code naming cleanup

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH-4.6 3/3] NVMe: Don't allow unsupported flags

[Christoph Hellwig]

On Thu, Feb 18, 2016@04:21:17PM -0700, Keith Busch wrote:
> Alternatively I have a different patch that builds SGL's if the flags
> has it set and the device supports SGL. Any interest?
> 
> I didn't post it since the fast path only gets PRP-able scatter lists,
> and the additional logic to handle SGL's complicates handling the NVMe
> IO descriptor.

I don't think we should allow applications to control the SGL use.
There are a couple reasons for that in upcoming specs that I can't talk
about, but I think it should be common sense to not expose these sorts
of low level details to userspace.

>  drivers/nvme/host/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 33ad10d..d8c3a55 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -398,7 +398,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
>  
>  	memset(&c, 0, sizeof(c));
>  	c.rw.opcode = io.opcode;
> -	c.rw.flags = io.flags;
> +	c.rw.flags = 0;

Shouldn't we check that flags is zero in the user structure and reject
the command if not?  Otherwise we can't safely add back support for
flags in the future if useful ones appear that we might want to support.

[PATCH-4.6 1/3] NVMe: Use simple ida interface

[Sagi Grimberg]

Looks good to me,

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

[PATCH-4.6 2/3] NVMe: Code naming cleanup

[Sagi Grimberg]

Looks good,

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