Re: [PATCH v6 1/9] block: Keyslot Manager for Inline Encryption
From: Christoph Hellwig <hch@infradead.org>
Date: 2020-01-17 09:10:05
Also in:
linux-f2fs-devel, linux-fscrypt, linux-fsdevel, linux-scsi
+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.