On Wed, Mar 25, 2020 at 08:06:53PM -0700, Satya Tangirala wrote:
> diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h
> new file mode 100644
> index 0000000000000..1e6d339fe1429
> --- /dev/null
> +++ b/block/blk-crypto-internal.h
> @@ -0,0 +1,172 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2019 Google LLC
> + */
> +
> +#ifndef __LINUX_BLK_CRYPTO_INTERNAL_H
> +#define __LINUX_BLK_CRYPTO_INTERNAL_H
> +
> +#include <linux/bio.h>
> +#include <linux/blkdev.h>
> +
> +/* Represents a crypto mode supported by blk-crypto */
> +struct blk_crypto_mode {
> + const char *cipher_str; /* crypto API name (for fallback case) */
Technically, cipher_str shouldn't be added until the patch that adds
blk-crypto-fallback.
> + unsigned int keysize; /* key size in bytes */
> + unsigned int ivsize; /* iv size in bytes */
> +};
> +
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +
> +static inline const struct blk_crypto_key *bio_crypt_key(struct bio *bio)
> +{
> + if (!bio_has_crypt_ctx(bio))
> + return NULL;
> + return bio->bi_crypt_context->bc_key;
> +}
I don't believe bio_crypt_key() is necessary -- see my comment below regarding
blk_mq_make_request().
> +bool bio_crypt_dun_is_contiguous(const struct bio_crypt_ctx *bc,
> + unsigned int bytes,
> + u64 next_dun[BLK_CRYPTO_DUN_ARRAY_SIZE])
next_dun should be const.
> +{
> + int i = 0;
> + unsigned int inc = bytes >> bc->bc_key->data_unit_size_bits;
> +
> + while (i < BLK_CRYPTO_DUN_ARRAY_SIZE) {
> + if (bc->bc_dun[i] + inc != next_dun[i])
> + return false;
> + /*
> + * If addition of inc to the current entry caused an overflow,
> + * then we have to carry "1" for the next entry - so inc
> + * needs to be "1" for the next loop iteration). Otherwise,
> + * we need inc to be 0 for the next loop iteration. Since
> + * overflow can be determined by (bc->bc_dun[i] + inc) < inc
> + * we can do the following.
> + */
> + inc = ((bc->bc_dun[i] + inc) < inc);
> + i++;
> + }
This comment is verbose but doesn't really explain what's going on.
I think it would be much more useful to add comments like:
/*
* Returns true if @bc_dun plus @bytes converted to data units is equal to
* @next_dun, treating the DUNs as multi-limb integers.
*/
bool bio_crypt_dun_is_contiguous(const struct bio_crypt_ctx *bc,
unsigned int bytes,
const u64 next_dun[BLK_CRYPTO_DUN_ARRAY_SIZE])
And:
/*
* If the addition in this limb overflowed, then the carry bit
* into the next limb is 1. Else the carry bit is 0.
*/
inc = ((bc->bc_dun[i] + inc) < inc);
> +
> + /*
> + * After going through all the entries in the dun, inc must be 0 for
> + * the duns to be contiguous.
> + */
> + return !inc;
> +}
This is new in v9; before it just returned true.
It would be more helpful if this comment was like:
/* If the DUN wrapped through 0, don't treat it as contiguous. */
> +/**
> + * __blk_crypto_init_request - Initializes the request's crypto fields based on
> + * the blk_crypto_key for a bio to be added to the
> + * request, and prepares it for hardware inline
> + * encryption.
> + *
> + * @rq: The request to init
> + * @key: The blk_crypto_key of bios that will (eventually) be added to @rq.
> + *
> + * Initializes the request's crypto fields to appropriate default values and
> + * tries to get a keyslot for the blk_crypto_key.
> + *
> + * Return: BLK_STATUS_OK on success, and negative error code otherwise.
> + */
> +blk_status_t __blk_crypto_init_request(struct request *rq,
> + const struct blk_crypto_key *key)
> +{
> + return blk_ksm_get_slot_for_key(rq->q->ksm, key, &rq->crypt_keyslot);
> +}
The comment of this function seems outdated. All it does it get a keyslot, but
the comment talks about initializing "crypto fields" (plural).
> +/**
> + * blk_crypto_evict_key() - Evict a key from any inline encryption hardware
> + * it may have been programmed into
> + * @q: The request queue who's keyslot manager this key might have been
> + * programmed into
> + * @key: The key to evict
> + *
> + * Upper layers (filesystems) should call this function to ensure that a key
> + * is evicted from hardware that it might have been programmed into. This
> + * will call blk_ksm_evict_key on the queue's keyslot manager, if one
> + * exists, and supports the crypto algorithm with the specified data unit size.
> + *
> + * Return: 0 on success or if key is not present in the q's ksm, -err on error.
> + */
> +int blk_crypto_evict_key(struct request_queue *q,
> + const struct blk_crypto_key *key)
> +{
> + if (q->ksm && blk_ksm_crypto_key_supported(q->ksm, key))
> + return blk_ksm_evict_key(q->ksm, key);
> +
> + return 0;
> +}
The keyslot manager is really an implementation detail of the block layer.
Therefore when documenting functions like blk_crypto_evict_key() that are the
API for use by upper layers like filesystems, we shouldn't really talk about the
keyslot manager, but rather describe the behavior in higher-level terms like
"evict a key from the inline encryption hardware".
> @@ -1971,6 +1974,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
> struct request *same_queue_rq = NULL;
> unsigned int nr_segs;
> blk_qc_t cookie;
> + blk_status_t ret;
>
> blk_queue_bounce(q, &bio);
> __blk_queue_split(q, &bio, &nr_segs);
> @@ -2002,6 +2006,14 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>
> cookie = request_to_qc_t(data.hctx, rq);
>
> + ret = blk_crypto_init_request(rq, bio_crypt_key(bio));
> + if (ret != BLK_STS_OK) {
> + bio->bi_status = ret;
> + bio_endio(bio);
> + blk_mq_free_request(rq);
> + return BLK_QC_T_NONE;
> + }
> +
> blk_mq_bio_to_request(rq, bio, nr_segs);
Wouldn't it make a lot more sense to do blk_crypto_init_request() after
blk_mq_bio_to_request() rather than before?
I.e., initialize request::crypt_ctx first, *then* get the keyslot. Not the
other way around.
That would allow removing the second argument to blk_crypto_init_request() and
removing bio_crypt_key(). blk_crypto_init_request() would only need to take in
the struct request.
> diff --git a/include/linux/blk-crypto.h b/include/linux/blk-crypto.h
> index b8d54eca1c0d8..4add2e8846fea 100644
> --- a/include/linux/blk-crypto.h
> +++ b/include/linux/blk-crypto.h
> @@ -6,6 +6,8 @@
> #ifndef __LINUX_BLK_CRYPTO_H
> #define __LINUX_BLK_CRYPTO_H
>
> +#include <linux/types.h>
> +
> enum blk_crypto_mode_num {
> BLK_ENCRYPTION_MODE_INVALID,
> BLK_ENCRYPTION_MODE_AES_256_XTS,
> @@ -24,7 +26,7 @@ enum blk_crypto_mode_num {
> * 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
> - * @dun_bytes: the number of bytes of DUN used when using this key
> + * @dun_bytes: the maximum number of bytes of DUN used when using this key
This comment update should be folded into the patch that added this.
> +/**
> + * struct bio_crypt_ctx - an inline encryption context
> + * @bc_key: the key, algorithm, and data unit size to use
> + * @bc_dun: the data unit number (starting IV) to use
> + * @bc_keyslot: the keyslot that has been assigned for this key in @bc_ksm,
> + * or -1 if no keyslot has been assigned yet.
> + * @bc_ksm: the keyslot manager into which the key has been programmed with
> + * @bc_keyslot, or NULL if this key hasn't yet been programmed.
> + *
> + * A bio_crypt_ctx specifies that the contents of the bio will be encrypted (for
> + * write requests) or decrypted (for read requests) inline by the storage device
> + * or controller.
> + */
> +struct bio_crypt_ctx {
> + const struct blk_crypto_key *bc_key;
> + u64 bc_dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
> +};
Kerneldoc needs to be updated; bc_keyslot and bc_ksm no longer exist.
- Eric
