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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] block/virtio_blk: DMA coherency violation on creq->abandoned
- [Medium] block/virtio_blk: TOCTOU race in control request timeout leads to memory leak
--

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

virtio_blk: Add control virtqueue support

This patch adds support for the optional virtio-blk control virtqueue,
allowing the driver to manage control-queue requests independently from
the data path. The control virtqueue is intended for a subsequent inline
encryption implementation.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 32bf3ba07a9dc..2fad86e8f7a9a 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
[ ... ]
> @@ -110,6 +123,20 @@ struct virtblk_req {
>  	struct scatterlist sg[];
>  };
>  
> +struct virtblk_ctrl_request {
> +	__virtio32 type;
> +	u8 status;
> +
> +	struct completion *compl;
> +	/*
> +	 * Set when virtblk_ctrl_vq_request()'s waiter timed out and moved on
> +	 * without freeing this request. Whichever of virtblk_ctrlq_callback()
> +	 * or virtblk_ctrl_vq_drain() later retrieves the buffer must free
> +	 * @compl and this struct instead of calling complete() on them.
> +	 */
> +	bool abandoned;
> +};

[Severity: High]
Could this cause a DMA coherency violation? The struct bundles the
device-written DMA field (status) with a CPU-modified software flag
(abandoned) within the same cacheline.

On non-cache-coherent architectures, if the CPU writes to the abandoned
flag while the device is actively DMA-writing to the status field, it
could result in cacheline tearing. This might lead to either silent data
corruption of the DMA-mapped response or loss of the CPU write.

[ ... ]
> +static void virtblk_ctrlq_callback(struct virtqueue *vq)
> +{
> +	struct virtio_blk *vblk = vq->vdev->priv;
> +	struct virtblk_ctrl_request *creq;
> +	unsigned long flags;
> +	unsigned int len;
> +
> +	spin_lock_irqsave(&vblk->ctrl_vq.lock, flags);
> +	do {
> +		virtqueue_disable_cb(vq);
> +		while ((creq = virtqueue_get_buf(vq, &len)) != NULL) {
> +			bool drained = false;
> +			bool abandoned = creq->abandoned;
> +
> +			if (WARN_ON_ONCE(!vblk->ctrl_vq.inflight)) {
[ ... ]
> +			if (drained)
> +				complete(&vblk->ctrl_vq.drained);
> +			if (abandoned) {
> +				kfree(creq->compl);
> +				kfree(creq);
> +			} else {
> +				complete(creq->compl);
> +			}
> +			spin_lock_irqsave(&vblk->ctrl_vq.lock, flags);
> +		}
> +	} while (!virtqueue_enable_cb(vq));
> +	spin_unlock_irqrestore(&vblk->ctrl_vq.lock, flags);
> +}

[Severity: Medium]
Is there a race condition here that could lead to a memory leak of both
the request and completion structs?

If a request times out exactly as the device completes it, the IRQ thread
might read creq->abandoned as false, unlock the spinlock, and get
preempted.

Meanwhile, virtblk_ctrl_vq_request() wakes up from the timeout, acquires
the lock, sets creq->abandoned to true, unlocks, and returns -ETIMEDOUT.

When the callback resumes, it checks its local abandoned variable (which
is still false) and calls complete(creq->compl) instead of freeing the
memory. Because the caller already discarded the pointer upon timeout,
both creq and compl would be permanently leaked.

[ ... ]
> +/* Submit a control-queue request and wait for completion. */
> +static int virtblk_ctrl_vq_request(struct virtio_blk *vblk,
[ ... ]
> +	if (wait_for_completion_timeout(comp, VIRTBLK_CTRL_VQ_TIMEOUT)) {
> +		mutex_unlock(&vblk->ctrl_vq.mutex);
> +		kfree(comp);
> +		return 0;
> +	}
> +
> +	/*
> +	 * The host hasn't responded within the timeout. @creq is still
> +	 * owned by the device, so don't touch its DMA-target fields or
> +	 * free it here. Mark it abandoned and hand ownership of both @creq
> +	 * and @comp to whichever of virtblk_ctrlq_callback() or
> +	 * virtblk_ctrl_vq_drain() retrieves the buffer later; unlock the
> +	 * mutex so subsequent requests aren't serialized behind an
> +	 * unresponsive host.
> +	 */
> +	spin_lock_irqsave(&vblk->ctrl_vq.lock, flags);
> +	creq->abandoned = true;
> +	spin_unlock_irqrestore(&vblk->ctrl_vq.lock, flags);
> +	mutex_unlock(&vblk->ctrl_vq.mutex);
> +
> +	dev_warn(&vblk->vdev->dev,
> +		 "control queue request timed out, abandoning\n");
> +	return -ETIMEDOUT;
> +}

[Severity: High]
This timeout path is where the CPU writes to the shared cacheline while
the DMA operation might still be in flight. Does this trigger the DMA
coherency violation mentioned above for struct virtblk_ctrl_request?

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