Describe the bug
An old RdmaEndpoint::PollCq callback can survive an RDMA
Reset()/health-check Revive() cycle and then access the resources of
the new RDMA generation.
PR #3261 / commit 58fec6f documented that a queued CQ callback may run
after the main Socket has been reset and revived. The merged fix drains
the old CQ after moving the QP to RESET, but it does not invalidate or
synchronize with a callback that has already copied the non-owning
RdmaEndpoint* from the old CQ Socket.
The following interleaving is still possible:
Old CQ Socket callback Main Socket / health checker
---------------------- ----------------------------
PollCq(old_cq_socket)
ep = old_cq_socket->user()
[preempted before Address()] QP failure / SetFailed()
WaitAndReset()
RdmaEndpoint::Reset()
DeallocateResources()
old_cq_socket->_user = NULL
drain old CQ
CheckHealth() / Revive()
AllocateResources()
create new _resource
create new _cq_sid
Socket::Address(ep->_socket->id())
succeeds after revival
cq = ep->_resource->... New CQ callback may run
// This is the new CQ
ibv_poll_cq(cq)
HandleCompletion(wc)
Setting the old CQ Socket's _user to NULL does not invalidate the
ep pointer already copied by a running callback.
PollCq verifies that the main Socket is addressable, but does not
verify that:
m->id() is still equal to ep->_cq_sid;
- the callback belongs to the current RDMA generation;
ep->_resource is the resource for which the callback was created.
The old and new CQ Socket callbacks can therefore both poll the new CQ
and concurrently call HandleCompletion() on the same RdmaEndpoint.
Because they belong to different CQ Sockets, Socket-level event
serialization does not serialize their access to the endpoint.
HandleCompletion() modifies _sq_sent, _rq_received, _sbuf,
_rbuf, and the main Socket's _read_buf without endpoint-level
synchronization.
For example, concurrent SEND completions can race on:
_sbuf[_sq_sent++].clear();
This may result in an out-of-bounds _sbuf access or concurrent
IOBuf::clear() operations, matching the crash stack reported in
#3252:
butil::IOBuf::clear()
brpc::rdma::RdmaEndpoint::HandleCompletion()
brpc::rdma::RdmaEndpoint::PollCq()
brpc::Socket::ProcessEvent()
To Reproduce
There is no minimal standalone reproducer yet.
The race can be reproduced deterministically with a test hook:
- Enable RDMA and Socket health checking.
- Pause an old CQ callback immediately after reading
m->user(), but
before calling Socket::Address(ep->_socket->id(), ...).
- Force the QP into an error state.
- Allow
WaitAndReset(), RdmaEndpoint::Reset(), health checking, and
Revive() to complete.
- Complete a new RDMA handshake so that a new
_resource and
_cq_sid are installed.
- Resume the old callback while the new CQ callback is active.
- Generate SEND/RECV completions or run the test under TSAN.
Expected behavior
A CQ callback must never access an RdmaEndpoint or RdmaResource
belonging to a different Reset/Revive generation.
Reset should either wait for all callbacks of the old generation to
finish, or callbacks should carry lifetime-protected context and reject
themselves when their CQ Socket, resource, or generation is no longer
current.
A complete fix could bind each callback to immutable context such as:
{ main_socket_id, cq_socket_id, resource, generation }
After acquiring the main Socket, PollCq should validate the CQ Socket,
resource, and generation before accessing ep->_resource.
Polling-mode REMOVE operations require equivalent synchronization.
Versions
OS: Debian 12
Compiler: g++ 14.2.0
brpc: 1.14; the callback lifecycle described above is also present in master
protobuf: not relevant
Additional context/screenshots
Related issue: #3252
Related PR: #3261
CQ-draining fix: 58fec6fe
PR #3261 explicitly considered incrementing a generation in Reset()
and validating it in PollCq, but the merged implementation chose CQ
draining only.
Draining removes stale CQEs. It does not stop an old callback from
resuming after revival and selecting the newly installed
ep->_resource.
Describe the bug
An old
RdmaEndpoint::PollCqcallback can survive an RDMAReset()/health-checkRevive()cycle and then access the resources ofthe new RDMA generation.
PR #3261 / commit 58fec6f documented that a queued CQ callback may run
after the main Socket has been reset and revived. The merged fix drains
the old CQ after moving the QP to RESET, but it does not invalidate or
synchronize with a callback that has already copied the non-owning
RdmaEndpoint*from the old CQ Socket.The following interleaving is still possible:
Setting the old CQ Socket's
_usertoNULLdoes not invalidate theeppointer already copied by a running callback.PollCqverifies that the main Socket is addressable, but does notverify that:
m->id()is still equal toep->_cq_sid;ep->_resourceis the resource for which the callback was created.The old and new CQ Socket callbacks can therefore both poll the new CQ
and concurrently call
HandleCompletion()on the sameRdmaEndpoint.Because they belong to different CQ Sockets, Socket-level event
serialization does not serialize their access to the endpoint.
HandleCompletion()modifies_sq_sent,_rq_received,_sbuf,_rbuf, and the main Socket's_read_bufwithout endpoint-levelsynchronization.
For example, concurrent SEND completions can race on:
This may result in an out-of-bounds
_sbufaccess or concurrentIOBuf::clear()operations, matching the crash stack reported in#3252:
To Reproduce
There is no minimal standalone reproducer yet.
The race can be reproduced deterministically with a test hook:
m->user(), butbefore calling
Socket::Address(ep->_socket->id(), ...).WaitAndReset(),RdmaEndpoint::Reset(), health checking, andRevive()to complete._resourceand_cq_sidare installed.Expected behavior
A CQ callback must never access an
RdmaEndpointorRdmaResourcebelonging to a different Reset/Revive generation.
Reset should either wait for all callbacks of the old generation to
finish, or callbacks should carry lifetime-protected context and reject
themselves when their CQ Socket, resource, or generation is no longer
current.
A complete fix could bind each callback to immutable context such as:
After acquiring the main Socket,
PollCqshould validate the CQ Socket,resource, and generation before accessing
ep->_resource.Polling-mode REMOVE operations require equivalent synchronization.
Versions
OS: Debian 12
Compiler: g++ 14.2.0
brpc: 1.14; the callback lifecycle described above is also present in master
protobuf: not relevant
Additional context/screenshots
Related issue: #3252
Related PR: #3261
CQ-draining fix: 58fec6fe
PR #3261 explicitly considered incrementing a generation in
Reset()and validating it in
PollCq, but the merged implementation chose CQdraining only.
Draining removes stale CQEs. It does not stop an old callback from
resuming after revival and selecting the newly installed
ep->_resource.