On 7/16/26 11:57 AM, Stefano Garzarella wrote:
> 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?
>
I'm guessing it should be:
Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
> 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.
>
Hmm, are we sure it's the case for our codepath? Looking at the
vhost_task loop function:
> static int vhost_task_fn(void *data)
>
> {
> for (;;) {
>
>
> if (signal_pending(current)) {
> if (get_signal(&ksig))
>
> break;
>
> }
>
> ...
> if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
> __set_current_state(TASK_RUNNING);
> break;
> }
>
> did_work = vtsk->fn(vtsk->data);
>
> ...
> }
>
>
> ...
>
> if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>
> set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
>
> vtsk->handle_sigkill(vtsk->data);
>
> }
>
> ...
> }
AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
setting. Like you said, STOP is set by vhost_task_stop. E.g. for our
RESET_OWNER case:
vhost_vsock_reset_owner()
vhost_dev_reset_owner()
vhost_dev_cleanup()
vhost_workers_free()
vhost_worker_destroy()
vhost_task_stop() // for vhost_task_ops backend
set_bit(VHOST_TASK_FLAGS_STOP)
So, first of all, actual work by .fn() callback is done after the exit
checks, therefore we skip it - no chance to drain there.
Secondly, the handle_sigkill() callback is deliberately NOT called in
the STOP case and only called on fatal signal delivery. And for
vhost_task backend the .handle_sigkill() callback is exactly
vhost_worker_killed().
So my understanding is: if we only call synchronize_rcu() here and leave
this path undrained, then whatever work which was put by send_pkt() for
the worker currently being freed - will be lost. Please correct me if
I'm wrong.
That said, I agree that vhost_run_work_list() will only work with
vhost_task backend, not with kthreads backend. If we do
vhost_worker_flush() instead - I guess it'll keep the drain here, yet
become backend-agnostic. I.e.:
> + if (!xa_empty(&dev->worker_xa)) {
> + synchronize_rcu();
> + xa_for_each(&dev->worker_xa, i, worker)
> + vhost_worker_flush(worker);
> + }
With the last 2 lines being equivalent to just calling
vhost_dev_flush(dev). And once we become backend-agnostic here, I'm
guessing the warning reported by Sashiko should be dealt with as well.
WDYT?
Andrey
> 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
>
