Re: [PATCH] vsock/virtio: prevent workers from using deleted virtqueues

[Stefano Garzarella <sgarzare@redhat.com>]

On Sun, Jul 26, 2026 at 08:58:01PM -0700, Weiming Shi wrote:
>The RX, TX and event workers read their virtqueue pointers before taking
>the mutex that protects the queue and its run flag. A work item delayed
>across freeze and restore can therefore retain a pointer deleted by
>virtio_vsock_vqs_del(), observe the run flag for the replacement queues,
>and use the freed pointer.
>
>RX has an additional path: when rx_run is clear, the common exit still
>refills the RX queue. A queued worker can consequently call
>virtqueue_add_sgs() immediately after freeze deletes the virtqueues.
>
>BUG: KASAN: slab-use-after-free in virtqueue_add_sgs
>Read of size 4 by task kworker/2:1
>Workqueue: virtio_vsock virtio_transport_rx_work
>Call Trace:
> virtqueue_add_sgs (drivers/virtio/virtio_ring.c:2796)
> virtio_vsock_rx_fill (net/vmw_vsock/virtio_transport.c:332)
> virtio_transport_rx_work (net/vmw_vsock/virtio_transport.c:701)
> process_one_work (kernel/workqueue.c:3314)
> worker_thread (kernel/workqueue.c:3478)
> kthread (kernel/kthread.c:436)
> ret_from_fork (arch/x86/kernel/process.c:158)
> ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
>...
>Freed by task 141:
> kfree (mm/slub.c:6566)
> vp_del_vq (drivers/virtio/virtio_pci_common.c:259)
> vp_del_vqs (drivers/virtio/virtio_pci_common.c:285)
> virtio_vsock_freeze (net/vmw_vsock/virtio_transport.c:912)
> virtio_device_freeze (drivers/virtio/virtio.c:658)
> virtio_pci_freeze (drivers/virtio/virtio_pci_common.c:601)
> pci_pm_freeze (drivers/pci/pci-driver.c:1098)
> device_suspend (drivers/base/power/main.c:1968)
>Kernel panic - not syncing: KASAN: panic_on_warn set ...
>
>Read each worker's virtqueue under its mutex after confirming that the
>queue is running, and only refill RX while RX is running.
>
>Fixes: b917507e5ad9 ("vsock/virtio: stop workers during the .remove()")
>Cc: stable@vger.kernel.org
>Reported-by: Xiang Mei <xmei5@asu.edu>
>Assisted-by: OpenAI-Codex:gpt-5
>Signed-off-by: Weiming Shi <bestswngs@gmail.com>
>---
> net/vmw_vsock/virtio_transport.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index 57f2d6ec3ffc..79cf19f58943 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -346,12 +346,13 @@ static void virtio_transport_tx_work(struct work_struct *work)
> 	struct virtqueue *vq;
> 	bool added = false;
>
>-	vq = vsock->vqs[VSOCK_VQ_TX];
> 	mutex_lock(&vsock->tx_lock);
>
> 	if (!vsock->tx_run)
> 		goto out;
>
>+	vq = vsock->vqs[VSOCK_VQ_TX];
>+
> 	do {
> 		struct sk_buff *skb;
> 		unsigned int len;
>@@ -451,13 +452,13 @@ static void virtio_transport_event_work(struct work_struct *work)
> 		container_of(work, struct virtio_vsock, event_work);
> 	struct virtqueue *vq;
>
>-	vq = vsock->vqs[VSOCK_VQ_EVENT];
>-
> 	mutex_lock(&vsock->event_lock);
>
> 	if (!vsock->event_run)
> 		goto out;
>
>+	vq = vsock->vqs[VSOCK_VQ_EVENT];
>+
> 	do {
> 		struct virtio_vsock_event *event;
> 		unsigned int len;
>@@ -634,13 +635,13 @@ static void virtio_transport_rx_work(struct work_struct *work)
> 		container_of(work, struct virtio_vsock, rx_work);
> 	struct virtqueue *vq;
>
>-	vq = vsock->vqs[VSOCK_VQ_RX];
>-
> 	mutex_lock(&vsock->rx_lock);
>
> 	if (!vsock->rx_run)
> 		goto out;
>
>+	vq = vsock->vqs[VSOCK_VQ_RX];
>+
> 	do {
> 		virtqueue_disable_cb(vq);
> 		for (;;) {
>@@ -689,7 +690,8 @@ static void virtio_transport_rx_work(struct work_struct *work)
> 	} while (!virtqueue_enable_cb(vq));
>
> out:
>-	if (vsock->rx_buf_nr < vsock->rx_buf_max_nr / 2)
>+	if (vsock->rx_run &&

Instead of adding this check, what about adding a new label...

>+	    vsock->rx_buf_nr < vsock->rx_buf_max_nr / 2)
> 		virtio_vsock_rx_fill(vsock);

out_nofill:
> 	mutex_unlock(&vsock->rx_lock);

And use it when we check `vsock->rx_run` ?

Since we are taking the mutex, we don't need to check it again.

The rest LGTM, but I agree with Bobby about the split, since the Fixes 
tag here is confusing. I'll reply there.

Thanks,
Stefano