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