Re: [PATCH v5 4/5] vhost: synchronize with RCU readers when freeing workers

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

On 7/23/26 7:50 PM, Michael S. Tsirkin wrote:
> On Thu, Jul 23, 2026 at 07:46:58PM +0300, Andrey Drobyshev wrote:
>> On 7/23/26 6:18 PM, Michael S. Tsirkin wrote:
>>> On Mon, Jul 20, 2026 at 01:22:40PM +0300, Andrey Drobyshev wrote:
>>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>>>> vq->worker and queues work on it.  vhost_workers_free() however clears
>>>> the vq->worker pointers and immediately frees the workers, without
>>>> waiting for a grace period.  A caller that fetched the worker right
>>>> before the pointer was cleared can therefore still be queueing work on
>>>> it while it is freed.  And even when the queueing itself wins the race,
>>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>>>> future attempts to queue it are silently skipped.
>>>>
>>>> None of the current callers can actually hit this: net and scsi stop
>>>> their virtqueues before the workers are freed, and vsock unhashes the
>>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>>>> before the workers go away.  But the upcoming VHOST_RESET_OWNER support
>>>> in vhost-vsock keeps the device hashed while its workers are freed, so
>>>> the lockless send/cancel paths become able to race with the teardown.
>>>>
>>>> Fix this by clearing the vq->worker pointers, waiting for a grace
>>>> period, and then flushing the workers so any work the last readers
>>>> queued runs before the workers are freed.
>>>
>>>
>>>
>>>
>>>> Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>>> ---
>>>>  drivers/vhost/vhost.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>> index 4c525b3e16ea..d6e235c25254 100644
>>>> --- a/drivers/vhost/vhost.c
>>>> +++ b/drivers/vhost/vhost.c
>>>> @@ -729,6 +729,17 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>>>  
>>>>  	for (i = 0; i < dev->nvqs; i++)
>>>>  		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>>>> +
>>>> +	/*
>>>> +	 * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>>>> +	 * reader that fetched a worker before we cleared the pointers above
>>>> +	 * may still be queueing work on it.  Wait for those readers to
>>>> +	 * finish, then flush so any work they queued runs (clearing
>>>> +	 * VHOST_WORK_QUEUED) before the workers are freed.
>>>> +	 */
>>>> +	synchronize_rcu();
>>>
>>>
>>>
>>> Any way not to add this for all devices that don't need it?
>>> Or preferably, even for vsock in absense of the new ioctl?
>>>
>>
>> This code was initially local to vsock, and was moved here in v3->v4
>> after we discussed with Stefano that the issue looks more generic and
>> should probably be fixed in vhost.c (see
>> https://lore.kernel.org/virtualization/akO6tps94iFxCAWv@sgarzare-redhat).
>>
>> As a compromise, we can keep this code here, but only call it
>> conditionally.  Namely, create a bool flag on 'struct vhost_dev' which
>> is always false, only set it to true on RESET_OWNER, and only call this
>> code once it's set.  Clumsy, but this way no other code path would have
>> to wait the full grace period.
>>
>> What are your thoughts on that?
>>
>> Thanks,
>> Andrey
> 
> Or just thread a bool parameter to it?
>

Yep, that should work as well.  So we'll have to do:

vhost_dev_cleanup(struct vhost_dev *dev, bool sync)
  vhost_workers_free(struct vhost_dev *dev, bool sync)
    if (sync) {
      synchronize_rcu();
      vhost_dev_flush(dev);
    }

And then vhost_dev_reset_owner() will call vhost_dev_cleanup() with
sync=true, while everybody else (net_release, scsi_release, even
vhost_vsock_dev_release) will call it with sync=false.

I'll squash it all into the same 4th commit and resend, unless you have
any objections.

Thanks,
Andrey


>>>
>>>> +	vhost_dev_flush(dev);
>>>> +
>>>>  	/*
>>>>  	 * Free the default worker we created and cleanup workers userspace
>>>>  	 * created but couldn't clean up (it forgot or crashed).
>>>> -- 
>>>> 2.47.1
>>>
>