Re: [PATCH v2 2/4] block: Add Sed-opal library
On Fri, Dec 09, 2016 at 10:45:30AM -0700, Scott Bauer wrote: > Now, with this in mind, it sort of makes sense to move this from > block/ back into lib/ and interface with the character dev. Instead > of passing around block_devices, we can pass around struct file *'s. > Even the character device is always backed by the queues, and I'd really not prefer to have a struct file here - that's not useful at all for in-kernel users. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] block: Add Sed-opal library
On Thu, Dec 01, 2016 at 01:22:39PM -0500, Keith Busch wrote: > On Thu, Dec 01, 2016 at 10:53:43AM -0700, Scott Bauer wrote: > > > Maybe. I need to look at the TCG spec again (oh my good, what a fucking > > > mess), but if I remember the context if it is the whole nvme controller > > > and not just a namespace, so a block_device might be the wrong context. > > > Then again we can always go from the block_device to the controller > > > fairly easily. So instead of adding the security operation to the > > > block_device_operations which we don't really need for now maybe we > > > should add a security_conext to the block device so that we can avoid > > > all the lookup code? > > > > I spent some time this morning reading through the numerous specs/documents, > > with a lot of coffee. > > > > Specifically in: > > https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_02_Revision_1_00_20111230.pdf > > > > 5.5.2 > > Namespace > > > > A target that has multiple Namespaces MAY have multiple TPers. Each TPer > > SHALL be associated with a different Namespace. Every Namespace on a device > > is not required to have a TPer, but Namespaces that support the TCG Core > > specification commands and functionality SHALL have a TPer. A TPer SHALL > > only > > be associated with exactly one Namespace. A Namespace MAY have no TPer. > > > > From reading that it seems we will probably have to keep it at the block > > layer, > > since its possible to have a valid "Locking range 1" on n1 and a "Locking > > range 1" > > on n2. > > Thanks for tracking that down! Specifically for NVMe, security > send/recieve requires NSID, so it is a little more difficult to get to > that if we're not using the abstracton that contains the namespace. So turns out that version is old and it has since changed: https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf (section 5.5) So in this document Cristoph is right. There is a single TPER for the entire device. For devices with multiple namespaces, there will be a single global locking range. That single locking range covers the entire LBA range. Other locking ranges aren't allowed. Now, for a drive with one namespace There is a global LR and it MAY be allowed to have other user locking ranges as well. Now, with this in mind, it sort of makes sense to move this from block/ back into lib/ and interface with the character dev. Instead of passing around block_devices, we can pass around struct file *'s. Does anyone have and qualms/comments/anecdotes before I move everything around? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] block: Add Sed-opal library
On Thu, Dec 01, 2016 at 10:53:43AM -0700, Scott Bauer wrote: > > Maybe. I need to look at the TCG spec again (oh my good, what a fucking > > mess), but if I remember the context if it is the whole nvme controller > > and not just a namespace, so a block_device might be the wrong context. > > Then again we can always go from the block_device to the controller > > fairly easily. So instead of adding the security operation to the > > block_device_operations which we don't really need for now maybe we > > should add a security_conext to the block device so that we can avoid > > all the lookup code? > > I spent some time this morning reading through the numerous specs/documents, > with a lot of coffee. > > Specifically in: > https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_02_Revision_1_00_20111230.pdf > > 5.5.2 > Namespace > > A target that has multiple Namespaces MAY have multiple TPers. Each TPer > SHALL be associated with a different Namespace. Every Namespace on a device > is not required to have a TPer, but Namespaces that support the TCG Core > specification commands and functionality SHALL have a TPer. A TPer SHALL only > be associated with exactly one Namespace. A Namespace MAY have no TPer. > > From reading that it seems we will probably have to keep it at the block > layer, > since its possible to have a valid "Locking range 1" on n1 and a "Locking > range 1" > on n2. Thanks for tracking that down! Specifically for NVMe, security send/recieve requires NSID, so it is a little more difficult to get to that if we're not using the abstracton that contains the namespace. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] block: Add Sed-opal library
On Thu, Dec 01, 2016 at 02:04:56AM -0800, Christoph Hellwig wrote: > On Wed, Nov 30, 2016 at 07:50:07PM -0500, Keith Busch wrote: > > I think we should get rid of the "majmin" stuff > > Absolutely agreed. > > > > > and directly use > > block_device. Then if we add the security send/receive operations to the > > block_device_operations, that will simplify chaining the security request > > to the driver without needing to thread the driver's requested callback > > and data the way you have to here since all the necessary information > > is encapsulated in the block_device. > > Maybe. I need to look at the TCG spec again (oh my good, what a fucking > mess), but if I remember the context if it is the whole nvme controller > and not just a namespace, so a block_device might be the wrong context. > Then again we can always go from the block_device to the controller > fairly easily. So instead of adding the security operation to the > block_device_operations which we don't really need for now maybe we > should add a security_conext to the block device so that we can avoid > all the lookup code? I spent some time this morning reading through the numerous specs/documents, with a lot of coffee. Specifically in: https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_02_Revision_1_00_20111230.pdf 5.5.2 Namespace A target that has multiple Namespaces MAY have multiple TPers. Each TPer SHALL be associated with a different Namespace. Every Namespace on a device is not required to have a TPer, but Namespaces that support the TCG Core specification commands and functionality SHALL have a TPer. A TPer SHALL only be associated with exactly one Namespace. A Namespace MAY have no TPer. >From reading that it seems we will probably have to keep it at the block layer, since its possible to have a valid "Locking range 1" on n1 and a "Locking range 1" on n2. [snip] -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] block: Add Sed-opal library
On Wed, Nov 30, 2016 at 07:50:07PM -0500, Keith Busch wrote: > I think we should get rid of the "majmin" stuff Absolutely agreed. > > and directly use > block_device. Then if we add the security send/receive operations to the > block_device_operations, that will simplify chaining the security request > to the driver without needing to thread the driver's requested callback > and data the way you have to here since all the necessary information > is encapsulated in the block_device. Maybe. I need to look at the TCG spec again (oh my good, what a fucking mess), but if I remember the context if it is the whole nvme controller and not just a namespace, so a block_device might be the wrong context. Then again we can always go from the block_device to the controller fairly easily. So instead of adding the security operation to the block_device_operations which we don't really need for now maybe we should add a security_conext to the block device so that we can avoid all the lookup code? > We shouldn't need to be allocating an 'opal_dev' for every range. The > range-specific parts should be in a different structure that the opal_dev > can have a list of. That will simpify the unlock from suspend a bit. Agreed. > I can appreciate how compact this is, but this is a little harder to > read IMO, and it works only because you were so careful in setting up > the array. I think expanding the ioctl into a switch will be easier to > follow, and has a more tolerent coding convention for future additions. Agreed. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] block: Add Sed-opal library
On Tue, Nov 29, 2016 at 02:52:00PM -0700, Scott Bauer wrote:
> +struct opal_dev {
> + dev_t majmin;
> + sed_sec_submit *submit_fn;
> + void *submit_data;
> + struct opal_lock_unlock lkul;
> + const opal_step *funcs;
> + void **func_data;
> + bool resume_from_suspend;
> + struct opal_suspend_unlk *resume_data;
> + size_t num_func_data;
> + atomic_t in_use;
> + sector_t start;
> + sector_t length;
> + u8 lr;
> + u8 key_type;
> + u8 key_name[OPAL_KEY_MAX];
> + size_t key_name_len;
> + u8 key[OPAL_KEY_MAX];
> + size_t key_len;
> + u16 comID;
> + u32 HSN;
> + u32 TSN;
> + u64 align;
> + u64 lowest_lba;
> + struct list_head node;
> + char disk_name[DISK_NAME_LEN];
> + int state;
> +
> + struct opal_cmd cmd;
> + struct parsed_resp parsed;
> +
> + size_t prev_d_len;
> + void *prev_data;
> +
> + opal_step error_cb;
> + void *error_cb_data;
> +};
I think this structure could use some kernel-doc comments explaining what
all these fields are for. Some of them don't appear to be used anywhere
in the code, but it'd help to know what the fields are supposed to be for.
I think we should get rid of the "majmin" stuff and directly use
block_device. Then if we add the security send/receive operations to the
block_device_operations, that will simplify chaining the security request
to the driver without needing to thread the driver's requested callback
and data the way you have to here since all the necessary information
is encapsulated in the block_device.
We shouldn't need to be allocating an 'opal_dev' for every range. The
range-specific parts should be in a different structure that the opal_dev
can have a list of. That will simpify the unlock from suspend a bit.
> +/*
> + * N = number of format specifiers (1-999) to be replicated
> + * c = u8
> + * u = u64
> + * s = bytestring, length
> + *
> + * ret = test_and_add_token_va(cmd, "c",
> + * u8_val1);
> + *
> + * ret = test_and_add_token_va(cmd, "2c2u",
> + * u8_val1, u8_val2, u64_val1, u64_val2);
> + *
> + * ret = test_and_add_token_va(cmd, "3s",
> + * bytestring1, length1,
> + * bytestring2, length2,
> + * bytestring3, length3);
> + */
> +static int test_and_add_token_va(struct opal_cmd *cmd,
> + const char *fmt, ...)
This custom formated string parser looks too complicated to me. I'll try
propose something simpler once I fully understand all the parts to this.
> +#define CMD_TO_FN_INDX(cmd) \
> + (cmd) - IOC_SED_SAVE
> +
> +int (*sed_fn[])(struct block_device *bdev, struct sed_key *key,
> + void *sbmt_data, sed_sec_submit *submit_fn) =
> +{
> + sed_save,
> + sed_lock_unlock,
> + sed_take_ownership,
> + sed_activate_lsp,
> + sed_set_pw,
> + sed_activate_user,
> + sed_reverttper,
> + sed_setup_locking_range,
> + sed_adduser_to_lr,
> + sed_do_mbr,
> + sed_erase_lr,
> + sed_secure_erase_lr
> +};
> +
> +/* The sbmt_ctrl_data is a opaque pointer to some structure which will be
> used
> + * by the submit_fn to properly submit the opal command to the controller.
> + * The submit_fn must be a blocking call.
> + */
> +int blkdev_sed_ioctl(struct block_device *bdev, fmode_t fmode, unsigned int
> cmd,
> + unsigned long arg, void *sbmt_ctrl_data,
> + sed_sec_submit *submit_fn)
> +{
> + struct sed_key key;
> +
> + /* Caller should do this but since we're going to use cmd as an index
> + * lets 'trust but verify'.
> + */
> + if (!is_sed_ioctl(cmd))
> + return -EINVAL;
> + if (copy_from_user(, (void __user *)arg, sizeof(key)))
> + return -EFAULT;
> + return sed_fn[CMD_TO_FN_INDX(cmd)](bdev, , sbmt_ctrl_data,
> submit_fn);
I can appreciate how compact this is, but this is a little harder to
read IMO, and it works only because you were so careful in setting up
the array. I think expanding the ioctl into a switch will be easier to
follow, and has a more tolerent coding convention for future additions.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] block: Add Sed-opal library
es1;4205;0cOn Wed, Nov 30, 2016 at 01:13:57PM -0500, Keith Busch wrote: > On Tue, Nov 29, 2016 at 02:52:00PM -0700, Scott Bauer wrote: > > + dev = get_or_create_opal_dev(bdev, key->opal_act.key.lr, true); > > + if (!dev) > > + return -ENOMEM; > > The alloc_opal_dev from this call returns ERR_PTR values on error, so > the check should be: > > if (IS_ERR_OR_NULL(dev)) > return PTR_ERR(dev); Nice catch, I'll go double check the rest of the return values for this scenario. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
