On 7/16/26 7:13 PM, Stefano Garzarella wrote:
> 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.
>
But will it actually be handled?
vhost_transport_send_pkt() // called on every packet send
virtio_vsock_skb_queue_tail(&send_pkt_queue, skb) // add skb to list
vhost_vq_work_queue(&send_pkt_work) // try to arm the work
vhost_worker_queue()
if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
llist_add(&worker->work_list)
}
So send_pkt_queue is a list of skbs, it lives on the vhost_vsock device
state, and survives RESET_OWNER. In that sense you're probably right
that we aren't loosing any data.
There's also send_pkt_work object, also living on the vhost_vsock device
state. So we're accumulating skbs, and then send_pkg_work gets put in
the worker task list - but only if it's NOT already armed in there, i.e.
QUEUED bit is unset. And the bit gets cleared by the workload callback
- for vhost_task backend it's vhost_run_work_list().
The most important thing is WHERE this piece of work is being put. That
is worker->work_list - this list does not survive RESET_OWNER, as we
free the worker in vhost_workers_free().
Now imagine we have RESET_OWNER racing with send_pkt. In
vhost_workers_free() we acquire ptr to a worker but not NULL'ify it yet.
Then on the send_pkt path we arm the send_pkt_work, set the QUEUED bit,
and place it on the work_list of a DYING worker. Then the worker gets
freed. Now we have send_pkt_work (a singleton struct) with QUEUED set
in its flags, and with no worker to walk through this piece of work and
clear this flag. As a result - send_pkt_work can't be placed in the
list of any other worker, because it doesn't pass the "if
(!test_and_set_bit(QUEUED)" check. Thus no new packets can be
processed, and the connection is stalled.
Does this make sense?
>>
>> 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.
>
Agreed.
> 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.
>
Note that in our particular case we're gonna do:
vhost_workers_free()
vhost_dev_flush() // the flush we're planning to add
xa_for_each(&dev->worker_xa, i, worker)
vhost_worker_destroy(dev, worker)
xa_destroy(&dev->worker_xa)
So we walk through the XArray, destroy workers in it one by one, then
destroy the XArray itself. Then the next time we call
vhost_dev_flush(), e.g. from vhost_dev_stop() or wherever else, it tries
iterating over the XArray which no longer exists - which is gonna be a
no-op.
Now, we can reach vhost_workers_free() via (at least) 2 paths:
RESET_OWNER and device release path. On the former the flush is needed
as I illustrated above. On the latter it's indeed redundant but is
cheap as it's a no-op.
Andrey
> Thanks,
> Stefano
>
