* OPAL fixups
@ 2017-02-17 12:59 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
Hi all,
this contains a few more OPAL-related fixups. It tones down warnings a bit,
allocates the OPAL-ѕpecific data structure in a separate dynamic allocation,
checks for support of Security Send/Receive in NVMe before using them,
and makes sure we re-discovery the security capabilities after each reset.
^ permalink raw reply [flat|nested] 24+ messages in thread
* OPAL fixups
@ 2017-02-17 12:59 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
Hi all,
this contains a few more OPAL-related fixups. It tones down warnings a bit,
allocates the OPAL-?pecific data structure in a separate dynamic allocation,
checks for support of Security Send/Receive in NVMe before using them,
and makes sure we re-discovery the security capabilities after each reset.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/4] block/sed-opal: tone down not supported warnings
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 12:59 ` Christoph Hellwig
-1 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
Not having OPAL or a sub-feature supported is an entirely normal
condition for many drives, so don't warn about it. Keep the messages,
but tone them down to debug only.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index e95b8a57053d..bcdd5b6d02e8 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -387,16 +387,16 @@ static int opal_discovery0_end(struct opal_dev *dev)
}
if (!supported) {
- pr_err("This device is not Opal enabled. Not Supported!\n");
+ pr_debug("This device is not Opal enabled. Not Supported!\n");
return -EOPNOTSUPP;
}
if (!single_user)
- pr_warn("Device doesn't support single user mode\n");
+ pr_debug("Device doesn't support single user mode\n");
if (!found_com_id) {
- pr_warn("Could not find OPAL comid for device. Returning early\n");
+ pr_debug("Could not find OPAL comid for device. Returning early\n");
return -EOPNOTSUPP;;
}
@@ -1951,7 +1951,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
mutex_init(&opal_dev->dev_lock);
opal_dev->send_recv = send_recv;
if (check_opal_support(opal_dev) < 0)
- pr_warn("Opal is not supported on this device\n");
+ pr_debug("Opal is not supported on this device\n");
opal_dev->initialized = true;
}
EXPORT_SYMBOL(init_opal_dev);
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/4] block/sed-opal: tone down not supported warnings
@ 2017-02-17 12:59 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
Not having OPAL or a sub-feature supported is an entirely normal
condition for many drives, so don't warn about it. Keep the messages,
but tone them down to debug only.
Signed-off-by: Christoph Hellwig <hch at lst.de>
Reviewed-by: Scott Bauer <scott.bauer at intel.com>
---
block/sed-opal.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index e95b8a57053d..bcdd5b6d02e8 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -387,16 +387,16 @@ static int opal_discovery0_end(struct opal_dev *dev)
}
if (!supported) {
- pr_err("This device is not Opal enabled. Not Supported!\n");
+ pr_debug("This device is not Opal enabled. Not Supported!\n");
return -EOPNOTSUPP;
}
if (!single_user)
- pr_warn("Device doesn't support single user mode\n");
+ pr_debug("Device doesn't support single user mode\n");
if (!found_com_id) {
- pr_warn("Could not find OPAL comid for device. Returning early\n");
+ pr_debug("Could not find OPAL comid for device. Returning early\n");
return -EOPNOTSUPP;;
}
@@ -1951,7 +1951,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
mutex_init(&opal_dev->dev_lock);
opal_dev->send_recv = send_recv;
if (check_opal_support(opal_dev) < 0)
- pr_warn("Opal is not supported on this device\n");
+ pr_debug("Opal is not supported on this device\n");
opal_dev->initialized = true;
}
EXPORT_SYMBOL(init_opal_dev);
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 12:59 ` Christoph Hellwig
-1 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
Insted of bloating the containing structure with it all the time this
allocates struct opal_dev dynamically. Additionally this allows moving
the definition of struct opal_dev into sed-opal.c. For this a new
private data field is added to it that is passed to the send/receive
callback. After that a lot of internals can be made private as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: Scott Bauer <scott.bauer@intel.com>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>
---
block/opal_proto.h | 23 ++++++++++
block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++----
drivers/nvme/host/core.c | 9 ++--
drivers/nvme/host/nvme.h | 14 ++----
drivers/nvme/host/pci.c | 8 +++-
include/linux/sed-opal.h | 116 ++---------------------------------------------
6 files changed, 131 insertions(+), 140 deletions(-)
diff --git a/block/opal_proto.h b/block/opal_proto.h
index af9abc56c157..f40c9acf8895 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -19,6 +19,29 @@
#ifndef _OPAL_PROTO_H
#define _OPAL_PROTO_H
+/*
+ * These constant values come from:
+ * SPC-4 section
+ * 6.30 SECURITY PROTOCOL IN command / table 265.
+ */
+enum {
+ TCG_SECP_00 = 0,
+ TCG_SECP_01,
+};
+
+/*
+ * Token defs derived from:
+ * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
+ * 3.2.2 Data Stream Encoding
+ */
+enum opal_response_token {
+ OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
+ OPAL_DTA_TOKENID_SINT = 0xe1,
+ OPAL_DTA_TOKENID_UINT = 0xe2,
+ OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
+ OPAL_DTA_TOKENID_INVALID = 0X0
+};
+
#define DTAERROR_NO_METHOD_STATUS 0x89
#define GENERIC_HOST_SESSION_NUM 0x41
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bcdd5b6d02e8..d1c52ba4d62d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -31,6 +31,77 @@
#include "opal_proto.h"
+#define IO_BUFFER_LENGTH 2048
+#define MAX_TOKS 64
+
+typedef int (*opal_step)(struct opal_dev *dev);
+
+enum opal_atom_width {
+ OPAL_WIDTH_TINY,
+ OPAL_WIDTH_SHORT,
+ OPAL_WIDTH_MEDIUM,
+ OPAL_WIDTH_LONG,
+ OPAL_WIDTH_TOKEN
+};
+
+/*
+ * On the parsed response, we don't store again the toks that are already
+ * stored in the response buffer. Instead, for each token, we just store a
+ * pointer to the position in the buffer where the token starts, and the size
+ * of the token in bytes.
+ */
+struct opal_resp_tok {
+ const u8 *pos;
+ size_t len;
+ enum opal_response_token type;
+ enum opal_atom_width width;
+ union {
+ u64 u;
+ s64 s;
+ } stored;
+};
+
+/*
+ * From the response header it's not possible to know how many tokens there are
+ * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
+ * if we start dealing with messages that have more than that, we can increase
+ * this number. This is done to avoid having to make two passes through the
+ * response, the first one counting how many tokens we have and the second one
+ * actually storing the positions.
+ */
+struct parsed_resp {
+ int num;
+ struct opal_resp_tok toks[MAX_TOKS];
+};
+
+struct opal_dev {
+ bool supported;
+
+ void *data;
+ sec_send_recv *send_recv;
+
+ const opal_step *funcs;
+ void **func_data;
+ int state;
+ struct mutex dev_lock;
+ u16 comid;
+ u32 hsn;
+ u32 tsn;
+ u64 align;
+ u64 lowest_lba;
+
+ size_t pos;
+ u8 cmd[IO_BUFFER_LENGTH];
+ u8 resp[IO_BUFFER_LENGTH];
+
+ struct parsed_resp parsed;
+ size_t prev_d_len;
+ void *prev_data;
+
+ struct list_head unlk_lst;
+};
+
+
static const u8 opaluid[][OPAL_UID_LENGTH] = {
/* users */
[OPAL_SMUID_UID] =
@@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data)
static int opal_send_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->cmd, IO_BUFFER_LENGTH,
true);
}
static int opal_recv_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->resp, IO_BUFFER_LENGTH,
false);
}
@@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev)
return ret;
}
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
{
- if (opal_dev->initialized)
- return;
- INIT_LIST_HEAD(&opal_dev->unlk_lst);
- mutex_init(&opal_dev->dev_lock);
- opal_dev->send_recv = send_recv;
- if (check_opal_support(opal_dev) < 0)
+ struct opal_dev *dev;
+
+ dev = kmalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return NULL;
+
+ INIT_LIST_HEAD(&dev->unlk_lst);
+ mutex_init(&dev->dev_lock);
+ dev->data = data;
+ dev->send_recv = send_recv;
+ if (check_opal_support(dev) != 0) {
pr_debug("Opal is not supported on this device\n");
- opal_dev->initialized = true;
+ kfree(dev);
+ return NULL;
+ }
+ return dev;
}
EXPORT_SYMBOL(init_opal_dev);
@@ -2351,6 +2430,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
+ if (!dev)
+ return -ENOTSUPP;
if (!dev->supported) {
pr_err("Not supported\n");
return -ENOTSUPP;
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 04c48e75ff16..8aeb4a623b65 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return nvme_nvm_ioctl(ns, cmd, arg);
#endif
if (is_sed_ioctl(cmd))
- return sed_ioctl(&ns->ctrl->opal_dev, cmd,
+ return sed_ioctl(ns->ctrl->opal_dev, cmd,
(void __user *) arg);
return -ENOTTY;
}
@@ -1085,18 +1085,17 @@ static const struct pr_ops nvme_pr_ops = {
};
#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send)
{
+ struct nvme_ctrl *ctrl = data;
struct nvme_command cmd;
- struct nvme_ctrl *ctrl = NULL;
memset(&cmd, 0, sizeof(cmd));
if (send)
cmd.common.opcode = nvme_admin_security_send;
else
cmd.common.opcode = nvme_admin_security_recv;
- ctrl = container_of(dev, struct nvme_ctrl, opal_dev);
cmd.common.nsid = 0;
cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
cmd.common.cdw10[1] = cpu_to_le32(len);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 569cba14cede..5126c4bbee1a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -126,7 +126,7 @@ struct nvme_ctrl {
struct list_head node;
struct ida ns_ida;
- struct opal_dev opal_dev;
+ struct opal_dev *opal_dev;
char name[12];
char serial[20];
@@ -270,16 +270,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);
-#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-#else
-static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
-{
- return 0;
-}
-#endif /* CONFIG_BLK_DEV_SED_OPAL */
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send);
#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 e25d632bd9f2..5db8a38a8b43 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
if (dev->ctrl.admin_q)
blk_put_queue(dev->ctrl.admin_q);
kfree(dev->queues);
+ kfree(dev->ctrl.opal_dev);
kfree(dev);
}
@@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit);
+ if (!dev->ctrl.opal_dev) {
+ dev->ctrl.opal_dev =
+ init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ }
if (was_suspend)
- opal_unlock_from_suspend(&dev->ctrl.opal_dev);
+ opal_unlock_from_suspend(dev->ctrl.opal_dev);
result = nvme_setup_io_queues(dev);
if (result)
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index 205d520ea688..deee23d012e7 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -21,117 +21,14 @@
#include <uapi/linux/sed-opal.h>
#include <linux/kernel.h>
-/*
- * These constant values come from:
- * SPC-4 section
- * 6.30 SECURITY PROTOCOL IN command / table 265.
- */
-enum {
- TCG_SECP_00 = 0,
- TCG_SECP_01,
-};
struct opal_dev;
-#define IO_BUFFER_LENGTH 2048
-#define MAX_TOKS 64
-
-typedef int (*opal_step)(struct opal_dev *dev);
-typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-
-
-enum opal_atom_width {
- OPAL_WIDTH_TINY,
- OPAL_WIDTH_SHORT,
- OPAL_WIDTH_MEDIUM,
- OPAL_WIDTH_LONG,
- OPAL_WIDTH_TOKEN
-};
-
-/*
- * Token defs derived from:
- * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
- * 3.2.2 Data Stream Encoding
- */
-enum opal_response_token {
- OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
- OPAL_DTA_TOKENID_SINT = 0xe1,
- OPAL_DTA_TOKENID_UINT = 0xe2,
- OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
- OPAL_DTA_TOKENID_INVALID = 0X0
-};
-
-/*
- * On the parsed response, we don't store again the toks that are already
- * stored in the response buffer. Instead, for each token, we just store a
- * pointer to the position in the buffer where the token starts, and the size
- * of the token in bytes.
- */
-struct opal_resp_tok {
- const u8 *pos;
- size_t len;
- enum opal_response_token type;
- enum opal_atom_width width;
- union {
- u64 u;
- s64 s;
- } stored;
-};
-
-/*
- * From the response header it's not possible to know how many tokens there are
- * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
- * if we start dealing with messages that have more than that, we can increase
- * this number. This is done to avoid having to make two passes through the
- * response, the first one counting how many tokens we have and the second one
- * actually storing the positions.
- */
-struct parsed_resp {
- int num;
- struct opal_resp_tok toks[MAX_TOKS];
-};
-
-/**
- * struct opal_dev - The structure representing a OPAL enabled SED.
- * @supported: Whether or not OPAL is supported on this controller.
- * @send_recv: The combined sec_send/sec_recv function pointer.
- * @opal_step: A series of opal methods that are necessary to complete a command.
- * @func_data: An array of parameters for the opal methods above.
- * @state: Describes the current opal_step we're working on.
- * @dev_lock: Locks the entire opal_dev structure.
- * @parsed: Parsed response from controller.
- * @prev_data: Data returned from a method to the controller.
- * @unlk_lst: A list of Locking ranges to unlock on this device during a resume.
- */
-struct opal_dev {
- bool initialized;
- bool supported;
- sec_send_recv *send_recv;
-
- const opal_step *funcs;
- void **func_data;
- int state;
- struct mutex dev_lock;
- u16 comid;
- u32 hsn;
- u32 tsn;
- u64 align;
- u64 lowest_lba;
-
- size_t pos;
- u8 cmd[IO_BUFFER_LENGTH];
- u8 resp[IO_BUFFER_LENGTH];
-
- struct parsed_resp parsed;
- size_t prev_d_len;
- void *prev_data;
-
- struct list_head unlk_lst;
-};
+typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer,
+ size_t len, bool send);
#ifdef CONFIG_BLK_SED_OPAL
bool opal_unlock_from_suspend(struct opal_dev *dev);
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv);
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr);
static inline bool is_sed_ioctl(unsigned int cmd)
@@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev)
{
return false;
}
-static inline void init_opal_dev(struct opal_dev *opal_dev,
- sec_send_recv *send_recv)
-{
- opal_dev->supported = false;
- opal_dev->initialized = true;
-}
+#define init_opal_dev(data, send_recv) NULL
#endif /* CONFIG_BLK_SED_OPAL */
#endif /* LINUX_OPAL_H */
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically
@ 2017-02-17 12:59 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
Insted of bloating the containing structure with it all the time this
allocates struct opal_dev dynamically. Additionally this allows moving
the definition of struct opal_dev into sed-opal.c. For this a new
private data field is added to it that is passed to the send/receive
callback. After that a lot of internals can be made private as well.
Signed-off-by: Christoph Hellwig <hch at lst.de>
Tested-by: Scott Bauer <scott.bauer at intel.com>
Reviewed-by: Scott Bauer <scott.bauer at intel.com>
---
block/opal_proto.h | 23 ++++++++++
block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++----
drivers/nvme/host/core.c | 9 ++--
drivers/nvme/host/nvme.h | 14 ++----
drivers/nvme/host/pci.c | 8 +++-
include/linux/sed-opal.h | 116 ++---------------------------------------------
6 files changed, 131 insertions(+), 140 deletions(-)
diff --git a/block/opal_proto.h b/block/opal_proto.h
index af9abc56c157..f40c9acf8895 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -19,6 +19,29 @@
#ifndef _OPAL_PROTO_H
#define _OPAL_PROTO_H
+/*
+ * These constant values come from:
+ * SPC-4 section
+ * 6.30 SECURITY PROTOCOL IN command / table 265.
+ */
+enum {
+ TCG_SECP_00 = 0,
+ TCG_SECP_01,
+};
+
+/*
+ * Token defs derived from:
+ * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
+ * 3.2.2 Data Stream Encoding
+ */
+enum opal_response_token {
+ OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
+ OPAL_DTA_TOKENID_SINT = 0xe1,
+ OPAL_DTA_TOKENID_UINT = 0xe2,
+ OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
+ OPAL_DTA_TOKENID_INVALID = 0X0
+};
+
#define DTAERROR_NO_METHOD_STATUS 0x89
#define GENERIC_HOST_SESSION_NUM 0x41
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bcdd5b6d02e8..d1c52ba4d62d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -31,6 +31,77 @@
#include "opal_proto.h"
+#define IO_BUFFER_LENGTH 2048
+#define MAX_TOKS 64
+
+typedef int (*opal_step)(struct opal_dev *dev);
+
+enum opal_atom_width {
+ OPAL_WIDTH_TINY,
+ OPAL_WIDTH_SHORT,
+ OPAL_WIDTH_MEDIUM,
+ OPAL_WIDTH_LONG,
+ OPAL_WIDTH_TOKEN
+};
+
+/*
+ * On the parsed response, we don't store again the toks that are already
+ * stored in the response buffer. Instead, for each token, we just store a
+ * pointer to the position in the buffer where the token starts, and the size
+ * of the token in bytes.
+ */
+struct opal_resp_tok {
+ const u8 *pos;
+ size_t len;
+ enum opal_response_token type;
+ enum opal_atom_width width;
+ union {
+ u64 u;
+ s64 s;
+ } stored;
+};
+
+/*
+ * From the response header it's not possible to know how many tokens there are
+ * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
+ * if we start dealing with messages that have more than that, we can increase
+ * this number. This is done to avoid having to make two passes through the
+ * response, the first one counting how many tokens we have and the second one
+ * actually storing the positions.
+ */
+struct parsed_resp {
+ int num;
+ struct opal_resp_tok toks[MAX_TOKS];
+};
+
+struct opal_dev {
+ bool supported;
+
+ void *data;
+ sec_send_recv *send_recv;
+
+ const opal_step *funcs;
+ void **func_data;
+ int state;
+ struct mutex dev_lock;
+ u16 comid;
+ u32 hsn;
+ u32 tsn;
+ u64 align;
+ u64 lowest_lba;
+
+ size_t pos;
+ u8 cmd[IO_BUFFER_LENGTH];
+ u8 resp[IO_BUFFER_LENGTH];
+
+ struct parsed_resp parsed;
+ size_t prev_d_len;
+ void *prev_data;
+
+ struct list_head unlk_lst;
+};
+
+
static const u8 opaluid[][OPAL_UID_LENGTH] = {
/* users */
[OPAL_SMUID_UID] =
@@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data)
static int opal_send_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->cmd, IO_BUFFER_LENGTH,
true);
}
static int opal_recv_cmd(struct opal_dev *dev)
{
- return dev->send_recv(dev, dev->comid, TCG_SECP_01,
+ return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
dev->resp, IO_BUFFER_LENGTH,
false);
}
@@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev)
return ret;
}
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
{
- if (opal_dev->initialized)
- return;
- INIT_LIST_HEAD(&opal_dev->unlk_lst);
- mutex_init(&opal_dev->dev_lock);
- opal_dev->send_recv = send_recv;
- if (check_opal_support(opal_dev) < 0)
+ struct opal_dev *dev;
+
+ dev = kmalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return NULL;
+
+ INIT_LIST_HEAD(&dev->unlk_lst);
+ mutex_init(&dev->dev_lock);
+ dev->data = data;
+ dev->send_recv = send_recv;
+ if (check_opal_support(dev) != 0) {
pr_debug("Opal is not supported on this device\n");
- opal_dev->initialized = true;
+ kfree(dev);
+ return NULL;
+ }
+ return dev;
}
EXPORT_SYMBOL(init_opal_dev);
@@ -2351,6 +2430,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
+ if (!dev)
+ return -ENOTSUPP;
if (!dev->supported) {
pr_err("Not supported\n");
return -ENOTSUPP;
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 04c48e75ff16..8aeb4a623b65 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
return nvme_nvm_ioctl(ns, cmd, arg);
#endif
if (is_sed_ioctl(cmd))
- return sed_ioctl(&ns->ctrl->opal_dev, cmd,
+ return sed_ioctl(ns->ctrl->opal_dev, cmd,
(void __user *) arg);
return -ENOTTY;
}
@@ -1085,18 +1085,17 @@ static const struct pr_ops nvme_pr_ops = {
};
#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send)
{
+ struct nvme_ctrl *ctrl = data;
struct nvme_command cmd;
- struct nvme_ctrl *ctrl = NULL;
memset(&cmd, 0, sizeof(cmd));
if (send)
cmd.common.opcode = nvme_admin_security_send;
else
cmd.common.opcode = nvme_admin_security_recv;
- ctrl = container_of(dev, struct nvme_ctrl, opal_dev);
cmd.common.nsid = 0;
cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
cmd.common.cdw10[1] = cpu_to_le32(len);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 569cba14cede..5126c4bbee1a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -126,7 +126,7 @@ struct nvme_ctrl {
struct list_head node;
struct ida ns_ida;
- struct opal_dev opal_dev;
+ struct opal_dev *opal_dev;
char name[12];
char serial[20];
@@ -270,16 +270,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);
-#ifdef CONFIG_BLK_SED_OPAL
-int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-#else
-static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send)
-{
- return 0;
-}
-#endif /* CONFIG_BLK_DEV_SED_OPAL */
+int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
+ bool send);
#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 e25d632bd9f2..5db8a38a8b43 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
if (dev->ctrl.admin_q)
blk_put_queue(dev->ctrl.admin_q);
kfree(dev->queues);
+ kfree(dev->ctrl.opal_dev);
kfree(dev);
}
@@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit);
+ if (!dev->ctrl.opal_dev) {
+ dev->ctrl.opal_dev =
+ init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ }
if (was_suspend)
- opal_unlock_from_suspend(&dev->ctrl.opal_dev);
+ opal_unlock_from_suspend(dev->ctrl.opal_dev);
result = nvme_setup_io_queues(dev);
if (result)
diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
index 205d520ea688..deee23d012e7 100644
--- a/include/linux/sed-opal.h
+++ b/include/linux/sed-opal.h
@@ -21,117 +21,14 @@
#include <uapi/linux/sed-opal.h>
#include <linux/kernel.h>
-/*
- * These constant values come from:
- * SPC-4 section
- * 6.30 SECURITY PROTOCOL IN command / table 265.
- */
-enum {
- TCG_SECP_00 = 0,
- TCG_SECP_01,
-};
struct opal_dev;
-#define IO_BUFFER_LENGTH 2048
-#define MAX_TOKS 64
-
-typedef int (*opal_step)(struct opal_dev *dev);
-typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp,
- void *buffer, size_t len, bool send);
-
-
-enum opal_atom_width {
- OPAL_WIDTH_TINY,
- OPAL_WIDTH_SHORT,
- OPAL_WIDTH_MEDIUM,
- OPAL_WIDTH_LONG,
- OPAL_WIDTH_TOKEN
-};
-
-/*
- * Token defs derived from:
- * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
- * 3.2.2 Data Stream Encoding
- */
-enum opal_response_token {
- OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
- OPAL_DTA_TOKENID_SINT = 0xe1,
- OPAL_DTA_TOKENID_UINT = 0xe2,
- OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
- OPAL_DTA_TOKENID_INVALID = 0X0
-};
-
-/*
- * On the parsed response, we don't store again the toks that are already
- * stored in the response buffer. Instead, for each token, we just store a
- * pointer to the position in the buffer where the token starts, and the size
- * of the token in bytes.
- */
-struct opal_resp_tok {
- const u8 *pos;
- size_t len;
- enum opal_response_token type;
- enum opal_atom_width width;
- union {
- u64 u;
- s64 s;
- } stored;
-};
-
-/*
- * From the response header it's not possible to know how many tokens there are
- * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
- * if we start dealing with messages that have more than that, we can increase
- * this number. This is done to avoid having to make two passes through the
- * response, the first one counting how many tokens we have and the second one
- * actually storing the positions.
- */
-struct parsed_resp {
- int num;
- struct opal_resp_tok toks[MAX_TOKS];
-};
-
-/**
- * struct opal_dev - The structure representing a OPAL enabled SED.
- * @supported: Whether or not OPAL is supported on this controller.
- * @send_recv: The combined sec_send/sec_recv function pointer.
- * @opal_step: A series of opal methods that are necessary to complete a command.
- * @func_data: An array of parameters for the opal methods above.
- * @state: Describes the current opal_step we're working on.
- * @dev_lock: Locks the entire opal_dev structure.
- * @parsed: Parsed response from controller.
- * @prev_data: Data returned from a method to the controller.
- * @unlk_lst: A list of Locking ranges to unlock on this device during a resume.
- */
-struct opal_dev {
- bool initialized;
- bool supported;
- sec_send_recv *send_recv;
-
- const opal_step *funcs;
- void **func_data;
- int state;
- struct mutex dev_lock;
- u16 comid;
- u32 hsn;
- u32 tsn;
- u64 align;
- u64 lowest_lba;
-
- size_t pos;
- u8 cmd[IO_BUFFER_LENGTH];
- u8 resp[IO_BUFFER_LENGTH];
-
- struct parsed_resp parsed;
- size_t prev_d_len;
- void *prev_data;
-
- struct list_head unlk_lst;
-};
+typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer,
+ size_t len, bool send);
#ifdef CONFIG_BLK_SED_OPAL
bool opal_unlock_from_suspend(struct opal_dev *dev);
-void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
+struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv);
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *ioctl_ptr);
static inline bool is_sed_ioctl(unsigned int cmd)
@@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev)
{
return false;
}
-static inline void init_opal_dev(struct opal_dev *opal_dev,
- sec_send_recv *send_recv)
-{
- opal_dev->supported = false;
- opal_dev->initialized = true;
-}
+#define init_opal_dev(data, send_recv) NULL
#endif /* CONFIG_BLK_SED_OPAL */
#endif /* LINUX_OPAL_H */
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands.
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 12:59 ` Christoph Hellwig
-1 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
From: Scott Bauer <scott.bauer@intel.com>
We need to verify that the controller supports the security
commands before actually trying to issue them.
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
[hch: moved the check so that we don't call into the OPAL code if not
supported]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 1 +
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/pci.c | 2 +-
include/linux/nvme.h | 1 +
4 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8aeb4a623b65..44a1a257e0b5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1285,6 +1285,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
return -EIO;
}
+ ctrl->oacs = le16_to_cpu(id->oacs);
ctrl->vid = le16_to_cpu(id->vid);
ctrl->oncs = le16_to_cpup(&id->oncs);
atomic_set(&ctrl->abort_limit, id->acl + 1);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5126c4bbee1a..14cfc6f7facb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -140,6 +140,7 @@ struct nvme_ctrl {
u32 max_hw_sectors;
u16 oncs;
u16 vid;
+ u16 oacs;
atomic_t abort_limit;
u8 event_limit;
u8 vwc;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5db8a38a8b43..ddc51adb594d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,7 +1789,7 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if (!dev->ctrl.opal_dev) {
+ if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
dev->ctrl.opal_dev =
init_opal_dev(&dev->ctrl, &nvme_sec_submit);
}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3e2ed49c3ad8..0b676a02cf3e 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -244,6 +244,7 @@ enum {
NVME_CTRL_ONCS_DSM = 1 << 2,
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
+ NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
};
struct nvme_lbaf {
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands.
@ 2017-02-17 12:59 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
From: Scott Bauer <scott.bauer@intel.com>
We need to verify that the controller supports the security
commands before actually trying to issue them.
Signed-off-by: Scott Bauer <scott.bauer at intel.com>
[hch: moved the check so that we don't call into the OPAL code if not
supported]
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
drivers/nvme/host/core.c | 1 +
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/pci.c | 2 +-
include/linux/nvme.h | 1 +
4 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8aeb4a623b65..44a1a257e0b5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1285,6 +1285,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
return -EIO;
}
+ ctrl->oacs = le16_to_cpu(id->oacs);
ctrl->vid = le16_to_cpu(id->vid);
ctrl->oncs = le16_to_cpup(&id->oncs);
atomic_set(&ctrl->abort_limit, id->acl + 1);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5126c4bbee1a..14cfc6f7facb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -140,6 +140,7 @@ struct nvme_ctrl {
u32 max_hw_sectors;
u16 oncs;
u16 vid;
+ u16 oacs;
atomic_t abort_limit;
u8 event_limit;
u8 vwc;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5db8a38a8b43..ddc51adb594d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,7 +1789,7 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if (!dev->ctrl.opal_dev) {
+ if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
dev->ctrl.opal_dev =
init_opal_dev(&dev->ctrl, &nvme_sec_submit);
}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3e2ed49c3ad8..0b676a02cf3e 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -244,6 +244,7 @@ enum {
NVME_CTRL_ONCS_DSM = 1 << 2,
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
+ NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
};
struct nvme_lbaf {
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 12:59 ` Christoph Hellwig
-1 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
To: scott.bauer, keith.busch, jonathan.derrick, axboe; +Cc: linux-block, linux-nvme
A device may change capabilities after each reset, e.g. due to a firmware
upgrade. We should thus check for Security Send/Receive and OPAL support
after each reset.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/pci.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ddc51adb594d..c5986850f88b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
+ kfree(dev->ctrl.opal_dev);
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
dev->ctrl.opal_dev =
init_opal_dev(&dev->ctrl, &nvme_sec_submit);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
@ 2017-02-17 12:59 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 12:59 UTC (permalink / raw)
A device may change capabilities after each reset, e.g. due to a firmware
upgrade. We should thus check for Security Send/Receive and OPAL support
after each reset.
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
drivers/nvme/host/pci.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ddc51adb594d..c5986850f88b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
+ kfree(dev->ctrl.opal_dev);
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
dev->ctrl.opal_dev =
init_opal_dev(&dev->ctrl, &nvme_sec_submit);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 15:26 ` Keith Busch
-1 siblings, 0 replies; 24+ messages in thread
From: Keith Busch @ 2017-02-17 15:26 UTC (permalink / raw)
To: Christoph Hellwig
Cc: scott.bauer, jonathan.derrick, axboe, linux-block, linux-nvme
On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote:
> @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> if (result)
> goto out;
>
> - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> + kfree(dev->ctrl.opal_dev);
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> dev->ctrl.opal_dev =
> init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> }
A couple things.
This has a use-after-free in opal_unlock_from_suspend if the nvme
device had an opal_dev before, but no longer support the capability
after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
But we don't want to unconditionally free it anyway during resume
since opal_unlock_from_suspend requires the exisiting opal_dev state
information saved in the 'unlk_list'.
Something like this instead:
---
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ddc51ad..8fa6be9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
- dev->ctrl.opal_dev =
- init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP)
+ if (was_suspend && dev->ctrl.opal_dev)
+ opal_unlock_from_suspend(dev->ctrl.opal_dev);
+ else if (!dev->ctrl.opal_dev)
+ dev->ctrl.opal_dev =
+ init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ } else {
+ kfree(dev->ctrl.opal_dev);
+ dev->ctrl.opal_dev = NULL;
}
- if (was_suspend)
- opal_unlock_from_suspend(dev->ctrl.opal_dev);
result = nvme_setup_io_queues(dev);
if (result)
--
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
@ 2017-02-17 15:26 ` Keith Busch
0 siblings, 0 replies; 24+ messages in thread
From: Keith Busch @ 2017-02-17 15:26 UTC (permalink / raw)
On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote:
> @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> if (result)
> goto out;
>
> - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> + kfree(dev->ctrl.opal_dev);
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> dev->ctrl.opal_dev =
> init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> }
A couple things.
This has a use-after-free in opal_unlock_from_suspend if the nvme
device had an opal_dev before, but no longer support the capability
after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
But we don't want to unconditionally free it anyway during resume
since opal_unlock_from_suspend requires the exisiting opal_dev state
information saved in the 'unlk_list'.
Something like this instead:
---
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ddc51ad..8fa6be9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work)
if (result)
goto out;
- if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
- dev->ctrl.opal_dev =
- init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP)
+ if (was_suspend && dev->ctrl.opal_dev)
+ opal_unlock_from_suspend(dev->ctrl.opal_dev);
+ else if (!dev->ctrl.opal_dev)
+ dev->ctrl.opal_dev =
+ init_opal_dev(&dev->ctrl, &nvme_sec_submit);
+ } else {
+ kfree(dev->ctrl.opal_dev);
+ dev->ctrl.opal_dev = NULL;
}
- if (was_suspend)
- opal_unlock_from_suspend(dev->ctrl.opal_dev);
result = nvme_setup_io_queues(dev);
if (result)
--
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: OPAL fixups
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 15:27 ` Keith Busch
-1 siblings, 0 replies; 24+ messages in thread
From: Keith Busch @ 2017-02-17 15:27 UTC (permalink / raw)
To: Christoph Hellwig
Cc: scott.bauer, jonathan.derrick, axboe, linux-block, linux-nvme
On Fri, Feb 17, 2017 at 01:59:37PM +0100, Christoph Hellwig wrote:
> Hi all,
>
> this contains a few more OPAL-related fixups. It tones down warnings a bit,
> allocates the OPAL-ѕpecific data structure in a separate dynamic allocation,
> checks for support of Security Send/Receive in NVMe before using them,
> and makes sure we re-discovery the security capabilities after each reset.
Patches 1-3 look good to me.
^ permalink raw reply [flat|nested] 24+ messages in thread
* OPAL fixups
@ 2017-02-17 15:27 ` Keith Busch
0 siblings, 0 replies; 24+ messages in thread
From: Keith Busch @ 2017-02-17 15:27 UTC (permalink / raw)
On Fri, Feb 17, 2017@01:59:37PM +0100, Christoph Hellwig wrote:
> Hi all,
>
> this contains a few more OPAL-related fixups. It tones down warnings a bit,
> allocates the OPAL-?pecific data structure in a separate dynamic allocation,
> checks for support of Security Send/Receive in NVMe before using them,
> and makes sure we re-discovery the security capabilities after each reset.
Patches 1-3 look good to me.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset
2017-02-17 15:26 ` Keith Busch
@ 2017-02-17 15:41 ` Scott Bauer
-1 siblings, 0 replies; 24+ messages in thread
From: Scott Bauer @ 2017-02-17 15:41 UTC (permalink / raw)
To: Keith Busch
Cc: Christoph Hellwig, jonathan.derrick, axboe, linux-block, linux-nvme
On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote:
> On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote:
> > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> > if (result)
> > goto out;
> >
> > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> > + kfree(dev->ctrl.opal_dev);
> > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> > dev->ctrl.opal_dev =
> > init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> > }
>
> A couple things.
>
> This has a use-after-free in opal_unlock_from_suspend if the nvme
> device had an opal_dev before, but no longer support the capability
> after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
>
> But we don't want to unconditionally free it anyway during resume
> since opal_unlock_from_suspend requires the exisiting opal_dev state
> information saved in the 'unlk_list'.
>
> Something like this instead:
>
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index ddc51ad..8fa6be9 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work)
> if (result)
> goto out;
>
> - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> - dev->ctrl.opal_dev =
> - init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP)
> + if (was_suspend && dev->ctrl.opal_dev)
> + opal_unlock_from_suspend(dev->ctrl.opal_dev);
> + else if (!dev->ctrl.opal_dev)
> + dev->ctrl.opal_dev =
> + init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> + } else {
> + kfree(dev->ctrl.opal_dev);
> + dev->ctrl.opal_dev = NULL;
Keith's comments made me realize something even deeper as well. Assuming the firmware
changed and we no longer support security commands we need to free the opal_dev structure
like we're doing but there is a possiblity that there were saved ranges in the structure
that we need to free as well. If the user had previously told the kernel to unlock 5
ranges coming out of a suspend there are 5 structures we need to free inside the opal_dev
before we free the opal dev. We'll need to re-introduce free_opal_dev() in the opal code
like we had a while back.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
@ 2017-02-17 15:41 ` Scott Bauer
0 siblings, 0 replies; 24+ messages in thread
From: Scott Bauer @ 2017-02-17 15:41 UTC (permalink / raw)
On Fri, Feb 17, 2017@10:26:51AM -0500, Keith Busch wrote:
> On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote:
> > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> > if (result)
> > goto out;
> >
> > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> > + kfree(dev->ctrl.opal_dev);
> > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> > dev->ctrl.opal_dev =
> > init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> > }
>
> A couple things.
>
> This has a use-after-free in opal_unlock_from_suspend if the nvme
> device had an opal_dev before, but no longer support the capability
> after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
>
> But we don't want to unconditionally free it anyway during resume
> since opal_unlock_from_suspend requires the exisiting opal_dev state
> information saved in the 'unlk_list'.
>
> Something like this instead:
>
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index ddc51ad..8fa6be9 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work)
> if (result)
> goto out;
>
> - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> - dev->ctrl.opal_dev =
> - init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP)
> + if (was_suspend && dev->ctrl.opal_dev)
> + opal_unlock_from_suspend(dev->ctrl.opal_dev);
> + else if (!dev->ctrl.opal_dev)
> + dev->ctrl.opal_dev =
> + init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> + } else {
> + kfree(dev->ctrl.opal_dev);
> + dev->ctrl.opal_dev = NULL;
Keith's comments made me realize something even deeper as well. Assuming the firmware
changed and we no longer support security commands we need to free the opal_dev structure
like we're doing but there is a possiblity that there were saved ranges in the structure
that we need to free as well. If the user had previously told the kernel to unlock 5
ranges coming out of a suspend there are 5 structures we need to free inside the opal_dev
before we free the opal dev. We'll need to re-introduce free_opal_dev() in the opal code
like we had a while back.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset
2017-02-17 17:01 ` Christoph Hellwig
@ 2017-02-17 16:55 ` Scott Bauer
-1 siblings, 0 replies; 24+ messages in thread
From: Scott Bauer @ 2017-02-17 16:55 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, jonathan.derrick, axboe, linux-block, linux-nvme
On Fri, Feb 17, 2017 at 06:01:28PM +0100, Christoph Hellwig wrote:
> On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote:
> > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote:
> > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> > > if (result)
> > > goto out;
> > >
> > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> > > + kfree(dev->ctrl.opal_dev);
> > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> > > dev->ctrl.opal_dev =
> > > init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> > > }
> >
> > A couple things.
> >
> > This has a use-after-free in opal_unlock_from_suspend if the nvme
> > device had an opal_dev before, but no longer support the capability
> > after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
> >
> > But we don't want to unconditionally free it anyway during resume
> > since opal_unlock_from_suspend requires the exisiting opal_dev state
> > information saved in the 'unlk_list'.
> >
> > Something like this instead:
>
> Yes, that looks fine to me. We'll probably also need the additional
> fixup Scott pointed out.
I'm working on it now. Do you want a diff like Keith did or a separate patch?
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
@ 2017-02-17 16:55 ` Scott Bauer
0 siblings, 0 replies; 24+ messages in thread
From: Scott Bauer @ 2017-02-17 16:55 UTC (permalink / raw)
On Fri, Feb 17, 2017@06:01:28PM +0100, Christoph Hellwig wrote:
> On Fri, Feb 17, 2017@10:26:51AM -0500, Keith Busch wrote:
> > On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote:
> > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> > > if (result)
> > > goto out;
> > >
> > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> > > + kfree(dev->ctrl.opal_dev);
> > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> > > dev->ctrl.opal_dev =
> > > init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> > > }
> >
> > A couple things.
> >
> > This has a use-after-free in opal_unlock_from_suspend if the nvme
> > device had an opal_dev before, but no longer support the capability
> > after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
> >
> > But we don't want to unconditionally free it anyway during resume
> > since opal_unlock_from_suspend requires the exisiting opal_dev state
> > information saved in the 'unlk_list'.
> >
> > Something like this instead:
>
> Yes, that looks fine to me. We'll probably also need the additional
> fixup Scott pointed out.
I'm working on it now. Do you want a diff like Keith did or a separate patch?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset
2017-02-17 15:26 ` Keith Busch
@ 2017-02-17 17:01 ` Christoph Hellwig
-1 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 17:01 UTC (permalink / raw)
To: Keith Busch
Cc: Christoph Hellwig, scott.bauer, jonathan.derrick, axboe,
linux-block, linux-nvme
On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote:
> On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote:
> > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> > if (result)
> > goto out;
> >
> > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> > + kfree(dev->ctrl.opal_dev);
> > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> > dev->ctrl.opal_dev =
> > init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> > }
>
> A couple things.
>
> This has a use-after-free in opal_unlock_from_suspend if the nvme
> device had an opal_dev before, but no longer support the capability
> after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
>
> But we don't want to unconditionally free it anyway during resume
> since opal_unlock_from_suspend requires the exisiting opal_dev state
> information saved in the 'unlk_list'.
>
> Something like this instead:
Yes, that looks fine to me. We'll probably also need the additional
fixup Scott pointed out.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
@ 2017-02-17 17:01 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 17:01 UTC (permalink / raw)
On Fri, Feb 17, 2017@10:26:51AM -0500, Keith Busch wrote:
> On Fri, Feb 17, 2017@01:59:41PM +0100, Christoph Hellwig wrote:
> > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work)
> > if (result)
> > goto out;
> >
> > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) {
> > + kfree(dev->ctrl.opal_dev);
> > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) {
> > dev->ctrl.opal_dev =
> > init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> > }
>
> A couple things.
>
> This has a use-after-free in opal_unlock_from_suspend if the nvme
> device had an opal_dev before, but no longer support the capability
> after resume. So you'd want to set ctrl.opal_dev to NULL after the free.
>
> But we don't want to unconditionally free it anyway during resume
> since opal_unlock_from_suspend requires the exisiting opal_dev state
> information saved in the 'unlk_list'.
>
> Something like this instead:
Yes, that looks fine to me. We'll probably also need the additional
fixup Scott pointed out.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] nvme: re-check security protocol support after reset
2017-02-17 16:55 ` Scott Bauer
@ 2017-02-17 17:16 ` Christoph Hellwig
-1 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 17:16 UTC (permalink / raw)
To: Scott Bauer
Cc: Christoph Hellwig, Keith Busch, jonathan.derrick, axboe,
linux-block, linux-nvme
On Fri, Feb 17, 2017 at 09:55:51AM -0700, Scott Bauer wrote:
> I'm working on it now. Do you want a diff like Keith did or a separate patch?
Please make it a proper patch that applies on top of my patches 1-3 (where
3 really is yours anyway). I'll respin 4 with the updates from Keith on
top of 1-3 + your patch then.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] nvme: re-check security protocol support after reset
@ 2017-02-17 17:16 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2017-02-17 17:16 UTC (permalink / raw)
On Fri, Feb 17, 2017@09:55:51AM -0700, Scott Bauer wrote:
> I'm working on it now. Do you want a diff like Keith did or a separate patch?
Please make it a proper patch that applies on top of my patches 1-3 (where
3 really is yours anyway). I'll respin 4 with the updates from Keith on
top of 1-3 + your patch then.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: OPAL fixups
2017-02-17 12:59 ` Christoph Hellwig
@ 2017-02-17 19:42 ` Jens Axboe
-1 siblings, 0 replies; 24+ messages in thread
From: Jens Axboe @ 2017-02-17 19:42 UTC (permalink / raw)
To: Christoph Hellwig, scott.bauer, keith.busch, jonathan.derrick
Cc: linux-block, linux-nvme
On 02/17/2017 05:59 AM, Christoph Hellwig wrote:
> Hi all,
>
> this contains a few more OPAL-related fixups. It tones down warnings a bit,
> allocates the OPAL-ѕpecific data structure in a separate dynamic allocation,
> checks for support of Security Send/Receive in NVMe before using them,
> and makes sure we re-discovery the security capabilities after each reset.
Applied 1-3, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 24+ messages in thread
* OPAL fixups
@ 2017-02-17 19:42 ` Jens Axboe
0 siblings, 0 replies; 24+ messages in thread
From: Jens Axboe @ 2017-02-17 19:42 UTC (permalink / raw)
On 02/17/2017 05:59 AM, Christoph Hellwig wrote:
> Hi all,
>
> this contains a few more OPAL-related fixups. It tones down warnings a bit,
> allocates the OPAL-?pecific data structure in a separate dynamic allocation,
> checks for support of Security Send/Receive in NVMe before using them,
> and makes sure we re-discovery the security capabilities after each reset.
Applied 1-3, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-02-17 19:42 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-17 12:59 OPAL fixups Christoph Hellwig
2017-02-17 12:59 ` Christoph Hellwig
2017-02-17 12:59 ` [PATCH 1/4] block/sed-opal: tone down not supported warnings Christoph Hellwig
2017-02-17 12:59 ` Christoph Hellwig
2017-02-17 12:59 ` [PATCH 2/4] block/sed-opal: allocate struct opal_dev dynamically Christoph Hellwig
2017-02-17 12:59 ` Christoph Hellwig
2017-02-17 12:59 ` [PATCH 3/4] nvme: Check for Security send/recv support before issuing commands Christoph Hellwig
2017-02-17 12:59 ` Christoph Hellwig
2017-02-17 12:59 ` [PATCH 4/4] nvme: re-check security protocol support after reset Christoph Hellwig
2017-02-17 12:59 ` Christoph Hellwig
2017-02-17 15:26 ` Keith Busch
2017-02-17 15:26 ` Keith Busch
2017-02-17 15:41 ` Scott Bauer
2017-02-17 15:41 ` Scott Bauer
2017-02-17 17:01 ` Christoph Hellwig
2017-02-17 17:01 ` Christoph Hellwig
2017-02-17 16:55 ` Scott Bauer
2017-02-17 16:55 ` Scott Bauer
2017-02-17 17:16 ` Christoph Hellwig
2017-02-17 17:16 ` Christoph Hellwig
2017-02-17 15:27 ` OPAL fixups Keith Busch
2017-02-17 15:27 ` Keith Busch
2017-02-17 19:42 ` Jens Axboe
2017-02-17 19:42 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.