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