diff mbox

opal: Use empty structure when not defined

Message ID 20170216075812.GA7662@infradead.org (mailing list archive)
State New, archived
Headers show

Commit Message

Christoph Hellwig Feb. 16, 2017, 7:58 a.m. UTC 
I'd rather prefer to make the structure separately allocated as
discussed before.  Scott, can you test the patch below?  I'm not near
my devices I could test on.

---
From b2cda0c7ec5c0ec66582655751838f519cfa1706 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Thu, 16 Feb 2017 08:49:56 +0100
Subject: block/sed-opal: make struct opal_dev private

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

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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(-)

Comments

Jon Derrick Feb. 16, 2017, 5:18 p.m. UTC | #1 
It looks good to me at first glance but I can't apply it. What tree are
you on?

On 02/16/2017 12:58 AM, Christoph Hellwig wrote:
> I'd rather prefer to make the structure separately allocated as
> discussed before.  Scott, can you test the patch below?  I'm not near
> my devices I could test on.
> 
> ---
> From b2cda0c7ec5c0ec66582655751838f519cfa1706 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 16 Feb 2017 08:49:56 +0100
> Subject: block/sed-opal: make struct opal_dev private
> 
> This moves 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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 bf1406e5159b..bdab4dfcbafd 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_warn("Opal is not supported on this device\n");
> -	opal_dev->initialized = true;
> +		kfree(dev);
> +		return NULL;
> +	}
> +	return dev;
>  }
>  EXPORT_SYMBOL(init_opal_dev);
>  
> @@ -2350,6 +2429,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
>  
>  	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 d9f49036819c..84eb86cc4a45 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, arg);
> +			return sed_ioctl(ns->ctrl->opal_dev, cmd, arg);
>  		return -ENOTTY;
>  	}
>  }
> @@ -1084,18 +1084,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 af1a85eae193..da39002285d0 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, unsigned long 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 */
>
Christoph Hellwig Feb. 16, 2017, 5:21 p.m. UTC | #2 
On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> It looks good to me at first glance but I can't apply it. What tree are
> you on?

It's against Jens' for-next tree (actually from two days ago, but
nothing in this area should have changed since then)
Scott Bauer Feb. 16, 2017, 5:37 p.m. UTC | #3 
On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> It looks good to me at first glance but I can't apply it. What tree are
> you on?
> 
> On 02/16/2017 12:58 AM, Christoph Hellwig wrote:
> > I'd rather prefer to make the structure separately allocated as
> > discussed before.  Scott, can you test the patch below?  I'm not near
> > my devices I could test on.
> > 

He doesn't have the most recent changes with the uapi IOW fixes. I got it applied
and tested and it works. Going to review it more aggressively now. We had something
very similar in one of the previous patches before we switched off of using a sed_context
struct.

If you do want to apply and test I did the following:
1) Pull Jens' for-next
2) reset fb2a77e4a25ef63ff5b51b3bd53027077b402b0d --hard
3) apply his patch
4) git pull https://kernel.googlesource.com/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
5) resolve the 2 small conflicts in core.c and sed-opal.h
   in sed-opal.h take his init_opal_dev change and the newer sed_ioctl
   in core.c take his change but change arg to be (void __user *) arg);
Scott Bauer Feb. 16, 2017, 5:39 p.m. UTC | #4 
On Thu, Feb 16, 2017 at 10:37:55AM -0700, Scott Bauer wrote:
> On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> > It looks good to me at first glance but I can't apply it. What tree are
> > you on?
> > 
> > On 02/16/2017 12:58 AM, Christoph Hellwig wrote:
> > > I'd rather prefer to make the structure separately allocated as
> > > discussed before.  Scott, can you test the patch below?  I'm not near
> > > my devices I could test on.
> > > 
> 
> He doesn't have the most recent changes with the uapi IOW fixes. I got it applied
> and tested and it works. Going to review it more aggressively now. We had something
> very similar in one of the previous patches before we switched off of using a sed_context
> struct.
> 
> If you do want to apply and test I did the following:
> 1) Pull Jens' for-next
> 2) reset fb2a77e4a25ef63ff5b51b3bd53027077b402b0d --hard
> 3) apply his patch
> 4) git pull https://kernel.googlesource.com/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
> 5) resolve the 2 small conflicts in core.c and sed-opal.h
>    in sed-opal.h take his init_opal_dev change and the newer sed_ioctl
>    in core.c take his change but change arg to be (void __user *) arg);

6) pull the newest commit from https://github.com/ScottyBauer/sed-opal-temp to get the uapi header change
Scott Bauer Feb. 16, 2017, 6:45 p.m. UTC | #5 
On Wed, Feb 15, 2017 at 11:58:12PM -0800, Christoph Hellwig wrote:
> I'd rather prefer to make the structure separately allocated as
> discussed before.  Scott, can you test the patch below?  I'm not near
> my devices I could test on.
> 
> ---
> From b2cda0c7ec5c0ec66582655751838f519cfa1706 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 16 Feb 2017 08:49:56 +0100
> Subject: block/sed-opal: make struct opal_dev private
> 
> This moves 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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 bf1406e5159b..bdab4dfcbafd 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_warn("Opal is not supported on this device\n");
> -	opal_dev->initialized = true;
> +		kfree(dev);
> +		return NULL;

We're going to have to change this check_opal_support to be != 0 instead of < 0.
I tested on a controller that does not have opal enabled and I get a kick back of:

[  112.296675] sed_opal:OPAL: Error on step function: 0 with error 1: Not Authorized
[  112.306242]  nvme1n1: p1 p2 p3

So we return the error 1 out of check_opal_support, and we'll never free the opal_dev.
There isnt any issues with potential crashes or other weird behavior
because we set dev->supported to be false, so if you try and call an ioctl on the
unsuported device you'll get a:

[  149.550024] sed_opal:OPAL: Not supported

but the memory is still there.

Also, since init_opal_dev gets called from reset_work any time we do that we'll
spam the user with that pr_warn, is there a pr_warn_once or something we can use
instead?

I looked at the rest of the pr_warns and there is one in discovery0_end that we'll
want to convert to a pr_warn_once as well:

     if (!single_user)
		pr_warn("Device doesn't support single user mode\n");

Since we use discovery0 to get a comid every command we run on a non SUM device
will have that spammed to their dmesg.


Other than the above it looks fine to me.
Christoph Hellwig Feb. 16, 2017, 8:07 p.m. UTC | #6 
On Thu, Feb 16, 2017 at 11:45:29AM -0700, Scott Bauer wrote:
> > +	if (check_opal_support(dev) < 0) {
> >  		pr_warn("Opal is not supported on this device\n");
> > -	opal_dev->initialized = true;
> > +		kfree(dev);
> > +		return NULL;
> 
> We're going to have to change this check_opal_support to be != 0 instead of < 0.

Yes.  And we should simply turn all these printk into pr_debug anway -
not having OPAL is a prefectly fine condition, no need to spam the log
for it.

And btw, I think we should check for bit 0 in OACS before ever doing
a security send / receive.
Scott Bauer Feb. 16, 2017, 8:52 p.m. UTC | #7 
On Thu, Feb 16, 2017 at 12:07:08PM -0800, Christoph Hellwig wrote:
> On Thu, Feb 16, 2017 at 11:45:29AM -0700, Scott Bauer wrote:
> > > +	if (check_opal_support(dev) < 0) {
> > >  		pr_warn("Opal is not supported on this device\n");
> > > -	opal_dev->initialized = true;
> > > +		kfree(dev);
> > > +		return NULL;
> > 
> > We're going to have to change this check_opal_support to be != 0 instead of < 0.
> 
> Yes.  And we should simply turn all these printk into pr_debug anway -
> not having OPAL is a prefectly fine condition, no need to spam the log
> for it.
> 
> And btw, I think we should check for bit 0 in OACS before ever doing
> a security send / receive.

That sounds reasonable. It can go in the nvme_send_recv function before we
actually attempt to send anything. The code paths fall nicely in that when
we attempt to look for support we have to do a discovery0 which will fail
on the OACS bit and we'll deallocate everything.

I'll spin this up now.
diff mbox

Patch

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 bf1406e5159b..bdab4dfcbafd 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_warn("Opal is not supported on this device\n");
-	opal_dev->initialized = true;
+		kfree(dev);
+		return NULL;
+	}
+	return dev;
 }
 EXPORT_SYMBOL(init_opal_dev);
 
@@ -2350,6 +2429,8 @@  int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
 
 	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 d9f49036819c..84eb86cc4a45 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, arg);
+			return sed_ioctl(ns->ctrl->opal_dev, cmd, arg);
 		return -ENOTTY;
 	}
 }
@@ -1084,18 +1084,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 af1a85eae193..da39002285d0 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, unsigned long 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 */