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

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

On 7/23/26 5:39 PM, Stefano Garzarella wrote:
> 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.
> 

No particular reason, just while we're at it.  Alright, no respin, will
send it separately.  Then I'll consider this series reviewed - unless,
maybe, Michael will want to take a look as well?

Thanks,
Andrey