* Re: [PATCH] vhost: clear vq->worker under vq->mutex when freeing workers
2026-08-06 13:35 ` Stefano Garzarella
@ 2026-08-06 13:53 ` Sean Christopherson
2026-08-06 14:12 ` Stefano Garzarella
2026-08-06 14:40 ` Stefano Garzarella
2026-08-19 10:47 ` Andrey Drobyshev
2 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2026-08-06 13:53 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Andrey Drobyshev, linux-kernel, kvm, virtualization, netdev, mst,
stefanha, jasowangio, eperezma
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vhost: clear vq->worker under vq->mutex when freeing workers
2026-08-06 13:53 ` Sean Christopherson
@ 2026-08-06 14:12 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2026-08-06 14:12 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andrey Drobyshev, linux-kernel, kvm, virtualization, netdev, mst,
stefanha, jasowangio, eperezma
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: clear vq->worker under vq->mutex when freeing workers
2026-08-06 13:35 ` Stefano Garzarella
2026-08-06 13:53 ` Sean Christopherson
@ 2026-08-06 14:40 ` Stefano Garzarella
2026-08-19 10:47 ` Andrey Drobyshev
2 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2026-08-06 14:40 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: linux-kernel, kvm, virtualization, netdev, mst, stefanha,
jasowangio, eperezma
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vhost: clear vq->worker under vq->mutex when freeing workers
2026-08-06 13:35 ` Stefano Garzarella
2026-08-06 13:53 ` Sean Christopherson
2026-08-06 14:40 ` Stefano Garzarella
@ 2026-08-19 10:47 ` Andrey Drobyshev
2 siblings, 0 replies; 6+ messages in thread
From: Andrey Drobyshev @ 2026-08-19 10:47 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-kernel, kvm, virtualization, netdev, mst, stefanha,
jasowangio, eperezma
Hello Stefano!
Sorry for the delay on this one.
On 8/6/26 4:35 PM, Stefano Garzarella 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.
>
Sure, will add the link.
>>
>> 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?
>
You and Sashiko are right, thanks for pointing out. Previous workers'
update in vhost_vq_reset() makes both our update and our locking
redundant. I think we should just leave only one of those functions
updating workers.
I suggest we leave vhost_workers_free() as-is (this patch), while
dropping rcu_assign_pointer() from vhost_vq_reset(). Two issues with that:
1. vhost_vq_reset() is also called from vhost_dev_init(), as an
initializer. But we can simply add workers' initialization to
vhost_dev_init().
2. In vhost_dev_cleanup(), we'd have had workers already nullified
between vhost_vq_reset() and vhost_workers_free() calls. After
suggested fix - not anymore. So that's a behaviour change. But I don't
see anybody else touching workers before vhost_workers_free() on cleanup
path, so AFAICT it looks safe.
Another note: I initially based this patch on master branch, which is
wrong as it doesn't cleanly apply on top of our previously merged vsock
patches. I'll on Michael's mst/linux-next tree and send v2.
Thanks,
Andrey
> 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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread