Re: [PATCH v11 02/12] block: Keyslot Manager for Inline Encryption

From: Eric Biggers <ebiggers@kernel.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-ext4@vger.kernel.org,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Kim Boojin <boojin.kim@samsung.com>
Subject: Re: [PATCH v11 02/12] block: Keyslot Manager for Inline Encryption
Date: Wed, 29 Apr 2020 23:46:21 -0700	[thread overview]
Message-ID: <20200430064621.GA16238@sol.localdomain> (raw)
In-Reply-To: <20200429072121.50094-3-satyat@google.com>

A few very minor comments:

On Wed, Apr 29, 2020 at 07:21:11AM +0000, Satya Tangirala wrote:
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> new file mode 100644
> index 0000000000000..b584723b392ad
> --- /dev/null
> +++ b/block/keyslot-manager.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Google LLC
> + */
> +
> +/**
> + * DOC: The Keyslot Manager
> + *
> + * Many devices with inline encryption support have a limited number of "slots"
> + * into which encryption contexts may be programmed, and requests can be tagged
> + * with a slot number to specify the key to use for en/decryption.
> + *
> + * As the number of slots are limited, and programming keys is expensive on
> + * many inline encryption hardware, we don't want to program the same key into
> + * multiple slots - if multiple requests are using the same key, we want to
> + * program just one slot with that key and use that slot for all requests.
> + *
> + * The keyslot manager manages these keyslots appropriately, and also acts as
> + * an abstraction between the inline encryption hardware and the upper layers.
> + *
> + * Lower layer devices will set up a keyslot manager in their request queue
> + * and tell it how to perform device specific operations like programming/
> + * evicting keys from keyslots.
> + *
> + * Upper layers will call blk_ksm_get_slot_for_key() to program a
> + * key into some slot in the inline encryption hardware.
> + */
> +#include <crypto/algapi.h>

Now that this file doesn't use crypto_memneq(), the include of <crypto/algapi.h>
can be removed.

> +/**
> + * blk_ksm_get_slot_for_key() - Program a key into a keyslot.
> + * @ksm: The keyslot manager to program the key into.
> + * @key: Pointer to the key object to program, including the raw key, crypto
> + *	 mode, and data unit size.
> + * @keyslot: A pointer to return the pointer of the allocated keyslot.
> + *
> + * Get a keyslot that's been programmed with the specified key.  If one already
> + * exists, return it with incremented refcount.  Otherwise, wait for a keyslot
> + * to become idle and program it.
> + *
> + * Context: Process context. Takes and releases ksm->lock.
> + * Return: BLK_STS_OK on success (and keyslot is set to the pointer of the
> + *	   allocated keyslot), or some other blk_status_t otherwise (and
> + *	   keyslot is set to NULL).
> + */
> +blk_status_t blk_ksm_get_slot_for_key(struct blk_keyslot_manager *ksm,
> +				      const struct blk_crypto_key *key,
> +				      struct blk_ksm_keyslot **slot_ptr)

The comment should say @slot_ptr, not @keyslot.  You can find kerneldoc warnings
using 'scripts/kernel-doc -v -none $file'.

> +/**
> + * blk_ksm_crypto_cfg_supported() - Find out if the crypto_mode, dusize, dun
> + *				    bytes of a crypto_config are supported by a
> + *				    ksm.

IMO, shorten this a bit to something like "Find out if a crypto configuration is
supported by a ksm", so that less of the comment becomes outdated when someone
adds a new field.

> + * @ksm: The keyslot manager to check
> + * @cfg: The crypto configuration to check for.
> + *
> + * Checks for crypto_mode/data unit size/dun bytes support.
> + *
> + * Return: Whether or not this ksm supports the specified crypto key.

"config", not "key".

- Eric