Re: [PATCH v6 4/5] vhost: synchronize with RCU readers when freeing workers

[Stefano Garzarella <sgarzare@redhat.com>]

On Fri, Jul 24, 2026 at 02:45:41PM +0300, Andrey Drobyshev wrote:
>vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>vq->worker and queues work on it.  vhost_workers_free() however clears
>the vq->worker pointers and immediately frees the workers, without
>waiting for a grace period.  A caller that fetched the worker right
>before the pointer was cleared can therefore still be queueing work on
>it while it is freed.  And even when the queueing itself wins the race,
>the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>future attempts to queue it are silently skipped.
>
>None of the current callers can actually hit this: net and scsi stop
>their virtqueues before the workers are freed, and vsock unhashes the
>device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>in vhost-vsock keeps the device hashed while its workers are freed, so
>the lockless send/cancel paths become able to race with the teardown.
>
>Fix this by threading a bool 'sync' param through the chain:
>
>  vhost_dev_reset_owner()
>    vhost_dev_cleanup()
>      vhost_workers_free()
>
>When set, after clearing the vq->worker pointers, wait for a grace
>period and flush the workers so any work the last RCU readers queued
>runs (clearing VHOST_WORK_QUEUED) before the workers are freed.  Only
>the vsock RESET_OWNER path (added in the next patch) passes sync=true;
>every other teardown has already quiesced and passes false, so they do
>not need to wait the grace period.

Maybe the previous version was enough, but this one definitely covers
it, so if it's okay with Michael, it's okay with me too:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
> drivers/vhost/net.c   |  4 ++--
> drivers/vhost/scsi.c  |  2 +-
> drivers/vhost/test.c  |  4 ++--
> drivers/vhost/vdpa.c  |  4 ++--
> drivers/vhost/vhost.c | 26 +++++++++++++++++++++-----
> drivers/vhost/vhost.h |  5 +++--
> drivers/vhost/vsock.c |  2 +-
> 7 files changed, 32 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>index 3e72b9c6af0c..fd089412c020 100644
>--- a/drivers/vhost/net.c
>+++ b/drivers/vhost/net.c
>@@ -1452,7 +1452,7 @@ static int vhost_net_release(struct inode *inode, struct file *f)
> 	vhost_net_stop(n, &tx_sock, &rx_sock);
> 	vhost_net_flush(n);
> 	vhost_dev_stop(&n->dev);
>-	vhost_dev_cleanup(&n->dev);
>+	vhost_dev_cleanup(&n->dev, false);
> 	vhost_net_vq_reset(n);
> 	if (tx_sock)
> 		sockfd_put(tx_sock);
>@@ -1661,7 +1661,7 @@ static long vhost_net_reset_owner(struct vhost_net *n)
> 	vhost_net_stop(n, &tx_sock, &rx_sock);
> 	vhost_net_flush(n);
> 	vhost_dev_stop(&n->dev);
>-	vhost_dev_reset_owner(&n->dev, umem);
>+	vhost_dev_reset_owner(&n->dev, umem, false);
> 	vhost_net_vq_reset(n);
> done:
> 	mutex_unlock(&n->dev.mutex);
>diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
>index 9a1253b9d8c5..b6002b64cf42 100644
>--- a/drivers/vhost/scsi.c
>+++ b/drivers/vhost/scsi.c
>@@ -2350,7 +2350,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f)
> 	mutex_unlock(&vs->dev.mutex);
> 	vhost_scsi_clear_endpoint(vs, &t);
> 	vhost_dev_stop(&vs->dev);
>-	vhost_dev_cleanup(&vs->dev);
>+	vhost_dev_cleanup(&vs->dev, false);
> 	kfree(vs->dev.vqs);
> 	kfree(vs->vqs);
> 	kfree(vs->old_inflight);
>diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
>index 24514c8fdee4..84ed9ea81219 100644
>--- a/drivers/vhost/test.c
>+++ b/drivers/vhost/test.c
>@@ -163,7 +163,7 @@ static int vhost_test_release(struct inode *inode, struct file *f)
> 	vhost_test_stop(n, &private);
> 	vhost_test_flush(n);
> 	vhost_dev_stop(&n->dev);
>-	vhost_dev_cleanup(&n->dev);
>+	vhost_dev_cleanup(&n->dev, false);
> 	kfree(n->dev.vqs);
> 	kfree(n);
> 	return 0;
>@@ -238,7 +238,7 @@ static long vhost_test_reset_owner(struct vhost_test *n)
> 	vhost_test_stop(n, &priv);
> 	vhost_test_flush(n);
> 	vhost_dev_stop(&n->dev);
>-	vhost_dev_reset_owner(&n->dev, umem);
>+	vhost_dev_reset_owner(&n->dev, umem, false);
> done:
> 	mutex_unlock(&n->dev.mutex);
> 	return err;
>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>index ac55275fa0d0..427c4a34db2c 100644
>--- a/drivers/vhost/vdpa.c
>+++ b/drivers/vhost/vdpa.c
>@@ -898,7 +898,7 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> 	case VHOST_SET_OWNER:
> 		r = vhost_vdpa_bind_mm(v);
> 		if (r)
>-			vhost_dev_reset_owner(d, NULL);
>+			vhost_dev_reset_owner(d, NULL, false);
> 		break;
> 	}
> out:
>@@ -1396,7 +1396,7 @@ static void vhost_vdpa_cleanup(struct vhost_vdpa 
>*v)
> 	}
>
> 	vhost_vdpa_free_domain(v);
>-	vhost_dev_cleanup(&v->vdev);
>+	vhost_dev_cleanup(&v->vdev, false);
> 	kfree(v->vdev.vqs);
> 	v->vdev.vqs = NULL;
> }
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 4c525b3e16ea..b18429b15d32 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -719,7 +719,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> 	kfree(worker);
> }
>
>-static void vhost_workers_free(struct vhost_dev *dev)
>+static void vhost_workers_free(struct vhost_dev *dev, bool sync)
> {
> 	struct vhost_worker *worker;
> 	unsigned long i;
>@@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>
> 	for (i = 0; i < dev->nvqs; i++)
> 		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>+
>+	/*
>+	 * This path can be reached by lockless work queuers.  In this case
>+	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>+	 * RCU reader that fetched a worker before we cleared the pointers above
>+	 * may still be queueing work on it.  Wait for those readers to finish,
>+	 * then flush so any work they queued runs (clearing VHOST_WORK_QUEUED)
>+	 * before the workers are freed.  Notably, that is the case for the
>+	 * vsock RESET_OWNER path.
>+	 */
>+	if (sync) {
>+		synchronize_rcu();
>+		vhost_dev_flush(dev);
>+	}
>+
> 	/*
> 	 * Free the default worker we created and cleanup workers userspace
> 	 * created but couldn't clean up (it forgot or crashed).
>@@ -1148,11 +1163,12 @@ struct vhost_iotlb *vhost_dev_reset_owner_prepare(void)
> EXPORT_SYMBOL_GPL(vhost_dev_reset_owner_prepare);
>
> /* Caller should have device mutex */
>-void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
>+void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem,
>+			   bool sync)
> {
> 	int i;
>
>-	vhost_dev_cleanup(dev);
>+	vhost_dev_cleanup(dev, sync);
>
> 	dev->fork_owner = fork_from_owner_default;
> 	dev->umem = umem;
>@@ -1197,7 +1213,7 @@ void vhost_clear_msg(struct vhost_dev *dev)
> }
> EXPORT_SYMBOL_GPL(vhost_clear_msg);
>
>-void vhost_dev_cleanup(struct vhost_dev *dev)
>+void vhost_dev_cleanup(struct vhost_dev *dev, bool sync)
> {
> 	int i;
>
>@@ -1221,7 +1237,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> 	dev->iotlb = NULL;
> 	vhost_clear_msg(dev);
> 	wake_up_interruptible_poll(&dev->wait, EPOLLIN | EPOLLRDNORM);
>-	vhost_workers_free(dev);
>+	vhost_workers_free(dev, sync);
> 	vhost_detach_mm(dev);
> }
> EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
>diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>index 0192ade6e749..29fb1f510a34 100644
>--- a/drivers/vhost/vhost.h
>+++ b/drivers/vhost/vhost.h
>@@ -216,8 +216,9 @@ long vhost_dev_set_owner(struct vhost_dev *dev);
> bool vhost_dev_has_owner(struct vhost_dev *dev);
> long vhost_dev_check_owner(struct vhost_dev *);
> struct vhost_iotlb *vhost_dev_reset_owner_prepare(void);
>-void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *iotlb);
>-void vhost_dev_cleanup(struct vhost_dev *);
>+void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *iotlb,
>+			   bool sync);
>+void vhost_dev_cleanup(struct vhost_dev *dev, bool sync);
> void vhost_dev_stop(struct vhost_dev *);
> long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
> long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp);
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index d5022d21120b..87309c68af15 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -824,7 +824,7 @@ static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
>
> 	virtio_vsock_skb_queue_purge(&vsock->send_pkt_queue);
>
>-	vhost_dev_cleanup(&vsock->dev);
>+	vhost_dev_cleanup(&vsock->dev, false);
> 	put_net_track(vsock->net, &vsock->ns_tracker);
> 	kfree(vsock->dev.vqs);
> 	vhost_vsock_free(vsock);
>-- 
>2.47.1
>