[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 at dpdk.org

Reported-by: Stephen Hemminger <stephen at networkplumber.org>
Suggested-by: Stephen Hemminger <stephen at networkplumber.org>
Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman at 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