Re: [PATCH v3 2/2] vsock/virtio: avoid refilling the RX queue after teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect `flush_work` ordering in `virtio_vsock_remove()` allows `virtio_transport_rx_work` to execute after the `vsock` object has been freed.
--

--- Patch [2]: [PATCH v3 2/2] vsock/virtio: avoid refilling the RX queue after teardown ---
commit e28975cab51c80e977d0774ef653bc9c4660fff0
Author: Weiming Shi <bestswngs@gmail.com>

vsock/virtio: avoid refilling the RX queue after teardown

Commit b917507e5ad9 ("vsock/virtio: stop workers during the .remove()")
made the RX worker jump to its common exit when rx_run is clear.  That
exit still refills the RX queue when the buffer count is low, so work
queued across virtio_vsock_vqs_del() can add buffers after the virtqueues
have been deleted.

Link: https://lore.kernel.org/r/20260727035804.1860862-1-bestswngs@gmail.com

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 57f2d6ec3ffc6..96c9fe8d357c6 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c

[ ... ]

> @@ -634,12 +635,12 @@ 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;
> +		goto out_nofill;

[Severity: High]
This is a pre-existing issue, but I have a question about the workqueue
teardown logic that can still trigger a use-after-free for vsock.

In virtio_vsock_remove(), the workqueues are flushed in this order:

net/vmw_vsock/virtio_transport.c:virtio_vsock_remove() {
	...
	flush_work(&vsock->rx_work);
	flush_work(&vsock->tx_work);
	flush_work(&vsock->event_work);
	flush_work(&vsock->send_pkt_work);
	...
}

However, virtio_transport_send_pkt_work() can queue rx_work without holding
locks if restart_rx is true:

net/vmw_vsock/virtio_transport.c:virtio_transport_send_pkt_work() {
	...
out:
	mutex_unlock(&vsock->tx_lock);

	if (restart_rx)
		queue_work(virtio_vsock_workqueue, &vsock->rx_work);
}

If a worker executing virtio_transport_send_pkt_work() is preempted just
before calling queue_work(), and a concurrent virtio_vsock_remove() flushes
rx_work and then blocks flushing send_pkt_work, couldn't the preempted
worker then resume and queue a new rx_work?

This newly queued rx_work would be left pending after virtio_vsock_remove()
completes, causing a use-after-free when the vsock structure is freed.

Should virtio_vsock_remove() flush send_pkt_work before rx_work to prevent
this race?

> +
> +	vq = vsock->vqs[VSOCK_VQ_RX];
>  
>  	do {
>  		virtqueue_disable_cb(vq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785352330.git.bestswngs@gmail.com?part=2