Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1544747 > unrolled thread

[PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code. Scott Bauer <scott.bauer@intel.com> - 2016-12-19 20:50 +0100
    Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Keith Busch <keith.busch@intel.com> - 2016-12-19 23:00 +0100
      Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Scott Bauer <scott.bauer@intel.com> - 2016-12-19 23:40 +0100
        Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-20 07:20 +0100
          Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Keith Busch <keith.busch@intel.com> - 2016-12-20 16:40 +0100
            Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-20 16:50 +0100
              Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Scott Bauer <sbauer@eng.utah.edu> - 2016-12-20 17:40 +0100
                Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-21 10:10 +0100
              Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Scott Bauer <scott.bauer@intel.com> - 2016-12-20 19:10 +0100
                Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-21 10:40 +0100
    Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. kbuild test robot <lkp@intel.com> - 2016-12-20 05:20 +0100
    Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-20 07:30 +0100
    Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-20 07:50 +0100
    Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Jethro Beekman <kernel@jbeekman.nl> - 2016-12-25 15:50 +0100
      Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Scott Bauer <scott.bauer@intel.com> - 2016-12-27 23:30 +0100
        Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED  Allocation code. Christoph Hellwig <hch@infradead.org> - 2016-12-28 09:50 +0100

#1544747 — [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

This patch implements the necessary logic to unlock a SED
enabled device coming back from an S3.

The patch also implements the necessary logic to allocate the
appropriate opal_dev structures to support the OPAL protocol.

Signed-off-by: Scott Bauer <scott.bauer@intel.com>
Signed-off-by: Rafael Antognolli <Rafael.Antognolli@intel.com>
---
 drivers/nvme/host/core.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  8 +++++-
 drivers/nvme/host/pci.c  | 10 +++++++-
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b40cfb0..f9731ce 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -28,6 +28,8 @@
 #include <linux/t10-pi.h>
 #include <scsi/sg.h>
 #include <asm/unaligned.h>
+#include <linux/sed.h>
+#include <linux/sed-opal.h>
 
 #include "nvme.h"
 #include "fabrics.h"
@@ -762,6 +764,48 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	return status;
 }
 
+static int nvme_sec_submit(struct nvme_ctrl *ctrl, u16 spsp, u8 secp,
+			   void *buffer, size_t len, u8 opcode)
+{
+	struct nvme_command cmd = { 0 };
+	struct nvme_ns *ns = NULL;
+
+	mutex_lock(&ctrl->namespaces_mutex);
+	if (!list_empty(&ctrl->namespaces))
+		ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
+
+	mutex_unlock(&ctrl->namespaces_mutex);
+	if (!ns)
+		return -ENODEV;
+
+	cmd.common.opcode = opcode;
+	cmd.common.nsid = ns->ns_id;
+	cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
+	cmd.common.cdw10[1] = cpu_to_le32(len);
+
+	return __nvme_submit_sync_cmd(ctrl->admin_q, &cmd, NULL, buffer, len,
+				      ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0);
+}
+
+static int nvme_sec_send(void *ctrl_data, u16 spsp, u8 secp,
+			 void *buf, size_t len)
+{
+	return nvme_sec_submit(ctrl_data, spsp, secp, buf, len,
+			       nvme_admin_security_send);
+}
+
+static int nvme_sec_recv(void *ctrl_data, u16 spsp, u8 secp,
+			 void *buf, size_t len)
+{
+	return nvme_sec_submit(ctrl_data, spsp, secp, buf, len,
+			       nvme_admin_security_recv);
+}
+
+static const struct sec_ops nvme_sec_ops = {
+	.sec_send = nvme_sec_send,
+	.sec_recv = nvme_sec_recv,
+};
+
 static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		unsigned int cmd, unsigned long arg)
 {
@@ -1051,6 +1095,28 @@ static const struct pr_ops nvme_pr_ops = {
 	.pr_clear	= nvme_pr_clear,
 };
 
+int nvme_opal_initialize(struct nvme_ctrl *ctrl)
+{
+	/* Opal dev has already been allocated for this controller */
+	if (ctrl->sed_ctx.dev)
+		return 0;
+
+	ctrl->sed_ctx.dev = alloc_opal_dev(ctrl->admin_q);
+	if (!ctrl->sed_ctx.dev)
+		return -ENOMEM;
+	ctrl->sed_ctx.ops = &nvme_sec_ops;
+	ctrl->sed_ctx.sec_data = ctrl;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nvme_opal_initialize);
+
+void nvme_unlock_from_suspend(struct nvme_ctrl *ctrl)
+{
+	if (opal_unlock_from_suspend(&ctrl->sed_ctx))
+		pr_warn("Failed to unlock one or more locking ranges!\n");
+}
+EXPORT_SYMBOL_GPL(nvme_unlock_from_suspend);
+
 static const struct block_device_operations nvme_fops = {
 	.owner		= THIS_MODULE,
 	.ioctl		= nvme_ioctl,
@@ -1312,6 +1378,7 @@ static int nvme_dev_open(struct inode *inode, struct file *file)
 		if (!kref_get_unless_zero(&ctrl->kref))
 			break;
 		file->private_data = ctrl;
+		file->f_sedctx = &ctrl->sed_ctx;
 		ret = 0;
 		break;
 	}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bd53214..851830b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -19,6 +19,7 @@
 #include <linux/kref.h>
 #include <linux/blk-mq.h>
 #include <linux/lightnvm.h>
+#include <linux/sed.h>
 
 enum {
 	/*
@@ -151,6 +152,8 @@ struct nvme_ctrl {
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
 
+	struct sed_context sed_ctx;
+
 	/* Fabrics only */
 	u16 sqsize;
 	u32 ioccsz;
@@ -256,7 +259,8 @@ static inline int nvme_error_status(u16 status)
 
 static inline bool nvme_req_needs_retry(struct request *req, u16 status)
 {
-	return !(status & NVME_SC_DNR || blk_noretry_request(req)) &&
+	return !(status & NVME_SC_DNR || status & NVME_SC_ACCESS_DENIED ||
+		 blk_noretry_request(req)) &&
 		(jiffies - req->start_time) < req->timeout &&
 		req->retries < nvme_max_retries;
 }
@@ -275,6 +279,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl);
 
 void nvme_queue_scan(struct nvme_ctrl *ctrl);
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
+void nvme_unlock_from_suspend(struct nvme_ctrl *ctrl);
+int nvme_opal_initialize(struct nvme_ctrl *ctrl);
 
 #define NVME_NR_AERS	1
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 2fd7dc2..d298c15 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -43,6 +43,7 @@
 #include <linux/types.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <asm/unaligned.h>
+#include <linux/sed-opal.h>
 
 #include "nvme.h"
 
@@ -1765,7 +1766,7 @@ static void nvme_reset_work(struct work_struct *work)
 {
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work);
 	int result = -ENODEV;
-
+	bool was_suspend = !!(dev->ctrl.ctrl_config & NVME_CC_SHN_NORMAL);
 	if (WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING))
 		goto out;
 
@@ -1796,6 +1797,13 @@ static void nvme_reset_work(struct work_struct *work)
 	if (result)
 		goto out;
 
+	result = nvme_opal_initialize(&dev->ctrl);
+	if (result)
+		goto out;
+
+	if (was_suspend)
+		nvme_unlock_from_suspend(&dev->ctrl);
+
 	result = nvme_setup_io_queues(dev);
 	if (result)
 		goto out;
-- 
2.7.4

[toc] | [next] | [standalone]

#1544785 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Mon, Dec 19, 2016 at 12:35:48PM -0700, Scott Bauer wrote:
> +static int nvme_sec_submit(struct nvme_ctrl *ctrl, u16 spsp, u8 secp,
> +			   void *buffer, size_t len, u8 opcode)
> +{
> +	struct nvme_command cmd = { 0 };
> +	struct nvme_ns *ns = NULL;
> +
> +	mutex_lock(&ctrl->namespaces_mutex);
> +	if (!list_empty(&ctrl->namespaces))
> +		ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
> +
> +	mutex_unlock(&ctrl->namespaces_mutex);
> +	if (!ns)
> +		return -ENODEV;
> +
> +	cmd.common.opcode = opcode;
> +	cmd.common.nsid = ns->ns_id;

Should be:

	cmd.common.nsid = cpu_to_le32(ns->ns_id);

But now wondering how you can send a security command to different
namespaces. That's why I thought it'd make more sense to threa this
through block_device, but maybe Christoph had some idea on how to get
the same functionality without that?

[toc] | [prev] | [next] | [standalone]

#1544799 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Mon, Dec 19, 2016 at 04:59:54PM -0500, Keith Busch wrote:
> On Mon, Dec 19, 2016 at 12:35:48PM -0700, Scott Bauer wrote:
> > +static int nvme_sec_submit(struct nvme_ctrl *ctrl, u16 spsp, u8 secp,
> > +			   void *buffer, size_t len, u8 opcode)
> > +{
> > +	struct nvme_command cmd = { 0 };
> > +	struct nvme_ns *ns = NULL;
> > +
> > +	mutex_lock(&ctrl->namespaces_mutex);
> > +	if (!list_empty(&ctrl->namespaces))
> > +		ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
> > +
> > +	mutex_unlock(&ctrl->namespaces_mutex);
> > +	if (!ns)
> > +		return -ENODEV;
> > +
> > +	cmd.common.opcode = opcode;
> > +	cmd.common.nsid = ns->ns_id;
> 
> Should be:
> 
> 	cmd.common.nsid = cpu_to_le32(ns->ns_id);
> 
> But now wondering how you can send a security command to different
> namespaces. That's why I thought it'd make more sense to threa this
> through block_device, but maybe Christoph had some idea on how to get
> the same functionality without that?


I went back and reviewed the spec 1.2.1:

http://www.nvmexpress.org/wp-content/uploads/NVM_Express_1_2_1_Gold_20160603.pdf
Section 5.18 (page 140->141)

Describes the security send command type and it doesn't have any reference of
a namespace ID. Anecdotally, I just removed the ns->ns_id line from the code and
everything still works as intended. Is there another portion of the spec or errata
that requires ns_id? (I can't access 1.2.1 errta the link doesn't work).

[toc] | [prev] | [next] | [standalone]

#1544933 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Mon, Dec 19, 2016 at 03:23:12PM -0700, Scott Bauer wrote:
> I went back and reviewed the spec 1.2.1:
> 
> http://www.nvmexpress.org/wp-content/uploads/NVM_Express_1_2_1_Gold_20160603.pdf
> Section 5.18 (page 140->141)
> 
> Describes the security send command type and it doesn't have any reference of
> a namespace ID. Anecdotally, I just removed the ns->ns_id line from the code and
> everything still works as intended. Is there another portion of the spec or errata
> that requires ns_id? (I can't access 1.2.1 errta the link doesn't work).

As far as I can tell Security Send / Receive has always been intended to
apply to the whole controller, even if that's something I would not
personally think is a good idea.

[toc] | [prev] | [next] | [standalone]

#1545209 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Mon, Dec 19, 2016 at 10:17:44PM -0800, Christoph Hellwig wrote:
> As far as I can tell Security Send / Receive has always been intended to
> apply to the whole controller, even if that's something I would not
> personally think is a good idea.

NVMe security commands required the namespace ID since the very
beginning. It's currently documented in figure 42 of section 5,
"Namespace Identifier Used" column.

[toc] | [prev] | [next] | [standalone]

#1545216 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Tue, Dec 20, 2016 at 10:49:16AM -0500, Keith Busch wrote:
> On Mon, Dec 19, 2016 at 10:17:44PM -0800, Christoph Hellwig wrote:
> > As far as I can tell Security Send / Receive has always been intended to
> > apply to the whole controller, even if that's something I would not
> > personally think is a good idea.
> 
> NVMe security commands required the namespace ID since the very
> beginning. It's currently documented in figure 42 of section 5,
> "Namespace Identifier Used" column.

Oh, for some reason I read a no there when looking it up.
Good to know, although TCG spec still seem to ignore it.

[toc] | [prev] | [next] | [standalone]

#1545251 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On 12/20/2016 08:46 AM, Christoph Hellwig wrote:
> On Tue, Dec 20, 2016 at 10:49:16AM -0500, Keith Busch wrote:
>> On Mon, Dec 19, 2016 at 10:17:44PM -0800, Christoph Hellwig wrote:
>>> As far as I can tell Security Send / Receive has always been intended to
>>> apply to the whole controller, even if that's something I would not
>>> personally think is a good idea.
>>
>> NVMe security commands required the namespace ID since the very
>> beginning. It's currently documented in figure 42 of section 5,
>> "Namespace Identifier Used" column.
> 
> Oh, for some reason I read a no there when looking it up.
> Good to know, although TCG spec still seem to ignore it.

Thanks Keith. Although TCG Spec currently ignores it in the future it may not.
In that case we should probably attempt to future proof it a bit. Since the
namespace ID is accessible via the block device structure I'll find a way to
include that in some opaque pointer that we can deliver through the core into
NVMe.

But this also brings up another question (and part of the reason I moved from
the block ioctl to fs ioctl): For drives with multiple namsepaces is it 
acceptable to allow a namespace, who has a segregated chunk of space, the ability
to perform actions outside of its range? Since the multiple namespaces portion
of the spec says there will be one Global LR a namespace that doesn't encompass
the entire LBA range can end up locking other LBAs via locking the global.

That's why I wanted to go to char dev because it seemed like a better fit for 
this scenario. Any thoughts on the above?



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

[toc] | [prev] | [next] | [standalone]

#1545677 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Tue, Dec 20, 2016 at 09:05:32AM -0700, Scott Bauer wrote:
> Thanks Keith. Although TCG Spec currently ignores it in the future it may not.
> In that case we should probably attempt to future proof it a bit. Since the
> namespace ID is accessible via the block device structure I'll find a way to
> include that in some opaque pointer that we can deliver through the core into
> NVMe.

Honestly I think this nsid issue is a major, major mess.  Without a nsid
only the global range makes sense, and in that case it does not make
sense to set a nsid (so it should be 0xFFFFFFFF).  This one more of the
major major issues because the NVMe group decided to not care about
security and outsourced it to that giant heap of crap called TCG.

I think we'll need to start a little conversation on the nvme list on
how to handle this, including potential errata.

> But this also brings up another question (and part of the reason I moved from
> the block ioctl to fs ioctl): For drives with multiple namsepaces is it 
> acceptable to allow a namespace, who has a segregated chunk of space, the ability
> to perform actions outside of its range? Since the multiple namespaces portion
> of the spec says there will be one Global LR a namespace that doesn't encompass
> the entire LBA range can end up locking other LBAs via locking the global.
> 
> That's why I wanted to go to char dev because it seemed like a better fit for 
> this scenario. Any thoughts on the above?

In SCSI we allow all kinds of ioctls affecting target-wide behavior on
the device nodes.  These needs to be protected using CAP_SYS_ADMIN,
otherwise we're going to get security issues, though (and in SCSI land
we had such issues before).

[toc] | [prev] | [next] | [standalone]

#1545332 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Tue, Dec 20, 2016 at 07:46:39AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 20, 2016 at 10:49:16AM -0500, Keith Busch wrote:
> > On Mon, Dec 19, 2016 at 10:17:44PM -0800, Christoph Hellwig wrote:
> > > As far as I can tell Security Send / Receive has always been intended to
> > > apply to the whole controller, even if that's something I would not
> > > personally think is a good idea.
> > 
> > NVMe security commands required the namespace ID since the very
> > beginning. It's currently documented in figure 42 of section 5,
> > "Namespace Identifier Used" column.
> 
> Oh, for some reason I read a no there when looking it up.
> Good to know, although TCG spec still seem to ignore it.

Before I submit another version I want to address a few design issues we seem
to be walking around a bit:

The other reviews you gave for the series are fine and will be implemented,
thank you for that.

The main development issue seems to be how the drivers/block layer interact
with the core sed.

1) We will move the core from lib/ back to block/ and add CONFIGS in kconfig.

2) Do we want to continue passing around a sed_context to the core? Instead of
   a block_device struct like we did in previous versions.

 2a) If we do wish to do wish to continue passng sed_contexts to the core I
 have to add a new variable to the block_device structure for our sed_context.
 Will this be acceptable? It wasn't acceptable for the file struct. The reason
 I need a new variable in the struct is:
 On the ioctl path, if we intercept the SED call in the block layer ioctl and
 have the call chain be:
 uland -> blk_ioctl -> sed_ioctl() -> sedcore -> sec_send/recv -> nvme
 then I need to be able to pass a sed_ctx struct in blk_ioctl to sed-ioctl and
 the only  way is to have it sitting in our block_device structure.

 The other way which was sorta nack'd last time is the following call chain:
 uland -> blk_ioctl -> nvme_ioctl -> sed_ioctl -> sedcore -> send/rcv -> nvme

 In this call chain in nvme_ioctl we have access to our block device struct
 and from there we can do blkdev->bd_disk->private_data to get our ns and then
 eventually our sed_ctx to pass to sed_ioctl. I could add the ns to the sec_data
 pointer in sed_context. This would give us access to ns without having to pass
 around a block device or store it anywhere.

 In the first scenario I can't work at all with opaque pointers like we can in
 the drivers itself (private_data). I don't know what they are, the drivers have
 the domain knowledge of what type they actually stored in private_data. That's
 why I need an explicit member in the block_device for the first scenario.

3) For NVMe we need access to our ns ID. It's in the block_device behind a few
pointers. What I can do is if we want to continue with the first ioctl path
described above is something like:

sed_ioctl(struct block_device *bdev, ...)
{
  sed_context *ctx = bdev->sed_ctx;
  ctx->sed_data = bdev->bd_disk->private_data;
  switch(cmd) {
  ...
  ...
  return some_opal_cmd(ctx);
  }
}

While this works for NVMe I don't know if this is acceptible for *all* users.
Since this is in a generic ioctl that is supposed to work with all drivers, who
knows what the hell they're putting in private_data and whether its useful for
their implementation of sec_send/recv.

I think that's all I have for now. If I think of anything throughout the day I'll
reply to to this email.

[toc] | [prev] | [next] | [standalone]

#1545681 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Tue, Dec 20, 2016 at 10:52:41AM -0700, Scott Bauer wrote:
> 2) Do we want to continue passing around a sed_context to the core?
> Instead of a block_device struct like we did in previous versions.

My personal preference would be the block_device, but in this case my
preference collides with the (sad) reality, and that reality is that TCG
did a major, major fuckup in specifyiong the NVMe interaction in the
SIIG and does not treat TPer as per-Namespaces, as it does in SCSI for
LUNs.  So in NVMe all the interactions are on a per-controller level,
not per namespaces.  And the block_device maps to the namespacespace in
NVMe.  So I fear we'll have to keep the sed_context.

>  2a) If we do wish to do wish to continue passng sed_contexts to the core I
>  have to add a new variable to the block_device structure for our sed_context.
>  Will this be acceptable?

We have a lot less block_device structures, and they are limited to
block devices, so I think it should be acceptable.

>  It wasn't acceptable for the file struct. The reason
>  I need a new variable in the struct is:
>  On the ioctl path, if we intercept the SED call in the block layer ioctl and
>  have the call chain be:
>  uland -> blk_ioctl -> sed_ioctl() -> sedcore -> sec_send/recv -> nvme

But I really don't think we strictly need it for this reason.  The above
callchain should be:

	userland -> blk_ioctl -> nvme_ioctl -> sed_opal_iocl

This allows passing the sec context including the submission method
to sed_opal_ioctl from the driver and should not require anything
in the block device.

>  then I need to be able to pass a sed_ctx struct in blk_ioctl to sed-ioctl and
>  the only  way is to have it sitting in our block_device structure.
> 
>  The other way which was sorta nack'd last time is the following call chain:
>  uland -> blk_ioctl -> nvme_ioctl -> sed_ioctl -> sedcore -> send/rcv -> nvme

Why was this nacked?  This is still my preference, except that it could
still be simplified a bit per the other comments, e.g. I don't think
we really need a sec_core.

>  In this call chain in nvme_ioctl we have access to our block device struct
>  and from there we can do blkdev->bd_disk->private_data to get our ns and then
>  eventually our sed_ctx to pass to sed_ioctl. I could add the ns to the sec_data
>  pointer in sed_context. This would give us access to ns without having to pass
>  around a block device or store it anywhere.

> 3) For NVMe we need access to our ns ID. It's in the block_device behind a few
> pointers. What I can do is if we want to continue with the first ioctl path
> described above is something like:

The think is the ns does not matter for the OPAL device.  I suspect
the right answer is to pass 0xffffffff as the nsid.  I need to verify
this with some devices I should have access to, and you should verity
it with Intel.  I've also kicked a mail to some people involved with TCG
to see if we can get some agreement on this behavior.  If we can't get
agreement on that we'll just need to have a variable in the nvme_ctrl
that stores some valid nsid just for security send/receive.

> While this works for NVMe I don't know if this is acceptible for *all* users.

The other users are SCSI, ATA and eMMC.

For SCSI the scsi_device is per-lun, and the SIIS specifies that all
TPers are per-lun, so we'll simply have a context per scsi_device,
otherwise it should work the same as NVMe.  ATA doesn't support LUNs
(except for ATAPI, which is SCSI over ATA), so it's even simpler.
Additionally Linux hides ATA behind the SCSI layer, so the only thing
we'll need to implement is the translation between the two.  I don't
really know enough about eMMC to comment on it.

[toc] | [prev] | [next] | [standalone]

#1544893 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

[Multipart message — attachments visible in raw view] — view raw

Hi Scott,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.9 next-20161219]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Scott-Bauer/include-Add-definitions-for-sed/20161220-110214
config: x86_64-randconfig-i0-201651 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/nvme/host/core.c: In function 'nvme_sec_submit':
>> drivers/nvme/host/core.c:770:9: warning: missing braces around initializer [-Wmissing-braces]
     struct nvme_command cmd = { 0 };
            ^
   drivers/nvme/host/core.c:770:9: warning: (near initialization for 'cmd.<anonymous>') [-Wmissing-braces]
   Cyclomatic Complexity 5 include/linux/compiler.h:__read_once_size
   Cyclomatic Complexity 5 include/linux/compiler.h:__write_once_size
   Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:__set_bit
   Cyclomatic Complexity 2 arch/x86/include/asm/bitops.h:test_and_set_bit
   Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:constant_test_bit
   Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:variable_test_bit
   Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:fls64
   Cyclomatic Complexity 1 include/uapi/linux/byteorder/little_endian.h:__le64_to_cpup
   Cyclomatic Complexity 1 include/uapi/linux/byteorder/little_endian.h:__le16_to_cpup
   Cyclomatic Complexity 1 include/linux/log2.h:__ilog2_u64
   Cyclomatic Complexity 1 include/linux/list.h:INIT_LIST_HEAD
   Cyclomatic Complexity 1 include/linux/list.h:__list_del
   Cyclomatic Complexity 1 include/linux/list.h:list_empty
   Cyclomatic Complexity 1 arch/x86/include/asm/current.h:get_current
   Cyclomatic Complexity 2 arch/x86/include/asm/page_64.h:__phys_addr_nodebug
   Cyclomatic Complexity 1 include/asm-generic/getorder.h:__get_order
   Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:atomic_read
   Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:atomic_set
   Cyclomatic Complexity 2 arch/x86/include/asm/atomic.h:atomic_sub_and_test
   Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:atomic_add_return
   Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:atomic_cmpxchg
   Cyclomatic Complexity 5 arch/x86/include/asm/atomic.h:__atomic_add_unless
   Cyclomatic Complexity 1 include/linux/atomic.h:atomic_add_unless
   Cyclomatic Complexity 1 include/linux/err.h:PTR_ERR
   Cyclomatic Complexity 1 include/linux/thread_info.h:check_object_size
   Cyclomatic Complexity 1 include/linux/spinlock.h:spinlock_check
   Cyclomatic Complexity 1 include/linux/spinlock.h:spin_lock
   Cyclomatic Complexity 1 include/linux/spinlock.h:spin_lock_irq
   Cyclomatic Complexity 1 include/linux/spinlock.h:spin_unlock
   Cyclomatic Complexity 1 include/linux/spinlock.h:spin_unlock_irq
   Cyclomatic Complexity 1 include/linux/jiffies.h:_msecs_to_jiffies
   Cyclomatic Complexity 5 include/linux/jiffies.h:msecs_to_jiffies
   Cyclomatic Complexity 1 include/linux/workqueue.h:to_delayed_work
   Cyclomatic Complexity 1 include/linux/signal.h:sigismember
   Cyclomatic Complexity 1 include/linux/sched.h:task_thread_info
   Cyclomatic Complexity 1 include/linux/slab.h:__kmalloc_node
   Cyclomatic Complexity 68 include/linux/slab.h:kmalloc_large
   Cyclomatic Complexity 5 include/linux/slab.h:kmalloc
   Cyclomatic Complexity 1 include/linux/slab.h:kmalloc_node
   Cyclomatic Complexity 1 include/linux/slab.h:kzalloc
   Cyclomatic Complexity 1 include/linux/slab.h:kzalloc_node
   Cyclomatic Complexity 1 include/linux/kref.h:kref_init
   Cyclomatic Complexity 1 include/linux/kref.h:kref_get_unless_zero
   Cyclomatic Complexity 1 include/linux/device.h:dev_to_node
   Cyclomatic Complexity 1 include/linux/device.h:dev_get_drvdata
   Cyclomatic Complexity 1 include/linux/fs.h:iminor
   Cyclomatic Complexity 1 include/linux/genhd.h:get_capacity
   Cyclomatic Complexity 1 include/linux/genhd.h:set_capacity
   Cyclomatic Complexity 1 include/linux/kasan-checks.h:kasan_check_read
   Cyclomatic Complexity 1 include/linux/kasan-checks.h:kasan_check_write
   Cyclomatic Complexity 11 arch/x86/include/asm/uaccess.h:copy_from_user
   Cyclomatic Complexity 11 arch/x86/include/asm/uaccess.h:copy_to_user
   Cyclomatic Complexity 1 include/linux/blk_types.h:op_is_write
   Cyclomatic Complexity 1 include/linux/blkdev.h:queue_flag_set_unlocked
   Cyclomatic Complexity 1 include/linux/blkdev.h:blk_rq_pos
   Cyclomatic Complexity 1 include/linux/blkdev.h:blk_rq_bytes
   Cyclomatic Complexity 1 include/linux/blkdev.h:blk_integrity_rq
   Cyclomatic Complexity 1 include/linux/blkdev.h:blk_queue_max_integrity_segments
   Cyclomatic Complexity 1 include/linux/blk-mq.h:blk_mq_rq_to_pdu
   Cyclomatic Complexity 1 include/linux/module.h:try_module_get
   Cyclomatic Complexity 1 include/linux/module.h:module_put
   Cyclomatic Complexity 1 include/linux/unaligned/access_ok.h:put_unaligned_le64
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_req
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_block_nr
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_nvm_register
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_nvm_unregister
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_nvm_register_sysfs
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_nvm_unregister_sysfs
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_nvm_ns_supported
   Cyclomatic Complexity 1 drivers/nvme/host/nvme.h:nvme_get_ns_from_dev
   Cyclomatic Complexity 1 drivers/nvme/host/core.c:nvme_getgeo
   Cyclomatic Complexity 7 drivers/nvme/host/core.c:nvme_pr_type
   Cyclomatic Complexity 1 drivers/nvme/host/core.c:nvme_sysfs_show_address
   Cyclomatic Complexity 1 drivers/nvme/host/core.c:ns_cmp
   Cyclomatic Complexity 2 drivers/nvme/host/core.c:nvme_async_event_work
   Cyclomatic Complexity 2 include/linux/thread_info.h:test_ti_thread_flag
   Cyclomatic Complexity 1 include/linux/sched.h:test_tsk_thread_flag
   Cyclomatic Complexity 3 include/linux/blkdev.h:blk_get_integrity
   Cyclomatic Complexity 3 drivers/nvme/host/core.c:nvme_sysfs_reset
   Cyclomatic Complexity 21 drivers/nvme/host/core.c:nvme_dev_attrs_are_visible
   Cyclomatic Complexity 9 include/linux/blkdev.h:queue_logical_block_size
   Cyclomatic Complexity 7 drivers/nvme/host/core.c:nvme_dev_open
   Cyclomatic Complexity 3 drivers/nvme/host/nvme.h:nvme_reset_subsystem
   Cyclomatic Complexity 3 include/linux/nvme.h:nvme_is_write
   Cyclomatic Complexity 2 include/linux/err.h:IS_ERR
   Cyclomatic Complexity 1 include/linux/sched.h:signal_pending
   Cyclomatic Complexity 1 include/linux/sched.h:__fatal_signal_pending
   Cyclomatic Complexity 3 include/linux/sched.h:fatal_signal_pending
   Cyclomatic Complexity 1 drivers/nvme/host/core.c:nvme_setup_flush
   Cyclomatic Complexity 3 drivers/nvme/host/core.c:nvme_setup_discard
   Cyclomatic Complexity 13 drivers/nvme/host/core.c:nvme_setup_rw
   Cyclomatic Complexity 4 arch/x86/include/asm/uaccess.h:copy_user_overflow
   Cyclomatic Complexity 1 include/linux/workqueue.h:queue_delayed_work
   Cyclomatic Complexity 1 include/linux/workqueue.h:schedule_delayed_work
   Cyclomatic Complexity 3 drivers/nvme/host/core.c:nvme_keep_alive_end_io
   Cyclomatic Complexity 12 drivers/nvme/host/core.c:nvme_wait_ready
   Cyclomatic Complexity 7 drivers/nvme/host/core.c:nvme_set_queue_limits

vim +770 drivers/nvme/host/core.c

   754			timeout = msecs_to_jiffies(cmd.timeout_ms);
   755	
   756		status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
   757				(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
   758				&cmd.result, timeout);
   759		if (status >= 0) {
   760			if (put_user(cmd.result, &ucmd->result))
   761				return -EFAULT;
   762		}
   763	
   764		return status;
   765	}
   766	
   767	static int nvme_sec_submit(struct nvme_ctrl *ctrl, u16 spsp, u8 secp,
   768				   void *buffer, size_t len, u8 opcode)
   769	{
 > 770		struct nvme_command cmd = { 0 };
   771		struct nvme_ns *ns = NULL;
   772	
   773		mutex_lock(&ctrl->namespaces_mutex);
   774		if (!list_empty(&ctrl->namespaces))
   775			ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
   776	
   777		mutex_unlock(&ctrl->namespaces_mutex);
   778		if (!ns)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1544936 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

> +static int nvme_sec_send(void *ctrl_data, u16 spsp, u8 secp,
> +			 void *buf, size_t len)
> +{
> +	return nvme_sec_submit(ctrl_data, spsp, secp, buf, len,
> +			       nvme_admin_security_send);
> +}
> +
> +static int nvme_sec_recv(void *ctrl_data, u16 spsp, u8 secp,
> +			 void *buf, size_t len)
> +{
> +	return nvme_sec_submit(ctrl_data, spsp, secp, buf, len,
> +			       nvme_admin_security_recv);
> +}
> +
> +static const struct sec_ops nvme_sec_ops = {
> +	.sec_send = nvme_sec_send,
> +	.sec_recv = nvme_sec_recv,
> +};

Just make sec_submit the callback passed to the core and avoid
this boiler-plate code.

>  
> +int nvme_opal_initialize(struct nvme_ctrl *ctrl)
> +{
> +	/* Opal dev has already been allocated for this controller */
> +	if (ctrl->sed_ctx.dev)
> +		return 0;
> +
> +	ctrl->sed_ctx.dev = alloc_opal_dev(ctrl->admin_q);
> +	if (!ctrl->sed_ctx.dev)
> +		return -ENOMEM;
> +	ctrl->sed_ctx.ops = &nvme_sec_ops;
> +	ctrl->sed_ctx.sec_data = ctrl;

No need for sec_data callback, just pass the sed_ctx to the driver
and use container_of to get at the containing structure.

[toc] | [prev] | [next] | [standalone]

#1544948 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

> +void nvme_unlock_from_suspend(struct nvme_ctrl *ctrl)
> +{
> +	if (opal_unlock_from_suspend(&ctrl->sed_ctx))
> +		pr_warn("Failed to unlock one or more locking ranges!\n");
> +}
> +EXPORT_SYMBOL_GPL(nvme_unlock_from_suspend);

I don't think we even need this wrapper.  Also for the warning please
use dev_warn so that the user knows which controller failed.

> @@ -1765,7 +1766,7 @@ static void nvme_reset_work(struct work_struct *work)
>  {
>  	struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work);
>  	int result = -ENODEV;
> -
> +	bool was_suspend = !!(dev->ctrl.ctrl_config & NVME_CC_SHN_NORMAL);
>  	if (WARN_ON(dev->ctrl.state == NVME_CTRL_RESETTING))

Please don't remove the empty line after the variable declarations.
Also I would place your new line before the result line, as that makes
it a tad easier to read.

[toc] | [prev] | [next] | [standalone]

#1547161 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On 19-12-16 20:35, Scott Bauer wrote:
> @@ -1796,6 +1797,13 @@ static void nvme_reset_work(struct work_struct *work)
>  	if (result)
>  		goto out;
>  
> +	result = nvme_opal_initialize(&dev->ctrl);
> +	if (result)
> +		goto out;

It seems you always try to intialize OPAL even if the drive doesn't support it.
I think you should check if the device supports security commands and then see
if it supports OPAL before calling this. See e.g.
https://lkml.org/lkml/2016/6/19/139 . Ideally, this code would check all
supported protocols and initialize the appropriate security device based on that.

Jethro

[toc] | [prev] | [next] | [standalone]

#1547772 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

On Sun, Dec 25, 2016 at 03:15:52PM +0100, Jethro Beekman wrote:
> On 19-12-16 20:35, Scott Bauer wrote:
> > @@ -1796,6 +1797,13 @@ static void nvme_reset_work(struct work_struct *work)
> >  	if (result)
> >  		goto out;
> >  
> > +	result = nvme_opal_initialize(&dev->ctrl);
> > +	if (result)
> > +		goto out;
> 
> It seems you always try to intialize OPAL even if the drive doesn't support it.
> I think you should check if the device supports security commands and then see
> if it supports OPAL before calling this. See e.g.
> https://lkml.org/lkml/2016/6/19/139 . Ideally, this code would check all
> supported protocols and initialize the appropriate security device based on that.

The nvme_opal_initalize should probably be changed to nvme_opal_allocate or something.
It's not really initalizing anything other than allocting the necessary structures
for OPAL. In order to determine if the controller supports opal we need to allocate
the previously mentioned structures anyway. I want to stay away from making payloads
(specifically discovery0 ) payload in the nvme driver and allow the opal core to do a
all the grunt work. In the future we'll probably have to refactor the core a bit to do
just packet generation. It looks like at least for NVMe we're going to have to do a discovery
to figure out whether we've got mutiple locking ranges per NS or just one global lr during
initialization.

[toc] | [prev] | [next] | [standalone]

#1547883 — Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code.

> for OPAL. In order to determine if the controller supports opal we need to allocate
> the previously mentioned structures anyway. I want to stay away from making payloads
> (specifically discovery0 ) payload in the nvme driver and allow the opal core to do a
> all the grunt work. In the future we'll probably have to refactor the core a bit to do
> just packet generation. It looks like at least for NVMe we're going to have to do a discovery
> to figure out whether we've got mutiple locking ranges per NS or just one global lr during
> initialization.

We might have to do discovery in the nvme driver - there also is the
ATA security feature supported by various consumer drives.  Jethro has
been doing some work in that direction, although driven by userspace
for now. 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web