fix(rocev2.Server): free RX slab in destructor to fix zero-copy teardown segfault

docs/src/api/cpp/protocols/rocev2/server.rst

@@ -27,6 +27,11 @@ Threading and Lifecycle
 - An ibverbs failure inside ``postRecvWr()`` is caught inside the poll
   thread; the thread logs the error and exits cleanly instead of
   escaping the thread entry point and triggering ``std::terminate``.
+- Before calling ``stop()``, callers must quiesce ``completeConnection()``
+  and other public operations that use the Server's ibverbs resources.
+  Deferred returns from zero-copy buffers already issued by the Server may
+  overlap ``stop()``; their receive-WR re-posts are serialized against QP/MR
+  teardown internally.
 
 
 Python binding
@@ -72,16 +77,27 @@ Class Reference
 
    .. cpp:function:: ~Server()
 
-      Calls ``stop()``; ``stop()`` releases the slab MR and CQ/QP
-      resources, and the inherited ``Core`` destructor releases the PD
-      and ibverbs context.
+      Calls ``stop()`` to release the CQ/QP and MR resources, then frees
+      the RX slab **last**. The slab is deliberately freed here rather than
+      in ``stop()``: it is this ``Pool``'s buffer backing, and every
+      zero-copy ``Buffer`` handed downstream holds a ``shared_ptr`` to this
+      ``Server``, so the destructor cannot run until the last outstanding
+      frame is released — the slab is therefore never freed while a
+      downstream frame still references it. The inherited ``Core``
+      destructor then releases the PD and ibverbs context.
 
    .. cpp:function:: void stop()
 
       Signals the receive thread to exit, joins and deletes it, then
-      releases the ibverbs resources owned by ``Server``: destroys the
-      QP and CQ, deregisters the MR, and frees the slab. Idempotent —
-      safe to call multiple times (also called by ``~Server()``).
+      releases the external ibverbs resources owned by ``Server``: destroys
+      the QP and CQ and deregisters the MR. Does **not** free the RX slab —
+      that is the ``Pool`` buffer backing and is freed by ``~Server()`` (see
+      above), so a zero-copy ``Buffer`` still held downstream is never
+      stranded. Idempotent — safe to call multiple times (also called by
+      ``~Server()``). Callers must first quiesce ``completeConnection()`` and
+      other public ibverbs-resource operations. Deferred zero-copy buffer
+      returns may overlap ``stop()`` and are serialized internally against
+      destruction of the QP and MR.
 
    .. cpp:function:: void setFpgaGid(const std::string& gidBytes)
 

include/rogue/protocols/rocev2/Server.h

@@ -24,6 +24,7 @@
 
 #include <atomic>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <thread>
 #include <vector>
@@ -86,6 +87,9 @@ class Server : public rogue::protocols::rocev2::Core,
     // runThread() selects on wakeFd_[0]. Both ends are non-blocking.
     int wakeFd_[2];
 
+    // Serializes ibverbs resource teardown against deferred buffer re-posts.
+    std::mutex resourcesMtx_;
+
     // Intentional shadow: both Core and stream::Slave expose a `log_` member,
     // so any unqualified `log_` from inside Server would otherwise be
     // ambiguous.  Declaring our own `log_` here consolidates both names onto
@@ -105,9 +109,14 @@ class Server : public rogue::protocols::rocev2::Core,
     // poll/drain loop in runThread() exits.
     void processCompletion(struct ibv_wc& wc);
 
-    // Idempotent helper that releases every ibverbs / heap resource owned by
-    // Server in reverse allocation order.  Called by stop() and from the
-    // failed-construction path in the constructor.
+    // Releases the external ibverbs resources owned by Server (QP, CQ,
+    // comp-channel, MR registration, wake pipe) in reverse allocation order.
+    // Idempotent.  Does NOT free the RX slab: that is this Pool's buffer backing
+    // and lives with the Server object (freed in ~Server, mirroring
+    // ris::Pool::~Pool), so a zero-copy Buffer still held downstream is never
+    // stranded.  Resource teardown is serialized with postRecvWr() so deferred
+    // buffer returns cannot use a QP or MR while it is being destroyed.  The
+    // failed-construction path frees the slab explicitly.
     void cleanupResources();
 
   protected:
@@ -133,6 +142,20 @@ class Server : public rogue::protocols::rocev2::Core,
            uint32_t           rxQueueDepth);
 
     ~Server();
+
+    /**
+     * @brief Stops the receive thread and releases external ibverbs resources.
+     *
+     * @details
+     * Before calling `stop()`, callers must ensure that `completeConnection()`
+     * and any other public operations using the Server's ibverbs resources have
+     * completed and that no new ones can begin.  Deferred returns from
+     * previously issued zero-copy buffers may overlap `stop()`; their receive-WR
+     * re-posts are serialized against QP/MR teardown internally.
+     *
+     * The RX slab remains valid until destruction so retained zero-copy buffers
+     * do not reference freed memory after `stop()`.
+     */
     void stop();
 
     void setFpgaGid(const std::string& gidBytes);

src/rogue/protocols/rocev2/Server.cpp

@@ -102,6 +102,13 @@ rpr::Server::Server(const std::string& deviceName,
     memset(fpgaGid_, 0, 16);
     wakeFd_[0] = wakeFd_[1] = -1;
 
+    // Release the GIL for the ibverbs bring-up below (open device, reg_mr, create
+    // CQ/QP, modify to INIT, query GID) — blocking syscalls run while the Python
+    // caller holds the GIL.  Matches AxiStreamDma's constructor.  On a throw,
+    // GilRelease's destructor re-acquires the GIL before the boost.python
+    // exception translator runs.
+    rogue::GilRelease noGil;
+
     // The destructor does NOT run on a partially-constructed object, so any
     // throw between here and the end of the body would leak slab_ / mr_ /
     // cq_ / qp_.  Wrap the body in try/catch and call cleanupResources() on
@@ -281,17 +288,36 @@ rpr::Server::Server(const std::string& deviceName,
         log_->info("RC QP ready: qpn=0x%06x rqPsn=0x%06x sqPsn=0x%06x",
                    hostQpn_, hostRqPsn_, hostSqPsn_);
     } catch (...) {
+        // Failed construction: the destructor does NOT run on a partially
+        // constructed object, so release the ibverbs resources and free the slab
+        // here.  No zero-copy Buffer exists yet, so freeing the slab is safe.
         cleanupResources();
+        if (slab_) {
+            free(slab_);
+            slab_ = nullptr;
+        }
         throw;
     }
 }
 
 // ---------------------------------------------------------------------------
-// cleanupResources — release every ibverbs / heap resource owned by Server,
-// in reverse order of allocation.  Idempotent (safe to call from both the
-// failed-construction path and stop()).
+// cleanupResources — release the EXTERNAL ibverbs resources owned by Server
+// (QP, CQ, comp-channel, MR registration, wake pipe), in reverse order of
+// allocation.  Idempotent.
+//
+// The RX slab is deliberately NOT freed here: it is this Pool's buffer backing,
+// and per the base Pool convention (ris::Pool::~Pool) its lifetime is the Server
+// object's.  Freeing it in stop() would be a use-after-free for any zero-copy
+// Buffer still held downstream (each points into slab_ and keeps this Server
+// alive via shared_ptr<Pool>).  ~Server frees the slab; the failed-construction
+// path frees it explicitly (the destructor does not run on a partial object).
+//
+// Serialized with postRecvWr() so a deferred retBuffer() cannot re-post using
+// qp_/mr_ while stop() is destroying them.
 // ---------------------------------------------------------------------------
 void rpr::Server::cleanupResources() {
+    std::lock_guard<std::mutex> lock(resourcesMtx_);
+
     if (qp_) {
         ibv_destroy_qp(qp_);
         qp_ = nullptr;
@@ -308,10 +334,8 @@ void rpr::Server::cleanupResources() {
         ibv_dereg_mr(mr_);
         mr_ = nullptr;
     }
-    if (slab_) {
-        free(slab_);
-        slab_ = nullptr;
-    }
+    // slab_ is intentionally NOT freed here — it is the Pool buffer backing (see
+    // the comment above); ~Server and the failed-construction path free it.
     for (int i = 0; i < 2; ++i) {
         if (wakeFd_[i] >= 0) {
             close(wakeFd_[i]);
@@ -337,6 +361,11 @@ void rpr::Server::setFpgaGid(const std::string& gidBytes) {
 // ---------------------------------------------------------------------------
 void rpr::Server::completeConnection(uint32_t fpgaQpn, uint32_t fpgaRqPsn,
                                      uint32_t pmtu, uint32_t minRnrTimer) {
+    // Release the GIL for the ibverbs bring-up (two ibv_modify_qp transitions and
+    // the recv-WR pre-post loop) — blocking syscalls run from Python with the GIL
+    // held.  Mirrors the GIL handling in AxiStreamDma's construction/bring-up.
+    rogue::GilRelease noGil;
+
     // Single-use: a second call would reassign thread_ and orphan the
     // original std::thread.  Real misuse would also be caught by
     // ibv_modify_qp rejecting INIT→RTR when the QP is already in RTS,
@@ -454,6 +483,8 @@ std::string rpr::Server::getGid() const {
 // wr_id == slot index so no lookup is needed on completion
 // ---------------------------------------------------------------------------
 void rpr::Server::postRecvWr(uint32_t slot) {
+    std::lock_guard<std::mutex> lock(resourcesMtx_);
+
     // Defensive: slot indexes into slab_; a corrupted wr_id (from the CQ)
     // or meta (from retBuffer) must not produce an out-of-bounds slab
     // pointer.  Normal control flow keeps slot < numBufs_ because we set
@@ -464,6 +495,11 @@ void rpr::Server::postRecvWr(uint32_t slot) {
                                           "slot=%u out of range (numBufs=%u)",
                                           slot, numBufs_));
 
+    if (!qp_ || !mr_ || !slab_)
+        throw(rogue::GeneralError::create("rocev2::Server::postRecvWr",
+                                          "resources are not available for slot=%u",
+                                          slot));
+
     uint8_t* bufStart = slab_ + (static_cast<uint64_t>(slot) * bufSize_);
 
     struct ibv_sge sge;
@@ -495,11 +531,16 @@ void rpr::Server::postRecvWr(uint32_t slot) {
 // meta lower 24 bits = slot index (set in createBuffer() call in runThread)
 // ---------------------------------------------------------------------------
 void rpr::Server::retBuffer(uint8_t* data, uint32_t meta, uint32_t rawSize) {
+    // retBuffer runs from Buffer::~Buffer(), which can fire on a Python thread
+    // holding the GIL; release it around the ibv_post_recv re-post and decCounter
+    // (which locks).  Matches ris::Pool::retBuffer() and AxiStreamDma::retBuffer().
+    rogue::GilRelease noGil;
+
     uint32_t slot = meta & 0x00FFFFFF;
 
     log_->debug("retBuffer: re-posting slot=%u", slot);
 
-    if (threadEn_.load() && qp_) {
+    if (threadEn_.load()) {
         try {
             postRecvWr(slot);
         } catch (...) {
@@ -729,6 +770,18 @@ void rpr::Server::acceptFrame(ris::FramePtr /*frame*/) {
 // stop / destructor
 // ---------------------------------------------------------------------------
 void rpr::Server::stop() {
+    // Callers must quiesce completeConnection() and other public ibverbs
+    // resource operations before shutdown.  Deferred zero-copy buffer returns
+    // are the one operation allowed to overlap stop(); resourcesMtx_ serializes
+    // their receive-WR re-posts with cleanupResources() below.
+
+    // Release the GIL for the duration of teardown.  stop() is driven from Python
+    // (RoCEv2Server._stop) with the GIL held, and the receive thread re-acquires
+    // the GIL via ScopedGil inside sendFrame() when a downstream slave is Python;
+    // joining that thread below while holding the GIL would deadlock.  Matches
+    // AxiStreamDma::stop() / udp::Server::stop() / TcpCore::stop().
+    rogue::GilRelease noGil;
+
     // Signal the thread to exit if it is still running.  Always join /
     // delete thread_ when it is non-null: runThread() may have already
     // cleared threadEn_ itself (e.g. on IBV_WC_WR_FLUSH_ERR or a caught
@@ -749,10 +802,32 @@ void rpr::Server::stop() {
         delete thread_;
         thread_ = nullptr;
     }
+    // Release the external ibverbs resources now — QP/CQ/MR(dereg)/comp-channel
+    // and the wake pipe — mirroring AxiStreamDma::stop(), which closes its
+    // per-instance fd while leaving zero-copy backing memory alive until
+    // destruction.  The RX slab is this Pool's buffer backing, and zero-copy
+    // Buffers handed downstream still point into it.  Per the base Pool contract
+    // (ris::Pool::~Pool() frees its buffer memory at destruction, and every
+    // Buffer holds a shared_ptr to its Pool), the slab's lifetime is the Server
+    // object's; it is freed in ~Server (below), after the last outstanding Buffer
+    // has released the Server.
     cleanupResources();
 }
 
-rpr::Server::~Server() { this->stop(); }
+rpr::Server::~Server() {
+    this->stop();
+
+    // Free the RX slab last, following the base Pool convention: ris::Pool::~Pool()
+    // frees its buffer backing (dataQ_) at destruction, never in a stop().  Every
+    // zero-copy Buffer created over slab_ holds a shared_ptr to this Server
+    // (Pool::createBuffer -> Buffer::source_), so the destructor cannot run until
+    // the last outstanding Buffer is gone — the slab is therefore safe to free
+    // here and is never freed while a downstream frame still references it.
+    if (slab_) {
+        free(slab_);
+        slab_ = nullptr;
+    }
+}
 
 // ---------------------------------------------------------------------------
 // Python bindings

tests/cpp/protocols/rocev2/CMakeLists.txt

@@ -19,3 +19,16 @@ rogue_add_cpp_test(rogue-cpp-rocev2
    LIBRARIES
       ibverbs
 )
+
+# Isolated in its own binary: on a buggy (unpatched) build this test provokes a
+# genuine use-after-free (SIGSEGV) at the teardown read, so keeping it separate
+# means a regression here cannot abort the other rocev2 cpp cases.
+rogue_add_cpp_test(rogue-cpp-rocev2-teardown
+   SOURCES
+      test_rocev2_teardown.cpp
+   LABELS
+      cpp-rocev2
+      requires-python
+   LIBRARIES
+      ibverbs
+)