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

[Linlin Zhang <linlin.zhang@oss.qualcomm.com>]

On 10/16/2026 4:26 AM, Eric Biggers wrote:
> 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.)

Thanks for your comment!

Double confirmed that the virtio block driver is still built to virtio_blk.ko
in the combination CONFIG_VIRTIO_BLK=m and
CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION=y.

I'll modify it to bool and update '#if IS_ENABLED(CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION)'
to #ifdef CONFIG_VIRTIO_BLK_INLINE_ENCRYPTION

> 
>> +#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?

Thanks for pointing this out!

The response is bounded by the device-writable buffer provided by the frontend.
VIRTIO_BLK_CRYPTO_MODE_MAX defines the maximum number of valid modes supported
by this version of the frontend, with entry 0 reserved and always zero.

A backend implementing an older version of the spec may return fewer modes.
Conversely, if a newer backend supports additional modes, it must not write more
entries than can fit in the response buffer provided by the frontend. Therefore,
additional modes that cannot fit in an older frontend's response buffer cannot
be returned to that frontend.

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

Thanks for pointing it out!

I agree that the -ETIMEDOUT handling turns the keyslot management request into an
asynchronous operation, but I believe it won't leads to functional error for inline
encryption as 
  1. the key eviction in block-crypto layer doesn't depends on the return value of
     keyslot_evict of blk-crypto-ops
  2. the virtual keyslots per device in the host can be overwritten.

The timeout was added to avoid a lock-ordering hang in virtblk_remove().

  - A keyslot callback such as virtblk_crypto_keyslot_program() takes vdev_mutex to
    ensure the virtio device won't be set to NULL during the this operation.
    It holds that vdev_mutex while virtblk_ctrl_vq_request() sleeps waiting for the
    host.

  - virtblk_remove() calls virtblk_ctrl_vq_quiesce() and then acquires vdev_mutex.
    It only calls virtio_reset_device() and virtblk_ctrl_vq_drain() after acquiring
    that lock. Those are the calls that would wake the waiter with VIRTIO_BLK_S_IOERR.

  - If the host never answers, the waiter holds vdev_mutex and waits for a wakeup
    that only comes after virtblk_remove() gets vdev_mutex. virtblk_remove() waits
    for the waiter, so this is a dead lock. 

The 10-second timeout avoids the deadlock at the cost of falling back to asynchronous handling.

I don't think we can eliminate the asynchronous path entirely due to the lock
ordering between the key programming/eviction paths and virtblk_remove().
However, we can reduce the cases where it is needed. Ideally, it would only
be used during virtblk_remove(), whereas the current implementation may also
fall back to asynchronous handling when a keyslot operation does not complete
within 10 seconds.

> 
>> +/* 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.

ACK. Thanks for your comments!

Currently, only struct virtio_blk_crypto_key_desc and struct
virtio_blk_crypto_key_blob use VIRTIO_BLK_CRYPTO_MAX_KEY_SIZE for fixed-size
array fields. I will change these to variable-length arrays which are always
the last filed of the struct, with the actual length specified by the
corresponding key_size field. The device-readable/writable buffer size for
commands using these structures will therefore be the size of the fixed
fields plus key_size.

For struct virtio_blk_crypto_sw_secret, however, the software secret size is
always fixed at 32 bytes:

  /* Must match BLK_CRYPTO_SW_SECRET_SIZE in include/linux/blk-crypto.h */
  #define VIRTIO_BLK_CRYPTO_SW_SECRET_SIZE	32

  /* Reply to a VIRTIO_BLK_T_CRYPTO_DERIVE_SW_SECRET request. */
  struct virtio_blk_crypto_sw_secret {
	  __u8 secret[VIRTIO_BLK_CRYPTO_SW_SECRET_SIZE];
  };

So I think it makes sense to keep secret as a fixed-size array here.

Please let me know if you see any issues with this approach.

> 
> - Eric