Re: [PATCH v8 01/11] block: Keyslot Manager for Inline Encryption

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Mar 12, 2020 at 01:02:43AM -0700, Satya Tangirala wrote:
> +/**
> + * blk_ksm_init() - Initialize a keyslot manager
> + * @ksm: The keyslot_manager to initialize.
> + * @dev: Device for runtime power management (NULL if none)
> + * @num_slots: The number of key slots to manage.
> + *
> + * Allocate memory for keyslots and initialize a keyslot manager. Called by
> + * e.g. storage drivers to set up a keyslot manager in their request_queue.
> + *
> + * Return: 0 on success, or else a negative error code.
> + */
> +int blk_ksm_init(struct keyslot_manager *ksm, struct device *dev,
> +		 unsigned int num_slots)
> +{
> +	unsigned int slot;
> +	unsigned int i;
> +
> +	memset(ksm, 0, sizeof(*ksm));
> +
> +	if (num_slots == 0)
> +		return -EINVAL;
> +
> +	ksm->slots = kvcalloc(num_slots, sizeof(ksm->slots[0]), GFP_KERNEL);
> +	if (!ksm->slots)
> +		return -ENOMEM;
> +
> +	ksm->num_slots = num_slots;
> +	ksm->dev = dev;

Seems that now that keyslot_manager::dev is an unconditional field and callers
initialize other fields of the keyslot_manager, the 'dev' parameter should be
removed from blk_ksm_init() and the caller should be responsible for setting
'dev' if they need runtime power management.

> +/**
> + * blk_ksm_get_slot_for_key() - Program a key into a keyslot.
> + * @ksm: The keyslot manager to program the key into.
> + * @key: Pointer to the key object to program, including the raw key, crypto
> + *	 mode, and data unit size.
> + * @keyslot: A pointer to return the pointer of the allocated keyslot.
> + *
> + * Get a keyslot that's been programmed with the specified key.  If one already
> + * exists, return it with incremented refcount.  Otherwise, wait for a keyslot
> + * to become idle and program it.
> + *
> + * Context: Process context. Takes and releases ksm->lock.
> + * Return: BLK_STATUS_OK on success (and keyslot is set to the pointer of the
> + *	   allocated keyslot), and BLK_STATUS_IOERR otherwise (and keyslot is
> + *	   set to NULL).
> + */

There can be other errors besides BLK_STATUS_IOERR returned.

> +/**
> + * blk_ksm_put_slot() - Release a reference to a slot
> + * @slot: The keyslot to release the reference of.
> + *
> + * Context: Any context.
> + */
> +void blk_ksm_put_slot(struct blk_ksm_keyslot *slot)
> +{
> +	struct keyslot_manager *ksm = slot->ksm;
> +	unsigned long flags;
> +
> +	if (!slot)
> +		return;
> +
> +	if (atomic_dec_and_lock_irqsave(&slot->slot_refs,
> +					&ksm->idle_slots_lock, flags)) {
> +		list_add_tail(&slot->idle_slot_node,
> +			      &ksm->idle_slots);

Nit: the arguments to list_add_tail() fit on one line

> diff --git a/include/linux/keyslot-manager.h b/include/linux/keyslot-manager.h
> new file mode 100644
> index 000000000000..7f88ed02faee
> --- /dev/null
> +++ b/include/linux/keyslot-manager.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2019 Google LLC
> + */
> +
> +#ifndef __LINUX_KEYSLOT_MANAGER_H
> +#define __LINUX_KEYSLOT_MANAGER_H
> +
> +#include <linux/bio.h>
> +#include <linux/blk-crypto.h>
> +
> +struct keyslot_manager;
> +
> +/**
> + * struct keyslot_mgmt_ll_ops - functions to manage keyslots in hardware
> + * @keyslot_program:	Program the specified key into the specified slot in the
> + *			inline encryption hardware.
> + * @keyslot_evict:	Evict key from the specified keyslot in the hardware.
> + *			The key is provided so that e.g. dm layers can evict
> + *			keys from the devices that they map over.
> + *			Returns 0 on success, -errno otherwise.
> + *
> + * This structure should be provided by storage device drivers when they set up
> + * a keyslot manager - this structure holds the function ptrs that the keyslot
> + * manager will use to manipulate keyslots in the hardware.
> + */
> +struct keyslot_mgmt_ll_ops {
> +	blk_status_t (*keyslot_program)(struct keyslot_manager *ksm,
> +					const struct blk_crypto_key *key,
> +					unsigned int slot);
> +	int (*keyslot_evict)(struct keyslot_manager *ksm,
> +			     const struct blk_crypto_key *key,
> +			     unsigned int slot);
> +};

As I mentioned on one of the UFS patches, I'm not sure it's better to make
keyslot_program return blk_status_t rather than errno as it did before.
blk_ksm_get_slot_for_key() could just do the translation to blk_status_t.

> +struct keyslot_manager {

Now that the "blk_" prefix has been added to all the functions, perhaps the
actual struct should be renamed to "blk_keyslot_manager" too?
And likewise "blk_keyslot_mgmt_ll_ops".