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

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

On 9/20/2026 8:35 PM, sashiko-bot@kernel.org wrote:
> 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.

ACK

Fix it by moving both struct completion and bool abandoned to a pointer.

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

ACK

Lock the whole code snippet of do-while statement.

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

ACK

Move struct completion and bool abandoned in struct virtblk_ctrl_request
to virtblk_ctrl_completion *. one time kmalloc_obj() of
virtblk_ctrl_completion * allow them not in the scatterlist.