> +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.
