> -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Sunday, 25 October 2026 18:01 > To: Sriram Yagnaraman <[email protected]> > Cc: [email protected]; Jakub Grajciar <[email protected]>; [email protected] > Subject: Re: [PATCH v4 9/9] test/memif: add memif PMD unit test > > On Fri, 16 Oct 2026 21:58:57 +0200 > Sriram Yagnaraman <[email protected]> wrote: > > > Add a memif_pmd_autotest unit test, modelled on the TAP PMD test. > > memif differs from TAP in that a port is only usable once a peer > > connects, so the test creates a server and a client memif port in the > > same process, connected over an abstract-namespace socket (the > > loopback setup from the memif documentation), and waits for the > > asynchronous control-channel handshake to bring both ports up. > > > > The suite checks dev_info reports the memif driver, that the pair > > reaches link up, statistics get and reset, a client-to-server loopback > > burst with payload verification on both the slow and the single-pool > > fast path (a second default-bsize pair), multi-segment loopback across > > a ring wrap with payload verification (chained-descriptor Rx and Tx), > > that the pair reconnects after a port restart, that a zero-length Tx > > burst is handled, that a configured-but-not-started port can be closed > > without crashing, and that a server stopped after start refuses a > > peer. It is a fast test, runs without hugepages and under ASan. > > > > Signed-off-by: Sriram Yagnaraman <[email protected]> > > --- > > This test is getting lots of CI failures. > > AI says it found the answer. The analysis is likely correct, but the fix looks > awkward. > > From 9a0d594135e21e949e814df036d30d70ff93e45b Mon Sep 17 00:00:00 > 2001 > From: Stephen Hemminger <[email protected]> > Date: Sun, 25 Oct 2026 09:45:50 -0700 > Subject: [PATCH] net/memif: fix race between interrupt thread and stop > > memif_disconnect() can run in the interrupt thread, from the control channel > handler on peer EOF, at the same time as dev_stop/dev_close in the > application thread. When the handler is active the application side only > marks the callback pending and carries on freeing queues, regions and finally > the port. The handler then calls memif_disconnect() on a released ethdev > and crashes on dev->data. > > Seen as a SIGSEGV in memif_pmd_autotest teardown, where stopping the > server makes the client channel see EOF while the test is stopping and closing > the client. > > When called from an application thread, detach the channel under cc_lock > and wait for the handler with > rte_intr_callback_unregister_sync() before tearing down. The handler drops > its own callback if it finds the channel detached, otherwise the EOF keeps the > fd readable and the wait never completes. Hold cc_lock over the queue and > region teardown so the two paths cannot run it concurrently. > > Close the channel fd through its own handle when freeing it; the handle given > to the pending unregister callback is the EAL copy. > > Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD") > Cc: [email protected] > > Signed-off-by: Stephen Hemminger <[email protected]> > Co-Authored-By: Claude Opus 5.5 <[email protected]> > Claude-Session: > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fclaude. > ai%2Fcode%2Fsession_01FA4k3HfYcHiM1tu71SfXb1&data=05%7C02%7Csrira > m.yagnaraman%40ericsson.com%7Cf1e8aaf65f3745c8e82208df32b98aa7%7 > C92e84cebfbfd47abbe52080c6b87953f%7C0%7C0%7C639285444622358195% > 7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuM > DAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7 > C%7C&sdata=iKB%2BMRPw352jKwZQGBZlRGaZa07PINx5QR50NJ4aduM%3D& > reserved=0
I am cheap and don't have a Claude subscription yet, so could not access the above session. But I used my corporate subscription (incidentally using Claude Opus as well 😊) to analyze and review the patch The analysis is spot on, on the root cause. I was able to reproduce the SIGSEGV running the test in a loop x100 times. With your patch I couldn't reproduce it. I got AI to make a variant of the fix here; you are welcome to use either version to post a proper patch. From 47cbd5064033eca40a14301b99c4cace2d60fd71 Mon Sep 17 00:00:00 2001 From: Sriram Yagnaraman <[email protected]> Date: Tue, 27 Oct 2026 10:25:00 +0100 Subject: [PATCH] net/memif: fix race between interrupt thread and stop memif_disconnect() can run in the interrupt thread, from the control channel handler on peer EOF, at the same time as dev_stop/dev_close in the application thread. When the handler was active the application side only marked the callback pending and carried on freeing queues, regions and finally the port; the handler then dereferenced dev->data on a released ethdev and crashed. Seen as a SIGSEGV in memif_pmd_autotest teardown, where stopping the server makes the client channel see EOF while the test is stopping and closing the client. Detach the control channel under cc_lock, then, when called from an application thread, wait for the handler with rte_intr_callback_unregister_sync() before tearing anything down. The handler drops its own callback once it sees the channel detached (pmd->cc != cc), so the EOF that keeps the fd readable does not stall the wait. When called from the interrupt thread the handler cannot wait for itself, so it hands the free to the pending unregister callback. The teardown is split out into memif_cc_detach(), which has a single "call without cc_lock held" contract; once it returns the handler can no longer run for the device, so the queue and region teardown in memif_disconnect() needs no further locking. Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD") Cc: [email protected] Suggested-by: Stephen Hemminger <[email protected]> Signed-off-by: Sriram Yagnaraman <[email protected]> --- drivers/net/memif/memif_socket.c | 146 +++++++++++++++++++++---------- 1 file changed, 98 insertions(+), 48 deletions(-) diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c index 971964133c..70529b657d 100644 --- a/drivers/net/memif/memif_socket.c +++ b/drivers/net/memif/memif_socket.c @@ -572,84 +572,117 @@ memif_msg_enq_connected(struct rte_eth_dev *dev) } static void -memif_intr_unregister_handler(const struct rte_intr_handle *intr_handle, void *arg) +memif_cc_free(struct memif_control_channel *cc) { struct memif_msg_queue_elt *elt; - struct memif_control_channel *cc = arg; - RTE_SET_USED(intr_handle); - /* close control channel fd */ rte_intr_fd_close(cc->intr_handle); - /* clear message queue */ while ((elt = TAILQ_FIRST(&cc->msg_queue)) != NULL) { TAILQ_REMOVE(&cc->msg_queue, elt, next); rte_free(elt); } rte_intr_instance_free(cc->intr_handle); - /* free control channel */ rte_free(cc); } -void -memif_disconnect(struct rte_eth_dev *dev) +/* + * Pending unregister callback, run by the interrupt thread once + * memif_intr_handler() has returned. The handle passed in is the + * EAL copy, close the fd through the control channel's own handle. + */ +static void +memif_intr_unregister_handler(const struct rte_intr_handle *intr_handle, void *arg) { - struct pmd_internals *pmd = dev->data->dev_private; + RTE_SET_USED(intr_handle); + memif_cc_free(arg); +} + +/* + * Detach the control channel from the device and free it, synchronising + * with memif_intr_handler() which may be running in the interrupt thread. + * Must be called WITHOUT pmd->cc_lock held (the handler takes it). On + * return the handler can no longer run for this device. + */ +static void +memif_cc_detach(struct pmd_internals *pmd) +{ + struct memif_control_channel *cc; struct memif_msg_queue_elt *elt, *next; - struct memif_queue *mq; - struct rte_intr_handle *ih; - int i; int ret; - dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN; - pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTING; - pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTED; - rte_spinlock_lock(&pmd->cc_lock); - if (pmd->cc != NULL) { + cc = pmd->cc; + pmd->cc = NULL; + if (cc != NULL) { /* Clear control message queue (except disconnect message if any). */ - for (elt = TAILQ_FIRST(&pmd->cc->msg_queue); elt != NULL; elt = next) { + for (elt = TAILQ_FIRST(&cc->msg_queue); elt != NULL; elt = next) { next = TAILQ_NEXT(elt, next); if (elt->msg.type != MEMIF_MSG_TYPE_DISCONNECT) { - TAILQ_REMOVE(&pmd->cc->msg_queue, elt, next); + TAILQ_REMOVE(&cc->msg_queue, elt, next); rte_free(elt); } } /* send disconnect message (if there is any in queue) */ - memif_msg_send_from_queue(pmd->cc); + memif_msg_send_from_queue(cc); + if (TAILQ_FIRST(&cc->msg_queue) != NULL) + MIF_LOG(WARNING, "Unexpected message(s) in message queue."); + } + rte_spinlock_unlock(&pmd->cc_lock); - /* at this point, there should be no more messages in queue */ - if (TAILQ_FIRST(&pmd->cc->msg_queue) != NULL) { - MIF_LOG(WARNING, - "Unexpected message(s) in message queue."); - } + if (cc == NULL) + return; - ih = pmd->cc->intr_handle; - if (rte_intr_fd_get(ih) >= 0) { - ret = rte_intr_callback_unregister(ih, - memif_intr_handler, - pmd->cc); - /* - * If callback is active (disconnecting based on - * received control message). - */ - if (ret == -EAGAIN) { - ret = rte_intr_callback_unregister_pending(ih, - memif_intr_handler, - pmd->cc, - memif_intr_unregister_handler); - } else if (ret > 0) { - rte_intr_fd_close(ih); - rte_intr_instance_free(ih); - rte_free(pmd->cc); - } - pmd->cc = NULL; - if (ret <= 0) - MIF_LOG(WARNING, - "Failed to unregister control channel callback."); + if (rte_thread_is_intr()) { + /* + * In the interrupt thread (possibly memif_intr_handler() + * itself): cannot wait for the handler. If it is active, hand + * the free to the pending-unregister callback. + */ + ret = rte_intr_callback_unregister(cc->intr_handle, + memif_intr_handler, cc); + if (ret == -EAGAIN) + ret = rte_intr_callback_unregister_pending(cc->intr_handle, + memif_intr_handler, cc, + memif_intr_unregister_handler); + else if (ret > 0) + memif_cc_free(cc); + } else { + /* + * In an application thread (dev_stop/dev_close): the handler + * may be mid-flight and will drop itself once it sees pmd->cc + * detached. Wait for it, then free (-ENOENT: already dropped). + */ + ret = rte_intr_callback_unregister_sync(cc->intr_handle, + memif_intr_handler, cc); + if (ret > 0 || ret == -ENOENT) { + memif_cc_free(cc); + ret = 1; } } + if (ret <= 0) + MIF_LOG(WARNING, "Failed to unregister control channel callback."); +} + +void +memif_disconnect(struct rte_eth_dev *dev) +{ + struct pmd_internals *pmd = dev->data->dev_private; + struct memif_queue *mq; + int i; + + rte_spinlock_lock(&pmd->cc_lock); + dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN; + pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTING; + pmd->flags &= ~ETH_MEMIF_FLAG_CONNECTED; rte_spinlock_unlock(&pmd->cc_lock); + /* Tear down the control channel, synchronising with the handler. */ + memif_cc_detach(pmd); + + /* + * The handler can no longer run for this device, so the queues and + * regions can be freed without holding cc_lock. + */ /* unconfig interrupts */ for (i = 0; i < pmd->cfg.num_c2s_rings; i++) { if (pmd->role == MEMIF_ROLE_CLIENT) { @@ -868,6 +901,8 @@ static void memif_intr_handler(void *arg) { struct memif_control_channel *cc = arg; + struct pmd_internals *pmd; + bool owned; int ret; ret = memif_msg_receive(cc); @@ -898,6 +933,21 @@ memif_intr_handler(void *arg) MIF_LOG(WARNING, "eth dev not allocated"); return; } + + pmd = cc->dev->data->dev_private; + rte_spinlock_lock(&pmd->cc_lock); + owned = (pmd->cc == cc); + rte_spinlock_unlock(&pmd->cc_lock); + if (!owned) { + /* + * Detached by memif_disconnect() in an application thread + * which is waiting for this handler to finish and will free + * the channel. Stop polling the fd, it stays readable on EOF. + */ + rte_intr_callback_unregister_pending(cc->intr_handle, + memif_intr_handler, cc, NULL); + return; + } memif_disconnect(cc->dev); } -- 2.43.7
