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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] block: NULL pointer dereference in virtblk_ctrl_vq_request on device removal
- [High] block: memory leak in virtblk_ctrl_vq_request due to timeout race
- [High] block: UAF and double free in virtblk_probe error path
- [Low] block: missing mutex_destroy on virtblk_probe error path
--

commit 4ba5fa6213a4ecb3d12630534388c763dd962458
Author: Linlin Zhang <linlin.zhang@oss.qualcomm.com>
Subject: virtio_blk: Add control virtqueue support

This patch 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 a subsequent inline encryption
implementation.

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index c8bfbf8435ac7..2499ae2fb9e22 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c

[ ... ]

> @@ -864,11 +898,181 @@ static int virtblk_getgeo(struct gendisk *disk, struct hd_geometry *geo)
> +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)
> +{

[ ... ]

> +	if (wait_for_completion_timeout(&comp->done, VIRTBLK_CTRL_VQ_TIMEOUT)) {
> +		mutex_unlock(&vblk->ctrl_vq.mutex);
> +		kfree(comp);
> +		creq->compl = NULL;
> +		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);
> +	comp->abandoned = true;
> +	spin_unlock_irqrestore(&vblk->ctrl_vq.lock, flags);
> +	mutex_unlock(&vblk->ctrl_vq.mutex);

[Severity: High]
Could this leak memory if the request completes just as the timeout expires?

When wait_for_completion_timeout() expires, the waiter acquires ctrl_vq.lock
and sets comp->abandoned = true without checking if the completion was
already signaled.

If the hardware interrupt fires concurrently, virtblk_ctrlq_callback() might
run on another CPU before the lock is acquired, read abandoned as false,
call complete(), and skip freeing the memory. Since both paths assume the
other is responsible for cleanup during this specific race, the creq and
comp structures would be leaked permanently.

> +
> +	dev_warn(&vblk->vdev->dev,
> +		 "control queue request timed out, abandoning\n");

[Severity: Critical]
Is it safe to dereference vblk->vdev here without holding vdev_mutex?

If a request times out exactly as the device is being removed,
virtblk_remove() calls virtblk_ctrl_vq_quiesce(), which waits for the same
10-second timeout. When quiesce times out, it proceeds to reset the device,
drain the queue, and set vblk->vdev = NULL.

Meanwhile, this thread wakes up from the timeout, drops ctrl_vq.mutex (which
does not hold vdev_mutex), and dereferences vblk->vdev to log the warning.
Since vblk->vdev might have been cleared by the concurrent teardown, this
could cause a NULL pointer dereference.

> +	return -ETIMEDOUT;
> +}

[ ... ]

> @@ -1465,14 +1686,18 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	}
>  
>  	mutex_init(&vblk->vdev_mutex);
> +	mutex_init(&vblk->ctrl_vq.mutex);
> +	spin_lock_init(&vblk->ctrl_vq.lock);
>  
>  	vblk->vdev = vdev;
>  
>  	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
>  
>  	err = init_vq(vblk);
> -	if (err)
> +	if (err) {
> +		dev_err(&vdev->dev, "init virt queue failed: err = %d\n", err);
>  		goto out_free_vblk;
> +	}

[Severity: Low]
Does this error path miss destroying the newly initialized mutex?

mutex_init(&vblk->ctrl_vq.mutex) happens early in the probe sequence. If
init_vq() fails, the code jumps to out_free_vblk, which only calls
kfree(vblk). This bypasses mutex_destroy(), potentially leaking lockdep
tracking state when CONFIG_DEBUG_MUTEXES is enabled.

[ ... ]

> @@ -1554,6 +1779,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);

[Severity: High]
Could this error path trigger a use-after-free and double free?

If an error occurs and the path jumps to out_cleanup_disk (just above this
diff snippet), the code calls put_disk(vblk->disk). Since the disk has a
refcount of 1 here, this drops it to 0, triggering the virtblk_free_disk()
callback which frees vblk.

Execution then falls through to blk_mq_free_tag_set(&vblk->tag_set), touches
the freed memory by assigning vblk->ctrl_vq.vq = NULL, and ultimately calls
kfree(vblk) again.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009041727.3170811-1-linlin.zhang@oss.qualcomm.com?part=1