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. [ ... ] > +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? [ ... ] > @@ -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? [ ... ] > @@ -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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
