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

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

On 9/14/2026 10:02 PM, sashiko-bot@kernel.org 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 <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.
> 
> [ ... ]

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().