On 9/20/2026 8:35 PM, [email protected] 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 <[email protected]>
>
> 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.
