On Thu, 6 Aug 2026 at 15:35, Stefano Garzarella <[email protected]> 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 <[email protected]> > >--- > > 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/[email protected]?part=1 So, yeah, I think it's a valid report we should fix. Thanks, Stefano
