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: [email protected]
Reported-by: Stephen Hemminger <[email protected]>
Suggested-by: Stephen Hemminger <[email protected]>
Signed-off-by: Sriram Yagnaraman <[email protected]>
---
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
