fix(rocev2.Server): free RX slab in destructor to fix zero-copy teardown segfault - #1276
fix(rocev2.Server): free RX slab in destructor to fix zero-copy teardown segfault#1276ruck314 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
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 duringstop()/cleanupResources(). - Add
rogue::GilReleasearound ibverbs bring-up, buffer return repost, and teardown to avoid blocking/deadlocking Python threads. - Add an
rxe0loopback regression test (isolated binary) that retains a frame paststop()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.
…ro-copy UAF stop() -> cleanupResources() previously free()d the registered RX slab while zero-copy Buffers already handed downstream still pointed into it, surfacing as a segfault at process/GUI teardown. Every zero-copy Buffer holds a shared_ptr to its Pool (the Server), so ~Server cannot run until all outstanding Buffers are released -- that is the safe place to free the slab, mirroring ris::Pool::~Pool. stop() now releases only the external ibverbs resources (QP/CQ/MR/comp-channel/wake pipe), matching AxiStreamDma::stop(); the failed-construction path frees the slab explicitly since the destructor does not run on a partial object. Also add the missing rogue::GilRelease to the ibverbs bring-up (constructor, completeConnection), the Buffer::~Buffer re-post (retBuffer), and teardown (stop(), which joins the recv thread that re-acquires the GIL via sendFrame), matching AxiStreamDma and Pool.
New tests/cpp/protocols/rocev2/test_rocev2_teardown.cpp (gated on an rxe0 SoftRoCE device): loopback RDMA SEND, retain the received frame past Server::stop(), and assert the payload survives. RED on the old teardown (slab freed in stop()), GREEN with the destructor-frees-slab fix. The LoopbackClient uses RAII cleanup, scans the full GID table to match the complete IPv4-mapped prefix, and bound-checks the GID index before the uint8_t truncation.
Match the C++ API page to the teardown fix: stop() releases the QP/CQ/MR but no longer frees the RX slab, and ~Server() frees it last after the final zero-copy Buffer releases the Server.
bengineerd
left a comment
bengineerd
left a comment
There was a problem hiding this comment.
Review findings from local inspection of PR head 4fa2644.
| CHECK_EQ(std::memcmp(after.data(), client.sbuf.data(), kPayloadLen), 0); | ||
|
|
||
| // Release the held frame before the client MR/QP it (indirectly) races. | ||
| frame.reset(); |
There was a problem hiding this comment.
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.
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Additional review note on the AxiStreamDma teardown comparison. The AxiStreamDma follow-up is now split into #1277.
| thread_ = nullptr; | ||
| } | ||
| // Release the external ibverbs resources now — QP/CQ/MR(dereg)/comp-channel | ||
| // and the wake pipe — mirroring AxiStreamDma::stop(), which releases its |
There was a problem hiding this comment.
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.
Description
Fix a use-after-free in
rogue::protocols::rocev2::Serverteardown.stop()freed the registered RX slab while zero-copyBuffers already handed downstream still pointed into it, which surfaced as aSegmentation faultat process/GUI teardown. The slab is now freed in~Server(its lifetime is the Pool object's, as withris::Pool::~Pool), whilestop()releases only the external ibverbs resources (QP/CQ/MR/comp-channel/wake pipe), matchingAxiStreamDma::stop. Also adds the missingrogue::GilReleaseto the constructor,completeConnection,retBuffer, andstop()(matchingAxiStreamDma/Pool), plus a hardware-gated regression test that reproduces the UAF over anrxe0loopback.Details
Server::stop()→cleanupResources()previouslyfree()dslab_; a frame still queued in a downstream depacketizer /PrbsRxthen became a dangling pointer. Every zero-copyBufferholds ashared_ptrto itsPool(theServer), so~Servercannot run until all outstanding Buffers are released — the safe place to free the slab.cleanupResources()drops itsfreeSlabargument; the failed-construction path frees the slab explicitly (the destructor does not run on a partially-constructed object).rogue::GilReleaseadded to the ibverbs bring-up (constructor,completeConnection), theBuffer::~Bufferre-post (retBuffer), and teardown (stop()— which joins the recv thread that re-acquires the GIL viasendFrame), matching the convention already used inAxiStreamDmaand the basePool.tests/cpp/protocols/rocev2/test_rocev2_teardown.cpp(hardware-gated onrxe0): loopback RDMA SEND → retain the received frame paststop()→ assert the payload survives. It is RED on the old teardown and GREEN with the fix. Fullctest -L cpp(16/16) and the Python rocev2 suite (35/35) pass; validated end-to-end against a 10GbE RoCEv2 firmware target (25/25 PRBS runs, no segfault).