legionxiong opened a new issue, #3410:
URL: https://github.com/apache/brpc/issues/3410
# [rdma] stale CQ callback can survive Reset/Revive and poll a
new-generation CQ
**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 58fec6fe 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:
```text
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:
```cpp
_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:
```text
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:
1. Enable RDMA and Socket health checking.
2. Pause an old CQ callback immediately after reading `m->user()`, but
before calling `Socket::Address(ep->_socket->id(), ...)`.
3. Force the QP into an error state.
4. Allow `WaitAndReset()`, `RdmaEndpoint::Reset()`, health checking, and
`Revive()` to complete.
5. Complete a new RDMA handshake so that a new `_resource` and
`_cq_sid` are installed.
6. Resume the old callback while the new CQ callback is active.
7. 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:
```text
{ 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: https://github.com/apache/brpc/issues/3252
Related PR: https://github.com/apache/brpc/pull/3261
CQ-draining fix: https://github.com/apache/brpc/commit/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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
