From: Christoph Hellwig <hch@infradead.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>,
Kuohong Wang <kuohong.wang@mediatek.com>,
Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
linux-f2fs-devel@lists.sourceforge.net,
linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH v6 1/9] block: Keyslot Manager for Inline Encryption
Date: Fri, 17 Jan 2020 01:10:02 -0800 [thread overview]
Message-ID: <20200117091002.GA15396@infradead.org> (raw)
In-Reply-To: <20191218145136.172774-2-satyat@google.com>
> +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.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2020-01-17 9:10 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-18 14:51 [f2fs-dev] [PATCH v6 0/9] Inline Encryption Support Satya Tangirala via Linux-f2fs-devel
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala via Linux-f2fs-devel
2019-12-18 20:13 ` Eric Biggers
2020-01-17 9:10 ` Christoph Hellwig [this message]
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 2/9] block: Add encryption context to struct bio Satya Tangirala via Linux-f2fs-devel
2019-12-18 21:10 ` Eric Biggers
2019-12-18 21:21 ` Darrick J. Wong
2019-12-18 21:25 ` Martin K. Petersen
2019-12-18 22:27 ` Eric Biggers
2019-12-19 0:47 ` Martin K. Petersen
2019-12-20 3:52 ` Eric Biggers
2020-01-07 4:35 ` Martin K. Petersen
2020-01-08 14:07 ` Christoph Hellwig
2020-01-08 17:26 ` Eric Biggers
2020-01-17 8:32 ` Christoph Hellwig
2020-01-18 5:11 ` Eric Biggers
2020-01-21 22:05 ` Satya Tangirala via Linux-f2fs-devel
2020-01-09 3:40 ` Martin K. Petersen
2020-01-14 21:24 ` Eric Biggers
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 3/9] block: blk-crypto for Inline Encryption Satya Tangirala via Linux-f2fs-devel
2019-12-20 3:14 ` Eric Biggers
2019-12-20 5:10 ` Eric Biggers
2020-01-14 21:22 ` Eric Biggers
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 4/9] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala via Linux-f2fs-devel
2020-01-17 12:31 ` Christoph Hellwig
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 5/9] scsi: ufs: UFS crypto API Satya Tangirala via Linux-f2fs-devel
2019-12-20 4:48 ` Eric Biggers
2020-01-14 21:16 ` Eric Biggers
2020-01-17 13:51 ` Christoph Hellwig
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala via Linux-f2fs-devel
2019-12-20 5:44 ` Eric Biggers
2020-01-17 13:58 ` Christoph Hellwig
2020-01-18 5:27 ` Eric Biggers
2020-02-05 18:07 ` Christoph Hellwig
2020-01-18 3:58 ` Eric Biggers
2020-02-05 20:47 ` Eric Biggers
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 7/9] fscrypt: add inline encryption support Satya Tangirala via Linux-f2fs-devel
2020-01-14 21:12 ` Eric Biggers
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 8/9] f2fs: " Satya Tangirala via Linux-f2fs-devel
2019-12-20 4:23 ` Eric Biggers
2019-12-18 14:51 ` [f2fs-dev] [PATCH v6 9/9] ext4: " Satya Tangirala via Linux-f2fs-devel
2019-12-19 0:12 ` Eric Biggers
2019-12-19 0:31 ` Satya Tangirala via Linux-f2fs-devel
2019-12-22 0:16 ` Eric Biggers
2020-01-08 14:05 ` [f2fs-dev] [PATCH v6 0/9] Inline Encryption Support Christoph Hellwig
2020-01-08 18:43 ` Satya Tangirala via Linux-f2fs-devel
2020-01-17 8:52 ` Christoph Hellwig
2020-02-01 0:53 ` Satya Tangirala via Linux-f2fs-devel
2020-02-03 9:15 ` Christoph Hellwig
2020-02-04 3:39 ` Satya Tangirala via Linux-f2fs-devel
2020-02-04 14:58 ` Christoph Hellwig
2020-02-04 21:21 ` Eric Biggers
2020-02-05 7:36 ` Eric Biggers
2020-02-05 18:05 ` Christoph Hellwig
2020-02-21 12:30 ` Satya Tangirala via Linux-f2fs-devel
2020-02-21 14:20 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200117091002.GA15396@infradead.org \
--to=hch@infradead.org \
--cc=bmuthuku@qti.qualcomm.com \
--cc=boojin.kim@samsung.com \
--cc=kuohong.wang@mediatek.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=satyat@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).