fix request resources deallocation race - #6938
Conversation
yegorskii
added
blockstore
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
|
bc9e08b to
7115fc9
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
🟢 linux-x86_64-release-tsan target: cloud/blockstore/,cloud/storage/ (test time: 1196s): all tests PASSED for commit 7115fc9.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
🟢 linux-x86_64-relwithdebinfo target: cloud/storage/ (test time: 87s): all tests PASSED for commit 7115fc9.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1295s): all tests PASSED for commit 7115fc9.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
🟢 linux-x86_64-release-asan target: cloud/blockstore/,cloud/storage/ (test time: 1290s): all tests PASSED for commit 7115fc9.
|
| // req->OutBuffer/OutMemoryWindow may belong to an already torn down | ||
| // pool generation/PD. | ||
| auto* handler = req->Handler.get(); | ||
| handler->HandleResponse(std::move(req), RDMA_PROTO_FAIL, 0); |
There was a problem hiding this comment.
Did we check that all existing clients correctly handle responseBytes=0 case? E.g. not trying to parse the response?
| // belong to an older pool generation and cannot be safely | ||
| // released via the current pool/PD, so just drop them without | ||
| // touching SendBuffers/RecvBuffers or calling ibv_dealloc_mw. | ||
| req->InMemoryWindow.release(); |
There was a problem hiding this comment.
Wait, are we just leaking them? This feels wrong.
I'm not 100% sure that PD is ref-counted, but did we try to actually deallocate them? Even if calling ibv_dealloc_mw is unsafe, we need to at least free them
There was a problem hiding this comment.
From the man
ibv_dereg_mr() fails if any memory window is still bound to this MR
There was a problem hiding this comment.
And also
ibv_dealloc_pd() may fail if any other resource is still associated with the PD being freed
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1306s): some tests FAILED for commit b406ca2.
🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1079s): some tests FAILED for commit b406ca2.
🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 886s): some tests FAILED for commit b406ca2.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
🟢 linux-x86_64-release-asan target: cloud/blockstore/,cloud/storage/ (test time: 1102s): all tests PASSED for commit ddbc658.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
🟢 linux-x86_64-relwithdebinfo target: cloud/storage/ (test time: 86s): all tests PASSED for commit ddbc658.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
🟢 linux-x86_64-relwithdebinfo target: cloud/storage/ (test time: 84s): all tests PASSED for commit 9f38bee.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1337s): all tests PASSED for commit 9f38bee.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-asan have completed. Tip Planned checks for linux-x86_64-release-asan.
🟢 linux-x86_64-release-asan target: cloud/blockstore/,cloud/storage/ (test time: 1239s): all tests PASSED for commit 9f38bee.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-release-tsan have completed. Tip Planned checks for linux-x86_64-release-tsan.
🟢 linux-x86_64-release-tsan target: cloud/blockstore/,cloud/storage/ (test time: 1173s): all tests PASSED for commit 9f38bee.
|
2 participants
Notes
Fix a use-after-free in the RDMA client that can occur when an endpoint
reconnects while a request is still holding buffers from the connection that's
being torn down.
The problem. On every reconnect
TClientEndpoint::CreateQP()reinitializesSendBuffers/RecvBuffers.TBufferPool::Init()replaces the whole internalImpl, which destroys everyTChunkof the previous generation — and~TChunk()both deregisters the memory region andfree()s the chunk memory.A
TPooledBufferhanded out earlier keeps a rawTChunk*and an address insidethat chunk, so after a reconnect both are dangling. Two places didn't know about
this:
FreeRequest()(called from~TRequest(), whenever the caller drops itsTClientRequestPtr) unconditionally calledSendBuffers.ReleaseBuffer()/RecvBuffers.ReleaseBuffer(). Request lifetimeis controlled by the external caller, not by the endpoint, so a request that
outlived a reconnect would dereference a freed
TChunk.SendRequest()only checkedCheckState(EEndpointState::Connected), notwhich pool generation the request's buffers came from. If the endpoint
reconnected between
AllocateRequest()andSendRequest()— a windowinvisible to the endpoint's own tracking, since the request isn't enqueued
anywhere yet — a stale request would still be accepted, and its buffers used
for
IBV_WR_BIND_MW/PostSendor written to byAbortRequest().The fix. Added
BufferPoolGeneration, an atomic counter bumped inCreateQP(). The bump, bothInit()calls and theSendBuffer/RecvBufferacquisition now happen under
AllocationLock, so "generation + pool contents"is a single atomic unit for every thread;
AllocateRequest()stamps the requestwith the current generation inside the same lock, together with its buffer
acquisition. Three call sites check the stamp:
SendRequest()rejects requests from a stale generation, reportingRDMA_PROTO_FAILthrough the newRejectStaleGenerationRequest()helperinstead of
AbortRequest()(which serializes the error intoOutBuffer—unsafe for a stale request).
ResponseBufferis pointed at a preallocated,pre-serialized
E_RDMA_UNAVAILABLEerror with static storage duration.HandleQueuedRequests()repeats the check beforeStartRequest(). Thecaller's thread can be preempted between the check in
SendRequest()andInputRequests.Enqueue(), so a request can land in the queue afterAbortRequests()has already drained it and before the pools are replaced.FreeRequest()skips releasing buffers for a stale generation and just dropsthem: the chunk they came from no longer exists, so there is nothing to return
and nothing to leak in the current pool.
Also in this change:
AbortRequest()now eagerly destroys both memory windows(destroying a window invalidates it, which guarantees no remote write can
succeed afterwards), and
SerializeError()gained aTString-returningoverload for the preallocated error above.
Issue
#5456