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