On Thu, 5 Nov 2026 12:20:26 +0100 Sriram Yagnaraman <[email protected]> wrote:
> The control channel can be torn down from two threads: the EAL interrupt > thread (memif_intr_handler on peer disconnect/error) and an application > thread (dev_stop/dev_close -> memif_disconnect). Both race to unregister > the callback and free the channel, which memif_pmd_autotest SIGSEGVs on > during EAL teardown (seen on amd64 and arm64). > > Rework teardown around a single owner: > > - memif_disconnect() clears pmd->cc under cc_lock; whoever clears it > owns the free. > - Device teardown (link down, flags, queue eventfds, regions) moves to > memif_disconnect_queues(), run by whichever thread owns the > disconnect, so flag/link state is left consistent for reconnect. > - memif_cc_release() frees the channel: unregister_pending() on the > interrupt thread (deferred free, drops the fd from epoll after > dispatch), unregister_sync() on an application thread. > - memif_intr_handler() self-unregisters on disconnect so the EOF > (level-triggered) socket fd cannot re-fire (no livelock). > > Reported-by: Stephen Hemminger <[email protected]> > Suggested-by: Stephen Hemminger <[email protected]> > Signed-off-by: Sriram Yagnaraman <[email protected]> > --- AI found an error here.. [PATCH v2] net/memif: redesign control channel teardown Error: the application thread no longer waits for the handler once the handler has taken ownership, so the reported SIGSEGV is still reachable. owned = (pmd->cc == cc); if (owned) pmd->cc = NULL; ... if (owned) memif_disconnect_queues(cc->dev); Peer hangs up, handler clears pmd->cc and runs memif_disconnect_queues() after dropping cc_lock. Meanwhile dev_stop -> memif_disconnect() finds cc == NULL, skips memif_cc_release() and calls memif_disconnect_queues(dev) concurrently. memif_free_regions() then does munmap/close/rte_free on the same regions from two threads. dev_close followed by port release frees dev->data while the handler is still inside memif_disconnect_queues(cc->dev), which is the original memif_pmd_autotest crash. The "Idempotent ... safe to run from whichever thread" comment is not true; idempotent is not thread safe. Do the device teardown under cc_lock in both paths, and have the handler touch only cc after it drops the lock: /* handler */ rte_spinlock_lock(&pmd->cc_lock); owned = (pmd->cc == cc); if (owned) { pmd->cc = NULL; memif_disconnect_queues(cc->dev); } rte_spinlock_unlock(&pmd->cc_lock); rte_intr_callback_unregister_pending(cc->intr_handle, memif_intr_handler, cc, owned ? memif_intr_unregister_handler : NULL); /* memif_disconnect(), after memif_cc_release() */ rte_spinlock_lock(&pmd->cc_lock); memif_disconnect_queues(dev); rte_spinlock_unlock(&pmd->cc_lock); Error: pmd->cc is set to NULL while the handler may still be running and the handler dereferences pmd->cc. cc = pmd->cc; pmd->cc = NULL; The application thread then waits in rte_intr_callback_unregister_sync() on the assumption the handler is safe. It is not: memif_msg_receive() on HELLO/INIT/ADD_REGION/ADD_RING/CONNECT calls memif_msg_enq_ack(), memif_msg_enq_init(), memif_msg_enq_add_region(), memif_msg_enq_add_ring(), memif_msg_enq_connect() and memif_msg_enq_connected(), all of which do struct memif_msg_queue_elt *e = memif_msg_enq(pmd->cc); and memif_msg_enq() does TAILQ_INSERT_TAIL(&cc->msg_queue, ...) on NULL. Pass the channel down instead of re-reading pmd->cc on the interrupt thread, e.g. memif_msg_enq_ack(cc) using cc->dev for pmd. The channel stays valid until unregister_sync() returns. Error: memif_cc_release() leaks the channel when memif_disconnect() runs on the interrupt thread for a source that is not dispatching. if (rte_thread_is_intr()) { ret = rte_intr_callback_unregister_pending(ih, memif_bad_desc_disconnect() is an EAL alarm, and alarms run on the interrupt thread. The active source is the alarm timerfd, not the control socket, so unregister_pending() returns -EAGAIN. pmd->cc is already NULL, the callback stays registered and cc is never freed. When the peer later closes, the handler sees owned == false and passes a NULL free callback, so cc and its fd leak; if the port was released first, the handler dereferences cc->dev->data after free. The old code tried rte_intr_callback_unregister() first. Keep that: if (rte_thread_is_intr()) { ret = rte_intr_callback_unregister(ih, memif_intr_handler, cc); if (ret == -EAGAIN) ret = rte_intr_callback_unregister_pending(ih, memif_intr_handler, cc, memif_intr_unregister_handler); else if (ret > 0) memif_intr_unregister_handler(ih, cc); } Warning: on a protocol error the peer no longer gets the DISCONNECT message with the reason. memif_disconnect_queues(cc->dev); memif_msg_receive() queues a disconnect ("Invalid message size", "Unknown message type", "Incorrect secret", ...) and returns -1, which goes straight to the disconnect label. The old code called memif_disconnect(), which dropped the other queued messages and sent the DISCONNECT. The new handler path never calls memif_msg_send_from_queue(), and memif_intr_unregister_handler() frees the queued message. The peer now sees a bare EOF. Flush the disconnect message in the owned branch before the teardown. Warning: v1 had "Fixes: 09c7e63a71f9" and "Cc: [email protected]"; v2 dropped both. The race is in released versions; restore them. Info: pre-existing, not introduced by this patch. The single-owner model needs every write of pmd->cc under cc_lock, but memif_msg_receive_init() still does pmd->cc = cc; without the lock. A server port being stopped can have a new channel attached after memif_disconnect() has already looked at pmd->cc.
