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

On Tue, Jul 14, 2026 at 06:16:37PM +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.
>
>Close this the way vhost_worker_killed() already does: clear the
>vq->worker pointers, wait for a grace period, run whatever the last
>readers may have queued, and only then free the workers.  The
>synchronize_rcu() is skipped if the device has no workers, so cleanup of
>devices which never got an owner stays cheap.
>

Do we need a Fixes tag for this?

Thanks for pointing out that the issue wasn't occurring, but I think we 
should add it because it's a sneaky problem we discovered by chance.
IMO the code should already have `synchronize_rcu()` after 
`rcu_assign_pointer()` loop.

@Michael, what do you think?

>Suggested-by: Stefano Garzarella <[email protected]>
>Signed-off-by: Andrey Drobyshev <[email protected]>
>---
> drivers/vhost/vhost.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 4c525b3e16ea..0d1414d40f4e 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -729,6 +729,21 @@ 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
>+	 * caller that fetched a worker before we cleared the pointers above
>+	 * may still be about to queue work on it.  Wait for those RCU readers
>+	 * to finish before freeing the worker, then run whatever they queued
>+	 * so nothing is left with VHOST_WORK_QUEUED set.  Mirrors
>+	 * vhost_worker_killed().
>+	 */
>+	if (!xa_empty(&dev->worker_xa)) {
>+		synchronize_rcu();
>+		xa_for_each(&dev->worker_xa, i, worker)
>+			vhost_run_work_list(worker);
>+	}
>+

Following sashiko review [1], I tried to undersand why we need this, but 
TBH I'm really confused. That said, this seems wrong also because it 
will work only with vhost_tasks, and not with kthreads.

IIUC vhost_worker_killed() will be called anyway when calling 
vhost_worker_destroy(). For vhost_tasks, it will call 
vhost_task_do_stop() that calls vhost_task_stop(). This sets 
VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing 
stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that 
is exactly vhost_worker_killed() you mentioned we are mirroring here.

So, why we need this?

Should be enough to call synchronize_rcu() in any case after the 
rcu_assign_pointer() loop?

Thanks,
Stefano

[1] 
https://sashiko.dev/#/patchset/[email protected]?part=4