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
