On Thu, 23 Jul 2026 at 16:50, Andrey Drobyshev <[email protected]> wrote: > > 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 <[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 > >>> > >> > >> 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?
Yep, I mean this should go with Michael's tree, so ... :-) I sent my R-b to all patches, so I'm fine with this version. Thanks, Stefano
