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 <[email protected]>
Signed-off-by: Andrey Drobyshev <[email protected]>
---
drivers/vhost/vhost.c | 11 +++++++++++
1 file changed, 11 insertions(+)
Sashiko reported some potential issues here:
https://sashiko.dev/#/patchset/[email protected]?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
