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

[Stefano Garzarella]

On Thu, 6 Aug 2026 at 15:35, Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx>
> >---
> > 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?

Oh, sashiko reported pretty much the same
https://sashiko.dev/#/patchset/20260723153310.745855-1-andrey.drobyshev@xxxxxxxxxxxxx?part=1

So, yeah, I think it's a valid report we should fix.

Thanks,
Stefano