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

[Stefano Garzarella <sgarzare@redhat.com>]

On Thu, Jul 23, 2026 at 06:33:10PM +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.

mmm, vhost_dev_cleanup() updates vq->worker without the mutex too IIUC.

>
>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.
>
>This issue was found by Sashiko AI review.

Can you share a link to the review?

I don't know if it's common or not, but having the link in the commit or 
after --- will help the reviewers.

>
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
> drivers/vhost/vhost.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 4c525b3e16ea..dbb6cb5eccea 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -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);
>+	}

Pre-existing, but IIUC vhost_workers_free() is called only by 
vhost_dev_cleanup() at the bottom, after a loop calls vhost_vq_reset() 
on each virtqueue (without the mutex) where we already set `vq->worker` 
to NULL, so IMO at this point it's already NULL, no?

Thanks,
Stefano

> 	/*
> 	 * Free the default worker we created and cleanup workers userspace
> 	 * created but couldn't clean up (it forgot or crashed).
>-- 
>2.47.1
>