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 <[email protected]>
>>>> Signed-off-by: Andrey Drobyshev <[email protected]>
>>>> ---
>>>> 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
>>>
>
