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

[Andrey Drobyshev]

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

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