[PATCH v2] vhost: clear vq->worker under vq->mutex when freeing workers

[PATCH v2] vhost: clear vq->worker under vq->mutex when freeing workers

[Andrey Drobyshev]

Every other update of vq->worker is done under vq->mutex - the worker
attach/swap ioctls and vhost_worker_killed().  vhost_workers_free() is
the sole exception: it clears vq->worker without holding the lock.

The effect is harmless in practice, as this only happens while the
owning process (and thus the whole device) is dying, but the lockless
write is inconsistent with the rest of the code.  Clear vq->worker under
vq->mutex, like everyone else, so that all writers of vq->worker follow
the same locking rule.

vhost_vq_reset() also used to clear vq->worker locklessly, earlier on
the same teardown path, making the write in vhost_workers_free()
redundant.  Drop the clear from vhost_vq_reset() and initialize the
pointer in vhost_dev_init() instead, so that vhost_workers_free() is the
only place clearing vq->worker on teardown.  Any work queued while
vq->worker is still set is drained by the synchronize_rcu() +
vhost_dev_flush() in vhost_workers_free() before the workers are freed.

Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/kvm/20260721102325.1BD6C1F00A3A@smtp.kernel.org
Link: https://lore.kernel.org/kvm/20260724153334.1BCBF1F000E9@smtp.kernel.org
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---

v1 -> v2:

  * Drop the lockless vq->worker clear from vhost_vq_reset(), initialize
    vq->worker in vhost_dev_init() instead;
  * Rebase onto Michael's mst/linux-next tree (with my previously merged
    vsock patches);
  * Add links to Sashiko reports and adjust commit message.

v1: https://lore.kernel.org/kvm/20260723153310.745855-1-andrey.drobyshev@virtuozzo.com

 drivers/vhost/vhost.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a0c1d54019aa..5764a54ddc92 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -392,7 +392,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->busyloop_timeout = 0;
 	vq->umem = NULL;
 	vq->iotlb = NULL;
-	rcu_assign_pointer(vq->worker, NULL);
 	vhost_vring_call_reset(&vq->call_ctx);
 	__vhost_vq_meta_reset(vq);
 }
@@ -613,6 +612,7 @@ void vhost_dev_init(struct vhost_dev *dev,
 		vq->heads = NULL;
 		vq->nheads = NULL;
 		vq->dev = dev;
+		RCU_INIT_POINTER(vq->worker, NULL);
 		mutex_init(&vq->mutex);
 		vhost_vq_reset(dev, vq);
 		if (vq->handle_kick)
@@ -722,13 +722,19 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 static void vhost_workers_free(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
+	struct vhost_virtqueue *vq;
 	unsigned long i;
 
 	if (!dev->use_worker)
 		return;
 
-	for (i = 0; i < dev->nvqs; i++)
-		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+	for (i = 0; i < dev->nvqs; i++) {
+		vq = dev->vqs[i];
+
+		mutex_lock(&vq->mutex);
+		rcu_assign_pointer(vq->worker, NULL);
+		mutex_unlock(&vq->mutex);
+	}
 
 	/*
 	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
-- 
2.47.1

Re: [PATCH v2] vhost: clear vq->worker under vq->mutex when freeing workers

[Stefano Garzarella]

On Wed, Aug 19, 2026 at 02:43:28PM +0300, Andrey Drobyshev wrote:
>Every other update of vq->worker is done under vq->mutex - the worker
>attach/swap ioctls and vhost_worker_killed().  vhost_workers_free() is
>the sole exception: it clears vq->worker without holding the lock.
>
>The effect is harmless in practice, as this only happens while the
>owning process (and thus the whole device) is dying, but the lockless
>write is inconsistent with the rest of the code.  Clear vq->worker under
>vq->mutex, like everyone else, so that all writers of vq->worker follow
>the same locking rule.
>
>vhost_vq_reset() also used to clear vq->worker locklessly, earlier on
>the same teardown path, making the write in vhost_workers_free()
>redundant.  Drop the clear from vhost_vq_reset() and initialize the
>pointer in vhost_dev_init() instead, so that vhost_workers_free() is the
>only place clearing vq->worker on teardown.  Any work queued while
>vq->worker is still set is drained by the synchronize_rcu() +
>vhost_dev_flush() in vhost_workers_free() before the workers are freed.
>
>Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
>Closes: https://lore.kernel.org/kvm/20260721102325.1BD6C1F00A3A@smtp.kernel.org
>Link: https://lore.kernel.org/kvm/20260724153334.1BCBF1F000E9@smtp.kernel.org
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
>
>v1 -> v2:
>
>  * Drop the lockless vq->worker clear from vhost_vq_reset(), initialize
>    vq->worker in vhost_dev_init() instead;
>  * Rebase onto Michael's mst/linux-next tree (with my previously merged
>    vsock patches);
>  * Add links to Sashiko reports and adjust commit message.
>
>v1: https://lore.kernel.org/kvm/20260723153310.745855-1-andrey.drobyshev@virtuozzo.com
>
> drivers/vhost/vhost.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index a0c1d54019aa..5764a54ddc92 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -392,7 +392,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> 	vq->busyloop_timeout = 0;
> 	vq->umem = NULL;
> 	vq->iotlb = NULL;
>-	rcu_assign_pointer(vq->worker, NULL);
> 	vhost_vring_call_reset(&vq->call_ctx);
> 	__vhost_vq_meta_reset(vq);
> }
>@@ -613,6 +612,7 @@ void vhost_dev_init(struct vhost_dev *dev,
> 		vq->heads = NULL;
> 		vq->nheads = NULL;
> 		vq->dev = dev;
>+		RCU_INIT_POINTER(vq->worker, NULL);
> 		mutex_init(&vq->mutex);
> 		vhost_vq_reset(dev, vq);
> 		if (vq->handle_kick)
>@@ -722,13 +722,19 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
> static void vhost_workers_free(struct vhost_dev *dev)
> {
> 	struct vhost_worker *worker;
>+	struct vhost_virtqueue *vq;

nit: `vq` is used only in the for loop, so you can move this declaration 
inside the loop. (I'm not asking to respin for just this)

> 	unsigned long i;
>
> 	if (!dev->use_worker)
> 		return;)
>
>-	for (i = 0; i < dev->nvqs; i++)
>-		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>+	for (i = 0; i < dev->nvqs; i++) {
>+		vq = dev->vqs[i];
>+
>+		mutex_lock(&vq->mutex);
>+		rcu_assign_pointer(vq->worker, NULL);
>+		mutex_unlock(&vq->mutex);
>+	}
>

LGTM!

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