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

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

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.

Andrey