memif test failures on next-net

memif test failures on next-net

[Stephen Hemminger]

The memif autotest is failing in CI for next-net.

AI suggests that the issue is missing synchronization.
The proposed patch seems to have merge conflicts, and I can't really test it
well. Please make up something better.

Subject: [PATCH] net/memif: fix race between disconnect and control callback

The control channel callback runs on the interrupt thread and calls
memif_disconnect() when the peer hangs up. Nothing orders this against
the application stopping and closing the same port. If the peer of a
loopback pair is stopped first, the callback can still be running
memif_disconnect() when the port is closed and released, and it then
dereferences dev->data which is now NULL.

Seen as a SIGSEGV on the interrupt thread during teardown of
memif_pmd_autotest.

Take the control channel from the device under cc_lock, and release it
outside the lock. When not on the interrupt thread, wait for a running
callback with rte_intr_callback_unregister_sync() so the device stays
valid until it returns. The callback no longer disconnects a device
that has already dropped its channel, and queue and region teardown is
serialized under cc_lock.

Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
Cc: stable@dpdk.org


---
 drivers/net/memif/memif_socket.c | 101 +++++++++++++++++++++----------
 1 file changed, 68 insertions(+), 33 deletions(-)

diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index b47c7a3b..a7b1c45f 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -590,66 +590,86 @@ memif_intr_unregister_handler(const struct rte_intr_handle *intr_handle, void *a
 	rte_free(cc);
 }
 
+/*
+ * Detach a control channel that is no longer reachable from its device.
+ *
+ * If its callback is running on the interrupt thread, wait for it to
+ * return so that the device can be released safely by the caller.
+ * If called from that callback, defer the free until it returns.
+ */
+static void
+memif_cc_release(struct memif_control_channel *cc)
+{
+	struct rte_intr_handle *ih = cc->intr_handle;
+	int ret;
+
+	ret = rte_intr_callback_unregister(ih, memif_intr_handler, cc);
+	if (ret == -EAGAIN) {
+		if (rte_thread_is_intr()) {
+			ret = rte_intr_callback_unregister_pending(ih,
+					memif_intr_handler, cc,
+					memif_intr_unregister_handler);
+			if (ret <= 0)
+				MIF_LOG(WARNING,
+					"Failed to unregister control channel callback.");
+			return;
+		}
+		ret = rte_intr_callback_unregister_sync(ih, memif_intr_handler, cc);
+	}
+	if (ret < 0 && ret != -ENOENT && ret != -EINVAL)
+		MIF_LOG(WARNING, "Failed to unregister control channel callback.");
+
+	memif_intr_unregister_handler(ih, cc);
+}
+
 void
 memif_disconnect(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
 	struct memif_msg_queue_elt *elt, *next;
+	struct memif_control_channel *cc;
 	struct memif_queue *mq;
-	struct rte_intr_handle *ih;
 	int i;
-	int ret;
 
+	/*
+	 * Called from the application (stop/close) and from the interrupt
+	 * thread (peer hangup, bad descriptor). Take ownership of the control
+	 * channel under the lock so only one caller releases it.
+	 */
+	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_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);
 
 		/* at this point, there should be no more messages in queue */
-		if (TAILQ_FIRST(&pmd->cc->msg_queue) != NULL) {
+		if (TAILQ_FIRST(&cc->msg_queue) != NULL) {
 			MIF_LOG(WARNING,
 				"Unexpected message(s) in message queue.");
 		}
-
-		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.");
-		}
 	}
 	rte_spinlock_unlock(&pmd->cc_lock);
 
+	/* Must not hold cc_lock here, the running callback may need it. */
+	if (cc != NULL)
+		memif_cc_release(cc);
+
+	/* Serialize teardown against a concurrent disconnect from the other thread. */
+	rte_spinlock_lock(&pmd->cc_lock);
+
 	/* unconfig interrupts */
 	for (i = 0; i < pmd->cfg.num_c2s_rings; i++) {
 		if (pmd->role == MEMIF_ROLE_CLIENT) {
@@ -686,6 +706,7 @@ memif_disconnect(struct rte_eth_dev *dev)
 
 	/* reset connection configuration */
 	memset(&pmd->run, 0, sizeof(pmd->run));
+	rte_spinlock_unlock(&pmd->cc_lock);
 
 	MIF_LOG(DEBUG, "Disconnected, id: %d, role: %s.", pmd->id,
 		(pmd->role == MEMIF_ROLE_SERVER) ? "server" : "client");
@@ -862,6 +883,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);
@@ -892,6 +915,18 @@ memif_intr_handler(void *arg)
 		MIF_LOG(WARNING, "eth dev not allocated");
 		return;
 	}
+
+	/*
+	 * If the device was stopped on another thread, the channel was
+	 * already taken from it and that thread is waiting for this callback
+	 * to return before releasing the channel.
+	 */
+	pmd = cc->dev->data->dev_private;
+	rte_spinlock_lock(&pmd->cc_lock);
+	owned = (pmd->cc == cc);
+	rte_spinlock_unlock(&pmd->cc_lock);
+	if (!owned)
+		return;
 	memif_disconnect(cc->dev);
 }
 
-- 
2.43.0

[PATCH] net/memif: fix race between interrupt thread and stop

[Sriram Yagnaraman]

The control-channel callback runs on the interrupt thread and calls
memif_disconnect() when the peer hangs up. Nothing ordered this against
an application thread stopping and closing the same port. When the peer
of a loopback pair is stopped first, the callback can still be running
memif_disconnect() while the port is closed and released, and then
dereferences dev->data which is NULL. Seen as a SIGSEGV on the interrupt
thread during memif_pmd_autotest teardown.

Detach the control channel in memif_cc_detach(), called without cc_lock:

  - take pmd->cc under cc_lock so only one caller owns it;
  - on the interrupt thread (the callback itself) defer the free to the
    pending-unregister callback, as before;
  - on an application thread wait for a running callback with
    rte_intr_callback_unregister_sync() so the device stays valid until
    it returns, then free.

memif_intr_handler() no longer disconnects a device that has already
dropped this channel: it stops polling the fd and returns, letting the
waiting application thread free the channel. Once memif_cc_detach()
returns the handler cannot run for the device, so the queue and region
teardown no longer needs cc_lock.

Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
Cc: stable@dpdk.org

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
---
 drivers/net/memif/memif_socket.c | 141 +++++++++++++++++++++----------
 1 file changed, 96 insertions(+), 45 deletions(-)

diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index 971964133c..56fbbae9fa 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -572,12 +572,10 @@ 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 */
@@ -590,66 +588,98 @@ memif_intr_unregister_handler(const struct rte_intr_handle *intr_handle, void *a
 	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.
+ */
+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 runs on 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()) {
+		/* On the interrupt thread: cannot wait for the handler (it may
+		 * be us); 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 {
+		/* On an application thread: wait for the handler to finish,
+		 * then free (-ENOENT: it already dropped itself).
+		 */
+		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; free queues and
+	 * regions without cc_lock.
+	 */
 	/* unconfig interrupts */
 	for (i = 0; i < pmd->cfg.num_c2s_rings; i++) {
 		if (pmd->role == MEMIF_ROLE_CLIENT) {
@@ -868,6 +898,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 +930,25 @@ memif_intr_handler(void *arg)
 		MIF_LOG(WARNING, "eth dev not allocated");
 		return;
 	}
+
+	/*
+	 * If an application thread stopped the device it already detached
+	 * this channel and waits in memif_cc_detach() for us to return. Do
+	 * not disconnect a device that no longer owns this channel; stop
+	 * polling the fd (it stays readable on EOF) and let that thread free.
+	 */
+	pmd = cc->dev->data->dev_private;
+	rte_spinlock_lock(&pmd->cc_lock);
+	owned = (pmd->cc == cc);
+	rte_spinlock_unlock(&pmd->cc_lock);
+	if (!owned) {
+		/* The waiting application thread frees the channel; just stop
+		 * polling the fd here (no free callback).
+		 */
+		rte_intr_callback_unregister_pending(cc->intr_handle,
+					memif_intr_handler, cc, NULL);
+		return;
+	}
 	memif_disconnect(cc->dev);
 }
 
-- 
2.43.7

RE: memif test failures on next-net

[Sriram Yagnaraman]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, 29 October 2026 18:03
> To: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> Cc: dev@dpdk.org
> Subject: memif test failures on next-net
> 
> The memif autotest is failing in CI for next-net.
> 
> AI suggests that the issue is missing synchronization.
> The proposed patch seems to have merge conflicts, and I can't really test it
> well. Please make up something better.
> 

Sent https://patches.dpdk.org/project/dpdk/patch/20261030102218.3489371-1-sriram.yagnaraman@ericsson.com/ that applies cleanly on next-net for-main branch.
I was able to reproduce the SEGV without the above fix, 3/100 times, and with the fix 0/100 times. 

Re: [PATCH] net/memif: fix race between interrupt thread and stop

[Thomas Monjalon]

Hello,

30/10/2026 11:22, Sriram Yagnaraman:
> The control-channel callback runs on the interrupt thread and calls
> memif_disconnect() when the peer hangs up. Nothing ordered this against
> an application thread stopping and closing the same port. When the peer
> of a loopback pair is stopped first, the callback can still be running
> memif_disconnect() while the port is closed and released, and then
> dereferences dev->data which is NULL. Seen as a SIGSEGV on the interrupt
> thread during memif_pmd_autotest teardown.
> 
> Detach the control channel in memif_cc_detach(), called without cc_lock:
> 
>   - take pmd->cc under cc_lock so only one caller owns it;
>   - on the interrupt thread (the callback itself) defer the free to the
>     pending-unregister callback, as before;
>   - on an application thread wait for a running callback with
>     rte_intr_callback_unregister_sync() so the device stays valid until
>     it returns, then free.
> 
> memif_intr_handler() no longer disconnects a device that has already
> dropped this channel: it stops polling the fd and returns, letting the
> waiting application thread free the channel. Once memif_cc_detach()
> returns the handler cannot run for the device, so the queue and region
> teardown no longer needs cc_lock.

It seems the teardown is too much complex.
We can probably make something simpler with a re-design.
I'm not sure about applying this big fix on top of a fragile design.
I'm aware it may be a big task.
Tell us if you think the teardown can be better fixed easily.

Otherwise I suggest disabling the memif autotest
from the fast tests series which runs in CI.
What do you think?

Re: [PATCH] net/memif: fix race between interrupt thread and stop

[David Marchand]

On Fri, 30 Oct 2026 at 11:22, Sriram Yagnaraman
<sriram.yagnaraman@ericsson.com> wrote:
>
> The control-channel callback runs on the interrupt thread and calls
> memif_disconnect() when the peer hangs up. Nothing ordered this against
> an application thread stopping and closing the same port. When the peer
> of a loopback pair is stopped first, the callback can still be running
> memif_disconnect() while the port is closed and released, and then
> dereferences dev->data which is NULL. Seen as a SIGSEGV on the interrupt
> thread during memif_pmd_autotest teardown.
>
> Detach the control channel in memif_cc_detach(), called without cc_lock:
>
>   - take pmd->cc under cc_lock so only one caller owns it;
>   - on the interrupt thread (the callback itself) defer the free to the
>     pending-unregister callback, as before;
>   - on an application thread wait for a running callback with
>     rte_intr_callback_unregister_sync() so the device stays valid until
>     it returns, then free.
>
> memif_intr_handler() no longer disconnects a device that has already
> dropped this channel: it stops polling the fd and returns, letting the
> waiting application thread free the channel. Once memif_cc_detach()
> returns the handler cannot run for the device, so the queue and region
> teardown no longer needs cc_lock.
>
> Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> Cc: stable@dpdk.org
>
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>

This driver dealing with interrupt handle is really complex (some
logic should probably move elsewhere... like an alarm handler maybe).

If the issue is a race between an application requesting device close,
and the interrupt thread invoking the interrupt handle callback, then
it means that the device close is not properly unregistering the
interrupt handle in the first place.
So my guts tells me that something like this should help:
https://patchwork.dpdk.org/project/dpdk/patch/20261104164153.1674329-1-david.marchand@redhat.com/


-- 
David Marchand

RE: [PATCH] net/memif: fix race between interrupt thread and stop

[Sriram Yagnaraman]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, 4 November 2026 17:43
> To: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> Cc: dev@dpdk.org; Jakub Grajciar <jgrajcia@cisco.com>; Stephen Hemminger
> <stephen@networkplumber.org>; stable@dpdk.org
> Subject: Re: [PATCH] net/memif: fix race between interrupt thread and stop
> 
> On Fri, 30 Oct 2026 at 11:22, Sriram Yagnaraman
> <sriram.yagnaraman@ericsson.com> wrote:
> >
> > The control-channel callback runs on the interrupt thread and calls
> > memif_disconnect() when the peer hangs up. Nothing ordered this
> > against an application thread stopping and closing the same port. When
> > the peer of a loopback pair is stopped first, the callback can still
> > be running
> > memif_disconnect() while the port is closed and released, and then
> > dereferences dev->data which is NULL. Seen as a SIGSEGV on the
> > interrupt thread during memif_pmd_autotest teardown.
> >
> > Detach the control channel in memif_cc_detach(), called without cc_lock:
> >
> >   - take pmd->cc under cc_lock so only one caller owns it;
> >   - on the interrupt thread (the callback itself) defer the free to the
> >     pending-unregister callback, as before;
> >   - on an application thread wait for a running callback with
> >     rte_intr_callback_unregister_sync() so the device stays valid until
> >     it returns, then free.
> >
> > memif_intr_handler() no longer disconnects a device that has already
> > dropped this channel: it stops polling the fd and returns, letting the
> > waiting application thread free the channel. Once memif_cc_detach()
> > returns the handler cannot run for the device, so the queue and region
> > teardown no longer needs cc_lock.
> >
> > Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
> > Cc: stable@dpdk.org
> >
> > Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> > Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> 
> This driver dealing with interrupt handle is really complex (some logic should
> probably move elsewhere... like an alarm handler maybe).
> 
> If the issue is a race between an application requesting device close, and the
> interrupt thread invoking the interrupt handle callback, then it means that the
> device close is not properly unregistering the interrupt handle in the first place.
> So my guts tells me that something like this should help:
> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20261104164153.167432
> 9-1-
> david.marchand%40redhat.com%2F&data=05%7C02%7Csriram.yagnaraman
> %40ericsson.com%7C6fa63dfec8a649cc8e0008df3a92b277%7C92e84cebfbf
> d47abbe52080c6b87953f%7C0%7C0%7C639294073930934928%7CUnkno
> wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCI
> sIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&s
> data=1nsnyrcY0rxAgsQI1w5NqEVNbMxJnipJ8UTrE8j3y1s%3D&reserved=0
> 
> 
> --
> David Marchand

The above patch is correct and needs to be taken in, but it fixes the listener socket path (socket->intr_handle / memif_listener_handler in memif_socket_remove_device).
From the logs, I feel the CI autotest SIGSEGV is a different race: the control channel teardown (cc->intr_handle / memif_intr_handler) racing memif_disconnect() from dev_stop.

Anyhow, I am working on a simpler fix for the second race (assuming your patch doesn't solve the CI crashes).

[PATCH v2] net/memif: redesign control channel teardown

[Sriram Yagnaraman]

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 <stephen@networkplumber.org>
Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
---
v2:
  - Redesign rather than extend v1. v1's approach (detach + handler
    ownership re-check that returns without unregistering) could
    livelock: the EOF control socket is level-triggered, so the handler
    re-fires continuously and starves the application thread waiting in
    unregister_sync(). v2 makes the handler always self-unregister via
    unregister_pending(), removing the fd from epoll so it cannot
    re-fire.
  - Split device-level teardown (link/flags/queues/regions) into
    memif_disconnect_queues() so the handler-owned disconnect leaves the
    same consistent state as memif_disconnect(); fixes a reconnect
    regression seen while developing the single-owner model.
  - Addresses the "teardown too complex" feedback by giving the channel
    a single owner (whoever clears pmd->cc) instead of splitting the
    free across two threads.
---
 drivers/net/memif/memif_socket.c | 179 ++++++++++++++++++++++---------
 1 file changed, 126 insertions(+), 53 deletions(-)

diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index 971964133c..77d3044681 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -590,67 +590,23 @@ memif_intr_unregister_handler(const struct rte_intr_handle *intr_handle, void *a
 	rte_free(cc);
 }
 
-void
-memif_disconnect(struct rte_eth_dev *dev)
+/*
+ * Device-level teardown: mark the link down, close the per-queue interrupt
+ * eventfds and release the shared-memory regions. Idempotent and independent
+ * of the control channel, so it is safe to run from whichever thread owns the
+ * disconnect.
+ */
+static void
+memif_disconnect_queues(struct rte_eth_dev *dev)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
-	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) {
-		/* Clear control message queue (except disconnect message if any). */
-		for (elt = TAILQ_FIRST(&pmd->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);
-				rte_free(elt);
-			}
-		}
-		/* send disconnect message (if there is any in queue) */
-		memif_msg_send_from_queue(pmd->cc);
-
-		/* 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.");
-		}
-
-		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.");
-		}
-	}
-	rte_spinlock_unlock(&pmd->cc_lock);
-
-	/* unconfig interrupts */
 	for (i = 0; i < pmd->cfg.num_c2s_rings; i++) {
 		if (pmd->role == MEMIF_ROLE_CLIENT) {
 			if (dev->data->tx_queues != NULL)
@@ -697,6 +653,88 @@ memif_disconnect(struct rte_eth_dev *dev)
 		(pmd->role == MEMIF_ROLE_SERVER) ? "server" : "client");
 }
 
+/*
+ * Release a control channel and free it. Exactly one thread must do this
+ * per channel: the caller that cleared pmd->cc under cc_lock owns the free.
+ *
+ * On the interrupt thread the running callback cannot be waited for (it may
+ * be us), so the free is deferred to the pending-unregister callback, which
+ * EAL runs - and which removes the fd from the epoll set - once the current
+ * dispatch finishes. The handler therefore never returns leaving its own fd
+ * registered and hot, so it cannot re-fire on a half-torn-down channel.
+ *
+ * On an application thread the handler (if any) is waited out and the channel
+ * freed directly.
+ */
+static void
+memif_cc_release(struct memif_control_channel *cc)
+{
+	struct rte_intr_handle *ih = cc->intr_handle;
+	int ret;
+
+	if (rte_intr_fd_get(ih) < 0) {
+		/* Never registered; free directly. */
+		memif_intr_unregister_handler(ih, cc);
+		return;
+	}
+
+	if (rte_thread_is_intr()) {
+		ret = rte_intr_callback_unregister_pending(ih,
+				memif_intr_handler, cc,
+				memif_intr_unregister_handler);
+		if (ret < 0)
+			MIF_LOG(WARNING,
+				"Failed to defer control channel callback unregister.");
+	} else {
+		ret = rte_intr_callback_unregister_sync(ih,
+				memif_intr_handler, cc);
+		if (ret > 0 || ret == -ENOENT)
+			memif_intr_unregister_handler(ih, cc);
+		else
+			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_msg_queue_elt *elt, *next;
+	struct memif_control_channel *cc;
+
+	/*
+	 * Detach the control channel: whoever clears pmd->cc under the lock
+	 * becomes the single owner of the free. If it is already NULL another
+	 * thread (the interrupt handler, or a concurrent disconnect) has taken
+	 * ownership and will release it; we only do the device-level teardown.
+	 */
+	rte_spinlock_lock(&pmd->cc_lock);
+	cc = pmd->cc;
+	pmd->cc = NULL;
+	if (cc != NULL) {
+		/* Clear control message queue (except disconnect message if any). */
+		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(&cc->msg_queue, elt, next);
+				rte_free(elt);
+			}
+		}
+		/* send disconnect message (if there is any in queue) */
+		memif_msg_send_from_queue(cc);
+		/* at this point, there should be no more messages in queue */
+		if (TAILQ_FIRST(&cc->msg_queue) != NULL)
+			MIF_LOG(WARNING, "Unexpected message(s) in message queue.");
+	}
+	rte_spinlock_unlock(&pmd->cc_lock);
+
+	if (cc != NULL)
+		memif_cc_release(cc);
+
+	memif_disconnect_queues(dev);
+}
+
 static int
 memif_msg_receive(struct memif_control_channel *cc)
 {
@@ -868,6 +906,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,7 +938,40 @@ memif_intr_handler(void *arg)
 		MIF_LOG(WARNING, "eth dev not allocated");
 		return;
 	}
-	memif_disconnect(cc->dev);
+
+	/*
+	 * Self-unregister first. rte_intr_callback_unregister_pending() marks
+	 * this callback for deletion once the current dispatch returns, which
+	 * removes the fd from the epoll set. This is essential: the control
+	 * socket is at EOF and level-triggered, so without removing it the
+	 * handler would be re-dispatched forever (livelock) and starve any
+	 * application thread waiting in rte_intr_callback_unregister_sync().
+	 *
+	 * Ownership of the free is decided under cc_lock: whoever clears
+	 * pmd->cc frees the channel.
+	 *   - If this handler wins, it passes memif_intr_unregister_handler as
+	 *     the pending-unregister callback so EAL frees the channel after
+	 *     the dispatch, and it runs the device teardown.
+	 *   - If an application thread already cleared pmd->cc, it owns the
+	 *     free (it waits for us via unregister_sync, then frees); this
+	 *     handler passes a NULL unregister callback and does nothing else.
+	 */
+	pmd = cc->dev->data->dev_private;
+	rte_spinlock_lock(&pmd->cc_lock);
+	owned = (pmd->cc == cc);
+	if (owned)
+		pmd->cc = NULL;
+	rte_spinlock_unlock(&pmd->cc_lock);
+
+	ret = rte_intr_callback_unregister_pending(cc->intr_handle,
+			memif_intr_handler, cc,
+			owned ? memif_intr_unregister_handler : NULL);
+	if (ret < 0)
+		MIF_LOG(WARNING,
+			"Failed to defer control channel callback unregister.");
+
+	if (owned)
+		memif_disconnect_queues(cc->dev);
 }
 
 static void
-- 
2.43.7

RE: [PATCH] net/memif: fix race between interrupt thread and stop

[Sriram Yagnaraman]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, 4 November 2026 17:07
> To: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> Cc: dev@dpdk.org; Jakub Grajciar <jgrajcia@cisco.com>; Stephen Hemminger
> <stephen@networkplumber.org>; stable@dpdk.org; David Marchand
> <david.marchand@redhat.com>
> Subject: Re: [PATCH] net/memif: fix race between interrupt thread and stop
> 
> Hello,
> 
> 30/10/2026 11:22, Sriram Yagnaraman:
> > The control-channel callback runs on the interrupt thread and calls
> > memif_disconnect() when the peer hangs up. Nothing ordered this
> > against an application thread stopping and closing the same port. When
> > the peer of a loopback pair is stopped first, the callback can still
> > be running
> > memif_disconnect() while the port is closed and released, and then
> > dereferences dev->data which is NULL. Seen as a SIGSEGV on the
> > interrupt thread during memif_pmd_autotest teardown.
> >
> > Detach the control channel in memif_cc_detach(), called without cc_lock:
> >
> >   - take pmd->cc under cc_lock so only one caller owns it;
> >   - on the interrupt thread (the callback itself) defer the free to the
> >     pending-unregister callback, as before;
> >   - on an application thread wait for a running callback with
> >     rte_intr_callback_unregister_sync() so the device stays valid until
> >     it returns, then free.
> >
> > memif_intr_handler() no longer disconnects a device that has already
> > dropped this channel: it stops polling the fd and returns, letting the
> > waiting application thread free the channel. Once memif_cc_detach()
> > returns the handler cannot run for the device, so the queue and region
> > teardown no longer needs cc_lock.
> 
> It seems the teardown is too much complex.
> We can probably make something simpler with a re-design.
> I'm not sure about applying this big fix on top of a fragile design.
> I'm aware it may be a big task.
> Tell us if you think the teardown can be better fixed easily.
> 
> Otherwise I suggest disabling the memif autotest from the fast tests series
> which runs in CI.
> What do you think?
> 

I agree, a redesign is warranted. For now, I did, what I think, is a better fix: https://patches.dpdk.org/project/dpdk/patch/20261105112026.432015-1-sriram.yagnaraman@ericsson.com/
If we conclude that even that is too complex, we can disable the memif autotest for now, I can send an RFC for a slightly bigger redesign later.

Re: [PATCH v2] net/memif: redesign control channel teardown

[Stephen Hemminger]

On Thu, 5 Nov 2026 12:20:26 +0100
Sriram Yagnaraman <sriram.yagnaraman@ericsson.com> 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 <stephen@networkplumber.org>
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
> ---

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: stable@dpdk.org"; 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.