Re: [PATCH v4 2/2] virtio_blk: add inline encryption support

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Oct 08, 2026 at 09:17:14PM -0700, Linlin Zhang wrote:
> +config VIRTIO_BLK_INLINE_ENCRYPTION
> +	tristate "Virtio block inline encryption support"
> +	depends on VIRTIO_BLK && BLK_INLINE_ENCRYPTION
> +	help
> +	  Say 'Y or M' here will allow the virtio block driver to route crypto
> +	  requests to a different operating system in a virtualized
> +	  environment. This is useful to encrypt the data stored in the storage
> +	  by using storage inline crypto engine. The control queue feature bit
> +	  must be negotiated to enable this functionality.
> +
> +	  If unsure, say N.

This should be a bool, not a tristate.  (Since it controls the inclusion
of a feature in a module, rather than the inclusion of a module.)

> +#if IS_ENABLED(CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION)
> +static int virtblk_get_crypto_modes(struct virtio_blk *vblk,
> +				    unsigned int *crypto_modes_supported)
> +{
> +	unsigned int nr_modes = VIRTIO_BLK_CRYPTO_MODE_MAX + 1;
> +	struct scatterlist type_sg, resp_sg, status_sg, *sgs[3];
> +	struct virtblk_ctrl_request *creq;
> +	unsigned int i;
> +	int err;
> +
> +	creq = kzalloc_obj(*creq, GFP_KERNEL);
> +	if (!creq)
> +		return -ENOMEM;
> +
> +	creq->type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_CRYPTO_MODES);
> +
> +	sg_init_one(&type_sg, &creq->type, sizeof(creq->type));
> +	sg_init_one(&resp_sg, &creq->in_resp.modes, sizeof(creq->in_resp.modes));
> +	sg_init_one(&status_sg, &creq->status, sizeof(creq->status));
> +	sgs[0] = &type_sg;
> +	sgs[1] = &resp_sg;
> +	sgs[2] = &status_sg;
> +
> +	err = virtblk_ctrl_vq_request(vblk, creq, sgs, 1, 2);

What happens if the host follows a future version of the spec and wants
to return more than VIRTIO_BLK_CRYPTO_MODE_MAX modes?

> +static int virtblk_crypto_keyslot_program(struct blk_crypto_profile *profile,
> +					   const struct blk_crypto_key *key,
> +					   unsigned int slot)
> +{
> +	struct virtio_blk *vblk = virtblk_from_profile(profile);
> +	struct scatterlist type_sg, out_req_sg, status_sg, *sgs[3];
> +	struct virtblk_ctrl_request *creq;
> +	int err;
> +
> +	mutex_lock(&vblk->vdev_mutex);
> +	if (!vblk->vdev) {
> +		err = -ENXIO;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * GFP_NOIO: this callback runs on the bio-submission path, which
> +	 * memory reclaim can reach while writing back dirty pages to this
> +	 * same device; GFP_KERNEL here could recurse into that same reclaim
> +	 * and self-deadlock.
> +	 */
> +	creq = kzalloc_obj(*creq, GFP_NOIO);
> +	if (!creq) {
> +		err = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	creq->type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_CRYPTO_KEYSLOT_PROGRAM);
> +
> +	err = set_virtblk_crypto_key_desc(vblk->vdev, creq, key, slot);
> +	if (err)
> +		goto out_free;
> +
> +	sg_init_one(&type_sg, &creq->type, sizeof(creq->type));
> +	sg_init_one(&out_req_sg, &creq->out_req.key_desc, sizeof(creq->out_req.key_desc));
> +	sg_init_one(&status_sg, &creq->status, sizeof(creq->status));
> +	sgs[0] = &type_sg;
> +	sgs[1] = &out_req_sg;
> +	sgs[2] = &status_sg;
> +
> +	err = virtblk_ctrl_vq_request(vblk, creq, sgs, 2, 1);
> +	if (err == -ETIMEDOUT)
> +		goto out_unlock;

Is the -ETIMEDOUT handling really correct?  This basically turns this
into an asynchronous operation; the host might still complete it later.

> +/* Must be >= BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE in include/linux/blk-crypto.h */
> +#define VIRTIO_BLK_CRYPTO_MAX_KEY_SIZE		128

Does this value really need to be exposed in a UAPI header?  As
documented in the comment above BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE, it's
supposed to be possible to increase it if ever needed.  This change
takes away that flexibility by making the current (somewhat arbitrarily
chosen) value part of the UAPI.

- Eric