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

[PATCH] 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.

This issue was found by Sashiko AI review.

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);
+	}
 	/*
 	 * Free the default worker we created and cleanup workers userspace
 	 * created but couldn't clean up (it forgot or crashed).
-- 
2.47.1

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

[Stefano Garzarella]

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
>

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

[Sean Christopherson]

On Thu, Aug 06, 2026, Stefano Garzarella wrote:
> On Thu, Jul 23, 2026 at 06:33:10PM +0300, Andrey Drobyshev wrote:
> > 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.

+1.  FWIW, I've been using and encouraging the "standard" Reported-by+Closes, e.g.

    Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
    Closes: https://lore.kernel.org/all/20260625212001.3B6561F000E9@smtp.kernel.org

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

[Stefano Garzarella]

On Thu, Aug 06, 2026 at 06:53:12AM -0700, Sean Christopherson wrote:
>On Thu, Aug 06, 2026, Stefano Garzarella wrote:
>> On Thu, Jul 23, 2026 at 06:33:10PM +0300, Andrey Drobyshev wrote:
>> > 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.
>
>+1.  FWIW, I've been using and encouraging the "standard" Reported-by+Closes, e.g.
>
>    Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
>    Closes: https://lore.kernel.org/all/20260625212001.3B6561F000E9@smtp.kernel.org
>

Yeah, this makes a lot of sense to me! I'll encourage the same.

Thanks,
Stefano

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@redhat.com> 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@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?

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

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

Thanks,
Stefano