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 <[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? > 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 >> >
