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

[ruck314]

Description

Fix a use-after-free in rogue::protocols::rocev2::Server teardown. stop() freed the registered RX slab while zero-copy Buffers already handed downstream still pointed into it, which surfaced as a segmentation fault during process or GUI teardown.

The RX slab is now treated as the Pool's buffer backing and remains alive until ~Server(), while stop() releases only the external ibverbs resources. Deferred zero-copy buffer returns are serialized against QP/MR teardown. Callers must quiesce completeConnection() and other operations using the Server's ibverbs resources before calling stop(); only returns of previously issued zero-copy buffers may overlap shutdown.

Details

• Server::stop() previously called cleanupResources(), which freed slab_ even if a downstream frame still referenced it. Each zero-copy Buffer holds a shared_ptr to its Pool (the Server), so moving the slab free to ~Server() ensures that the backing memory outlives every outstanding buffer. The failed-construction path still frees the slab explicitly because the destructor does not run for a partially constructed object.
• cleanupResources() still destroys the QP and CQ, deregisters the MR, closes the completion channel and wake pipe, and remains idempotent.
• resourcesMtx_ serializes postRecvWr() with cleanupResources(). A deferred return therefore either completes its re-post before teardown or observes that the QP/MR are already unavailable; it cannot use them while they are being destroyed.
• The mutex deliberately does not attempt to make the complete public API concurrent with stop(). The documented lifecycle contract requires callers to quiesce completeConnection() and other ibverbs-resource operations first.
• Missing rogue::GilRelease coverage is added around ibverbs bring-up, connection completion, deferred buffer return, and shutdown. In particular, stop() may join a receive thread that re-acquires the GIL while delivering a frame.
• The hardware-gated rxe0 regression now retains multiple zero-copy frames, detaches them from HoldingSlave to break the Server -> Slave -> Frame -> Buffer -> Server ownership cycle, synchronizes concurrent buffer returns with stop(), verifies the retained payload remains valid after shutdown, and proves that the Server is ultimately destroyed with a weak_ptr.
• Test cleanup stops delivery before clearing retained frames, so fatal assertions cannot leak the ownership cycle or allow an arriving frame to recreate it during stack unwinding.

One performance question remains open: each active zero-copy return that re-posts a receive buffer now takes an additional mutex lock/unlock before the existing decCounter() lock. This is much narrower than locking every allocation, connection, getter, and control operation, but the PR has no focused RoCE receive-buffer-return throughput measurement. The general performance tests do not exercise this rxe0/ibverbs return path. If high-rate impact is the deciding issue, a hardware measurement or focused benchmark would close it properly.

Validation

• Local macOS build completed successfully.
• ctest --test-dir build --output-on-failure -L cpp: 14/14 passed. RoCEv2 is Linux-only and is not compiled in this build.
• Sphinx HTML documentation rendered successfully; the three warnings were pre-existing and unrelated to this change.
• git diff --check passed.
• The RoCEv2 regression is compiled on supported Linux builds and executes when an rxe0 device is present; otherwise it runtime-skips.
• Earlier end-to-end validation against a 10 GbE RoCEv2 firmware target completed 25/25 PRBS runs without the teardown segmentation fault.

The synchronized return-versus-stop regression is the best practical hardware-path coverage available without adding an invasive production test hook: it starts both operations together and returns several buffers, but it does not force a particular instruction-level interleaving inside libibverbs.

[Copilot]

Pull request overview

This PR fixes a RoCEv2 Server teardown use-after-free affecting zero-copy RX buffers by moving RX slab deallocation to ~Server (so it can’t be freed while downstream still holds Buffers), while keeping stop() responsible for releasing ibverbs/FD resources. It also aligns RoCEv2 bring-up/teardown paths with the project’s Python GIL-release convention and adds a hardware-gated regression test that reproduces the original failure mode on rxe0.

Changes:

• Prevent zero-copy UAF by freeing the RX slab in rogue::protocols::rocev2::Server::~Server() instead of during stop()/cleanupResources().
• Add rogue::GilRelease around ibverbs bring-up, buffer return repost, and teardown to avoid blocking/deadlocking Python threads.
• Add an rxe0 loopback regression test (isolated binary) that retains a frame past stop() and validates payload integrity.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/rogue/protocols/rocev2/Server.cpp	Moves slab lifetime to destructor, adjusts cleanup semantics, and adds GilRelease in key paths.
include/rogue/protocols/rocev2/Server.h	Updates cleanupResources() contract docs to reflect slab lifetime change.
tests/cpp/protocols/rocev2/test_rocev2_teardown.cpp	Adds a hardware-gated regression reproducer for the zero-copy teardown UAF.
tests/cpp/protocols/rocev2/CMakeLists.txt	Builds the new teardown regression as an isolated test binary.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[bengineerd]

Review findings from local inspection of PR head 4fa2644.

[bengineerd]

This does not actually release the last held frame. server owns slave via addSlave(), HoldingSlave::frames_ owns the FramePtr, and that frame's Buffer owns a shared_ptr back to the Server. frame.reset() only drops the local copy, so the test exits with a Server -> slave -> frame -> buffer -> Server cycle and ~Server() never frees the slab. Please add a HoldingSlave::clear() (or similar) and call it here before leaving the test.

[bengineerd]

The re-post check just below this is still racing teardown. A downstream frame can be released on another thread while stop() is past threadEn_.store(false) and about to destroy qp_/deregister mr_ in cleanupResources(). Since retBuffer() reads threadEn_ and qp_ without synchronizing with cleanupResources(), it can enter postRecvWr() with a QP/MR being destroyed. The re-post path and resource cleanup need shared exclusion (for example, a mutex around the threadEn_/qp_/mr_ check+post and cleanupResources()), or teardown needs another way to wait out active retBuffer() calls.

[bengineerd]

Additional review note on the AxiStreamDma teardown comparison. The AxiStreamDma follow-up is now split into #1277.

[bengineerd]

Update: I split the AxiStreamDma side of this into #1277.

I would not use AxiStreamDma::stop() as the lifecycle precedent here without caveating it. Looking at AxiStreamDma, it appears to have the same latent issues: RX zero-copy frames are created over desc_->rawBuff[...], stop() can call closeShared(desc_) and dmaUnMapDma() that mapping while downstream frames may still hold those buffers, and retBuffer() checks fd_ >= 0 before dmaRetIndex(fd_, ...) without synchronizing against stop() closing fd_. So matching the old AxiStreamDma behavior does not prove this teardown contract is safe; the AxiStreamDma fix is now tracked separately in #1277, and this PR should avoid presenting the old AxiStreamDma behavior as a safe model.