On Wed, Mar 25, 2020 at 08:06:53PM -0700, Satya Tangirala wrote:

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

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.

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