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

[Stefano Garzarella <sgarzare@redhat.com>]

On Thu, Jul 23, 2026 at 05:29:12PM +0300, Andrey Drobyshev wrote:
>On 7/23/26 5:03 PM, Stefano Garzarella wrote:
>> On Thu, Jul 23, 2026 at 04:57:47PM +0300, Andrey Drobyshev wrote:
>>> On 7/22/26 12:43 PM, Stefano Garzarella 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 <sgarzare@redhat.com>
>>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>>>> ---
>>>>> drivers/vhost/vhost.c | 11 +++++++++++
>>>>> 1 file changed, 11 insertions(+)
>>>>
>>>> Sashiko reported some potential issues here:
>>>> https://sashiko.dev/#/patchset/20260720102241.371610-1-andrey.drobyshev@virtuozzo.com?part=4
>>>>
>>>> IMO the first one is pre-existing, but not really sure it is a real
>>>> issue since happening when the worker/vmm is going to be killed.
>>>>
>>>
>>> Sashiko claims:
>>>
>>>> Will this leave the queued work unexecuted and permanently break the
>>> virtqueue by leaving VHOST_WORK_QUEUED set?
>>>
>>> I agree this issue is pre-existing and doesn't have much to do with our
>>> series here.  It looks real, but in reality should be harmless since we
>>> may only hit it while the device already dying.  Means there's no VQ
>>> state to be saved.  The only potentially observable artifact I guess is
>>> a warning here:
>>>
>>> vhost_workers_free()
>>>  vhost_worker_destroy()
>>>    WARN_ON(!llist_empty())
>>>
>>> So more of a cosmetic noise on a dying device.  Again, not relevant to
>>> this series.  But one optional way to make it go away would be to clear
>>> vq->worker under vq->mutex in vhost_workers_free() (mirroring
>>> vhost_worker_killed()).
>>>> The second one also not sure if it's an issue since the sender is not
>>>> lockless IIUC.
>>>>
>>>
>>> The term 'sender' is confusing here:
>>>
>>> * vhost_transport_send_pkt() is the .send_pkt() method of struct
>>> virtio_transport.  It only stores skbs into the queue, doesn't process
>>> them. It indeed is lockless as it doesn't take vq->mutex.
>>>
>>> * vhost_transport_send_pkt_work() is the .fn() method of send_pkt_work.
>>> It's called by the worker thread to process skbs in the queue, calls
>>> vhost_transport_do_send_pkt() which does in turn take vq->mutex.
>>>
>>> I think sashiko points out to the former.  Still, I don't think it's an
>>> actual bug.  Look:
>>>
>>> 1) By invoking flush, we wake the worker thread:
>>> vhost_dev_flush()
>>>  __vhost_worker_flush()
>>>    vhost_worker_queue(flush.work)
>>>      worker->ops->wakeup()
>>>
>>> 2) Then woken worker does:
>>> vhost_run_work_list()
>>>  llist_for_each_entry_safe(work) {
>>>    clear_bit(VHOST_WORK_QUEUED, &work->flags)
>>>    work->fn(work) // for send_pkt_work = vhost_transport_send_pkt_work
>>>  }
>>>
>>> 3) And then in work->fn() (vhost_transport_send_pkt_work):
>>> vhost_transport_send_pkt_work()
>>>  vhost_transport_do_send_pkt()
>>>    if (!vhost_vq_get_backend(vq))
>>>      goto out;
>>>
>>> As you can see, we return early in case backend was unset.
>>>
>>> So after doing this flush, we have: 1) QUEUED bit is unset; that makes
>>> send_pkt_work re-queueable. 2) But the queue doesn't actually get
>>> processed, and VQ state isn't actually touched here - so I guess
>>> Sashiko's conclusion is incorrect and there's no actual bug.
>>>
>>>> But, please can you double check them?
>>>>
>>>
>>> In general I'd leave this patch as-is as Sashiko's complaints aren't
>>> very convincing so far.  If you want I can add another patch which wraps
>>> NULLifying workers in vhost_workers_free() in vq->mutex.
>>>
>>> WDYT?
>>
>> Yeah, I agree, about the other patch, up to you, but I'll eventually
>> send it separately.
>>
>> Thanks,
>> Stefano
>>
>
>Alright, then let me resend it once more along with this 6th patch, so
>that we don't have it uncovered.

But why it should be part of this series?

If there is no strong reason, I'd send it as a separate patch.

E.g. even this patch in theory may be a separate one, but this is 
related to this series, so makes sense to have this included.

Thanks,
Stefano