On 9/14/2026 10:02 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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.
>
> [ ... ]
ACK
Update struct completion compl to a pointer of struct completion compl.
>> +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?
>
> [ ... ]
ACK
Add abandon flag in struct virtblk_ctrl_request. Set a 10 seconds timeout when
sending a request via the control queue. Set the control request as abandoned
once it doesn't return in 10 seconds to force finishing the caller in the
control
queue, so that q_usage_counter in the data I/O thread can decrease to 0, unblock
del_gendisk() finally.
>> @@ -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?
>
> [ ... ]
It doesn't lead to use-after-free or double-free.
Because
1. put_disk() calls disk_release();
2. disk_release(）calls disk->fops->free_disk() only if GD_ADDED is set.
3. GD_ADDED is only set in add_disk_final() after the successful call of
device_add_disk().
GD_ADDED won't be set if device_add_disk() returns the failure, then
virtblk_free_disk() won't be called. vblk->ctrl_vq.vq is set to NULL before
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?
>
ACK
Move virtblk_ctrl_vq_quiesce(vblk) after blk_mq_freeze_queue(q), before
virtio_reset_device().
