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

[Stephen Hemminger <stephen@networkplumber.org>]

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.