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

On Tue, Jul 28, 2026 at 10:17:55AM -0700, Bobby Eshleman wrote:
>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: [email protected]
>> Reported-by: Xiang Mei <[email protected]>
>> Assisted-by: OpenAI-Codex:gpt-5
>> Signed-off-by: Weiming Shi <[email protected]>
>> ---
>>  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 &&
>> +	    vsock->rx_buf_nr < vsock->rx_buf_max_nr / 2)
>>  		virtio_vsock_rx_fill(vsock);
>>  	mutex_unlock(&vsock->rx_lock);
>>  }
>> --
>> 2.55.0
>
>Not a strong opinion from me, but since this last hunk is the one that
>fixes the bug and the other hunks are moreso hardening, maybe break
>these out into two patches?

I think also the other hunks fix an issue, but I agree on the split 
since IMO we are fixing 2 different commits. Commit b917507e5ad9 
("vsock/virtio: stop workers during the .remove()") was before 
freeze/resume added by commit bd50c5dc182b ("vsock/virtio: add support 
for device suspend/resume"). Only after that one we can have the false 
-> true transition of *_run variables.

So IMO hunks 1-3 should have Fixes: bd50c5dc182b ... and hunk 4 should
have Fixes: b917507e5ad9 ...

That said, it's not a strong opinion here too, but if you prefer a 
single patch, please add both Fixes.

Thanks,
Stefano

>
>Besides, that all looks good to me.
>
>Reviewed-by: Bobby Eshleman <[email protected]>
>