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

On Thu, Jul 16, 2026 at 06:39:48PM +0300, Andrey Drobyshev wrote:
>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.

Yep, your right. But what will be the issue of loosing them?

IIUC we are not loosing any data, just avoiding some works that will be 
handled later when/if will set a new owner.

>
>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.

I'd avoid `if !xa_empty(&dev->worker_xa)` at all, and call 
synchronize_rcu() in any case.

About vhost_dev_flush(), we are calling it in several places, and maybe 
we should re-check them. E.g. we call in vhost_vsock_flush(), but it's 
also called by vhost_dev_stop(), maybe we can avoid to call 
vhost_vsock_flush() if we call vhost_dev_stop().

I'm not sure we really need another one here, but if you think some 
other works can be queued between the vhost_dev_stop() and the 
synchronize_rcu() we are adding here, then okay, it may have sense.

Thanks,
Stefano