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

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 <[email protected]>
>Closes: https://lore.kernel.org/kvm/[email protected]
>Link: https://lore.kernel.org/kvm/[email protected]
>Signed-off-by: Andrey Drobyshev <[email protected]>
>---
>
>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/[email protected]
>
> 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 <[email protected]>