Re: [PATCH v6 1/9] block: Keyslot Manager for Inline Encryption

[Christoph Hellwig <hch@infradead.org>]

> +struct keyslot_manager {
> +	unsigned int num_slots;
> +	struct keyslot_mgmt_ll_ops ksm_ll_ops;
> +	unsigned int crypto_mode_supported[BLK_ENCRYPTION_MODE_MAX];
> +	void *ll_priv_data;
> +
> +	/* Protects programming and evicting keys from the device */
> +	struct rw_semaphore lock;
> +
> +	/* List of idle slots, with least recently used slot at front */
> +	wait_queue_head_t idle_slots_wait_queue;
> +	struct list_head idle_slots;
> +	spinlock_t idle_slots_lock;
> +
> +	/*
> +	 * Hash table which maps key hashes to keyslots, so that we can find a
> +	 * key's keyslot in O(1) time rather than O(num_slots).  Protected by
> +	 * 'lock'.  A cryptographic hash function is used so that timing attacks
> +	 * can't leak information about the raw keys.
> +	 */
> +	struct hlist_head *slot_hashtable;
> +	unsigned int slot_hashtable_size;
> +
> +	/* Per-keyslot data */
> +	struct keyslot slots[];
> +};

Is there a rationale for making this structure private?  If it was
exposed we could embedd it into the containing structure (although
the slots would need a dynamic allocation), and instead of the
keyslot_manager_private helper, the caller could simply use
container_of.

> +struct keyslot_manager *keyslot_manager_create(unsigned int num_slots,
> +	const struct keyslot_mgmt_ll_ops *ksm_ll_ops,
> +	const unsigned int crypto_mode_supported[BLK_ENCRYPTION_MODE_MAX],
> +	void *ll_priv_data)

.. and then the caller could simply set the ops and the supported modes
array directly in the structure, simplifying the interface even further.

> +static int find_keyslot(struct keyslot_manager *ksm,
> +			const struct blk_crypto_key *key)
> +{
> +	const struct hlist_head *head = hash_bucket_for_key(ksm, key);
> +	const struct keyslot *slotp;
> +
> +	hlist_for_each_entry(slotp, head, hash_node) {
> +		if (slotp->key.hash == key->hash &&
> +		    slotp->key.crypto_mode == key->crypto_mode &&
> +		    slotp->key.data_unit_size == key->data_unit_size &&
> +		    !crypto_memneq(slotp->key.raw, key->raw, key->size))
> +			return slotp - ksm->slots;
> +	}
> +	return -ENOKEY;
> +}

I'd return the actual slot pointer here, as that seems the more natural
fit.  Then factor the pointer arithmetics into a little helper to make
it obvious for those few places that need the actual slot number.

Also can you add proper subsystem prefix to the various symbol names?

> +void keyslot_manager_get_slot(struct keyslot_manager *ksm, unsigned int slot)
> +{
> +	if (WARN_ON(slot >= ksm->num_slots))
> +		return;
> +
> +	WARN_ON(atomic_inc_return(&ksm->slots[slot].slot_refs) < 2);
> +}
> +
> +/**
> + * keyslot_manager_put_slot() - Release a reference to a slot
> + * @ksm: The keyslot manager to release the reference from.
> + * @slot: The slot to release the reference from.
> + *
> + * Context: Any context.
> + */
> +void keyslot_manager_put_slot(struct keyslot_manager *ksm, unsigned int slot)
> +{
> +	unsigned long flags;
> +
> +	if (WARN_ON(slot >= ksm->num_slots))
> +		return;
> +
> +	if (atomic_dec_and_lock_irqsave(&ksm->slots[slot].slot_refs,
> +					&ksm->idle_slots_lock, flags)) {
> +		list_add_tail(&ksm->slots[slot].idle_slot_node,
> +			      &ksm->idle_slots);
> +		spin_unlock_irqrestore(&ksm->idle_slots_lock, flags);
> +		wake_up(&ksm->idle_slots_wait_queue);
> +	}
> +}

How about passing the bio_crypt_ctx structure instead of the not very
nicely typed slot index?  Also if we merge the files both these helpers
should probably just go away and me merged into the 1 or 2 callers that
exist.

> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +
> +#define BLK_CRYPTO_MAX_KEY_SIZE		64
> +
> +/**
> + * struct blk_crypto_key - an inline encryption key
> + * @crypto_mode: encryption algorithm this key is for
> + * @data_unit_size: the data unit size for all encryption/decryptions with this
> + *	key.  This is the size in bytes of each individual plaintext and
> + *	ciphertext.  This is always a power of 2.  It might be e.g. the
> + *	filesystem block size or the disk sector size.
> + * @data_unit_size_bits: log2 of data_unit_size
> + * @size: size of this key in bytes (determined by @crypto_mode)
> + * @hash: hash of this key, for keyslot manager use only
> + * @raw: the raw bytes of this key.  Only the first @size bytes are used.
> + *
> + * A blk_crypto_key is immutable once created, and many bios can reference it at
> + * the same time.  It must not be freed until all bios using it have completed.
> + */
> +struct blk_crypto_key {
> +	enum blk_crypto_mode_num crypto_mode;
> +	unsigned int data_unit_size;
> +	unsigned int data_unit_size_bits;
> +	unsigned int size;
> +	unsigned int hash;
> +	u8 raw[BLK_CRYPTO_MAX_KEY_SIZE];
> +};
> +
> +#endif /* CONFIG_BLK_INLINE_ENCRYPTION */
> +#endif /* CONFIG_BLOCK */

I don't think we need any ifdefs around these declarations.