Re: [PATCH v2 1/2] virtio_blk: Add control virtqueue support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] DMA cacheline sharing causes memory corruption on non-coherent architectures.
- [High] Erroneous teardown ordering causes valid in-flight requests to fail with I/O errors during suspend.
- [High] Unbounded wait in `virtblk_ctrl_vq_request` deadlocks device teardown if the host is unresponsive.

Pre-existing issues:
- [High] Use-after-free and double-free of `vblk` in the `virtblk_probe` error path.
--

commit 36271d203fcaee50a88fd7b53aa73dbbba25cf7c
Author: linlzhan <linlin.zhang@oss.qualcomm.com>

virtio_blk: Add control virtqueue support

This commit adds support for the optional virtio-blk control virtqueue.
It allows the driver to manage control-queue requests independently from
the data path, preparing for subsequent inline encryption implementation.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
[ ... ]
> @@ -110,6 +123,12 @@ struct virtblk_req {
>  	struct scatterlist sg[];
>  };
>  
> +struct virtblk_ctrl_request {
> +	__virtio32 type;
> +	u8 status;
> +	struct completion compl;
> +};

[Severity: High]
Is it safe to place DMA targets (type, status) and CPU synchronization
primitives (struct completion compl) in the same struct without cacheline
alignment?

On non-coherent architectures, could the DMA API unmap operation invalidate
the cacheline and destroy the CPU's unwritten modifications to compl.wait,
thereby corrupting the waitqueue? Buffers that are targets of DMA typically
need to be cacheline-aligned or physically separated from CPU-modified
lock/sync primitives.

[ ... ]
> +static int virtblk_ctrl_vq_request(struct virtio_blk *vblk,
> +				    struct virtblk_ctrl_request *creq,
> +				    struct scatterlist *sgs[],
> +				    unsigned int out_sgs, unsigned int in_sgs)
> +{
[ ... ]
> +	wait_for_completion(&creq->compl);
> +	mutex_unlock(&vblk->ctrl_vq.mutex);
> +	return 0;
> +}

[Severity: High]
Could this unbounded wait deadlock device teardown if the host is
unresponsive?

When called from a block I/O context, this thread holds a reference to
q_usage_counter. If the host hangs, del_gendisk() in virtblk_remove() will
freeze the block queue and wait indefinitely for q_usage_counter to reach
zero.

Since virtblk_ctrl_vq_drain() is called after del_gendisk(), wouldn't the
system deadlock before the drain can recover the stuck requests?

[ ... ]
> @@ -1553,6 +1713,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>  out_free_vq:
>  	vdev->config->del_vqs(vdev);
>  	kfree(vblk->vqs);
> +	vblk->ctrl_vq.vq = NULL;
>  out_free_vblk:
>  	kfree(vblk);
>  out_free_index:

[Severity: High]
This is a pre-existing issue, but does the error path in virtblk_probe()
trigger a use-after-free and double-free?

If device_add_disk() fails, the path jumps to out_cleanup_disk which calls
put_disk(vblk->disk). Because vblk->disk->fops points to virtblk_fops,
dropping the last reference invokes virtblk_free_disk(), which frees vblk
and vblk->index.

By the time execution reaches out_free_vq, aren't accesses to vblk->vqs and
this new assignment to vblk->ctrl_vq.vq writing to freed memory, followed
by a double free of vblk in out_free_vblk?

[ ... ]
> @@ -1593,6 +1759,8 @@ static int virtblk_freeze_priv(struct virtio_device *vdev)
>  	struct request_queue *q = vblk->disk->queue;
>  	unsigned int memflags;
>  
> +	virtblk_ctrl_vq_quiesce(vblk);
> +
>  	/* Ensure no requests in virtqueues before deleting vqs. */
>  	memflags = blk_mq_freeze_queue(q);

[Severity: High]
By calling virtblk_ctrl_vq_quiesce() before blk_mq_freeze_queue(), could
valid in-flight requests fail with I/O errors?

virtblk_ctrl_vq_quiesce() sets ctrl_vq.dead = true. Since the block queue
is not yet frozen, in-flight block I/O requests that haven't yet submitted
their control requests (but have already incremented q_usage_counter) will
encounter dead == true when calling virtblk_ctrl_vq_request(), resulting in
-ENODEV.

Should the data plane be frozen before the control queue is marked dead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914133733.15429-1-linlin.zhang@oss.qualcomm.com?part=1