Repository navigation
fix(memory): structurally pair capability allocation/release (#1186 Phase 2 round-5 fix) - #1233
Closed
justinchuby wants to merge 1 commit into
Closed
justinchuby wants to merge 1 commit into
justinchuby wants to merge 1 commit into
Conversation
… redesign)
Round 5 review found the round-4 patch direction structurally unsafe:
capability release was validated by a self-reported MechanismId rather
than by construction, and several release paths gave back tracking
state (VA spans, granule refs, free lists) before or regardless of
whether the underlying CUDA release actually succeeded.
This commit redesigns the capability seam instead of patching it further:
- `onnx-runtime-memory-api`: introduce `ReleaseReport { unmapped_bytes,
complete }` as the return type for every release-shaped capability
method (`DeviceAllocator::deallocate_with_unmapped`,
`VirtualBacking::deallocate_committed`/`decommit_allocation_range`,
`SharedMapping::release_shared_mapping`), replacing bare `u64`. This
removes the "0 means nothing was mapped" vs "0 means release failed"
conflation the review identified: failure is now `Err(MemoryError)`,
and partial progress is `Ok(ReleaseReport::partial(n))`.
Introduce `DeviceMemoryMechanism`, one owned capability bundle that
wraps a *single* `Arc<T>` and populates its allocator/virtual-backing/
shared-mapping fields via `Arc::clone` of that same object. Its
constructors (`eager`, `with_virtual_backing`, `with_shared_mapping`,
`with_virtual_backing_and_shared_mapping`) each require the same
concrete `T` to implement every requested trait simultaneously, which
makes foreign-capability composition (a wrapper whose
`as_virtual_backing()` override lies about which object backs it) a
compile-time impossibility rather than a runtime check. This replaces
the forgeable `MechanismId::of(self)` identity check removed by this
commit: there is no longer any self-reported identity to forge.
`Any` is no longer a supertrait bound anywhere in the base
`DeviceAllocator` contract, so non-`'static` borrowed adapters remain
legal, addressing round 4's blocker 2.
- `onnx-runtime-cuda-memory`: rewrite `vmm_allocator.rs`'s release paths
(`deallocate_span`, `decommit_allocation_range`, and the rollback path
inside `allocate_committed`) to a reconcile-before-release-tracking
discipline: granule/span state is only removed from `live` tracking
and returned to the free list *after* the underlying CUDA unmap/handle
return succeeds, and any leftover state from a partial failure is
reconciled (kept tracked, not silently dropped) rather than given
back regardless of outcome. `release_granules_reconciled` returns
both the unresolved (still-owned) granules and the bytes that were
actually released, so callers can never double-count or leak
visibility into the fact that something didn't unmap. Uncommitted
rollback that cannot fully unwind is now counted via a bounded
`GLOBAL_ROLLBACK_LEAKS` counter instead of corrupting live/free-list
state to force success.
- `onnx-runtime-ep-cuda`: `CudaExecutionProvider` now stores one
`DeviceMemoryMechanism` instead of a separate `memory` +
`OnceLock<vmm>` pair, so there is exactly one selected mechanism per
provider, chosen once at construction/binding and never retargeted.
The `with_memory` family (`with_memory`,
`with_memory_and_virtual_backing`, `with_memory_and_shared_mapping`,
`with_memory_and_capabilities`) each build a `DeviceMemoryMechanism`
from one concrete allocator type and reject only on a genuine
after-VMM-selected conflict or a device mismatch -- never by
re-validating a capability's self-reported identity. Two GPU tests
are rewritten to prove the new API's structural closure directly
(`with_memory_never_surfaces_a_composites_foreign_virtual_backing`,
`with_memory_never_surfaces_a_foreign_vmm_embedded_at_offset_zero`):
a composite/offset-zero adversarial object passed to eager
`with_memory` compiles (since it only needs `DeviceAllocator`), but
its foreign capability overrides can never surface through the
resulting mechanism, because `DeviceMemoryMechanism::eager` never
looks at them.
`decommit_allocation_range`/`deallocate_with_unmapped` (the
`ExecutionProvider` trait's existing methods, out of Phase 2 scope)
extract `.unmapped_bytes` from the new `Result<ReleaseReport,
MemoryError>`-returning capability calls and surface errors via the
existing `EpError::Memory`/`EpError::KernelFailed` variants.
Targeted validation (see PR body for full detail):
cargo test -p onnx-runtime-memory-api -p onnx-runtime-memory-governor
-p onnx-runtime-cuda-memory -p onnx-runtime-ep-cuda
cargo test -p onnx-genai-ort --test governed_allocator_session
cargo clippy (targeted crates, -D warnings, --no-deps for ep-cuda)
cargo doc (targeted crates, RUSTDOCFLAGS="-D warnings", --no-deps)
cargo check --workspace --tests --exclude onnx-genai-bench
Part of #1186. Phase 2 only; no Phase 3/4 scope touched.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b2f3675-eff5-46ec-b60a-74f24d52984b
justinchuby
marked this pull request as draft
August 18, 2026 08:04
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
## Summary - Reverts the exact merged squash commit `5c181b49ed9ee073639f1ee3df11ef7c597eec46` from #1192. - Withdraws the bundled #1186 Phase 1 + Phase 2 memory capability split rather than attempting another fix-forward patch. - Preserves unrelated mainline work merged after the squash. Part of #1186. ## Why revert first Subsequent high-effort review found the merged release/identity design structurally unsafe: - shared-mapping release can race free-list address reuse; - `0` conflates a legitimate no-op/refund result with failure; - partial CUDA unmap and handle-release failures can leave software and driver state divergent; - pointer-only identity is vulnerable to ABA/address reuse; - the contract makes mechanism-identity guarantees it cannot actually prove. These are lifecycle and ownership hazards, not isolated implementation bugs. Leaving the contract on `main` while iterating would expose callers to unsafe semantics, so this PR withdraws it completely. ## Related work - #1192 merged the Phase 1 branch into `main` as squash `5c181b49`. - #1206 merged Phase 2 into the Phase 1 branch and was therefore bundled into that squash. - Draft #1233 attempted to structurally pair capability allocation/release. - Draft #1237 attempted a narrower corrective rollback of the identity/atomic-release apparatus. Those drafts do not eliminate the need to withdraw the already-merged contract. A safe Phase 1 and a deliberately narrowed Phase 2 will be re-landed as a new reviewable stack after redesign. ## Validation - `cargo metadata --locked --no-deps --format-version 1` - `cargo check --locked --all-targets -p onnx-runtime-memory-governor -p onnx-runtime-cuda-memory -p onnx-runtime-ep-api -p onnx-runtime-ep-cuda -p onnx-genai-ort` - `cargo test --locked -p onnx-runtime-memory-governor --lib` — 64 passed - `cargo test --locked -p onnx-runtime-cuda-memory --lib` — 8 passed - `cargo test --locked -p onnx-runtime-ep-api --lib provider::tests::` — 6 passed - `cargo test --locked -p onnx-runtime-ep-cuda --lib provider::tests::` — 8 passed, 6 GPU-gated tests ignored - `cargo test --locked -p onnx-genai-ort --lib governed_allocator::tests::` — 20 passed - `git diff --check` Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Superseded by the safety revert in #1247. Phase 1 and a narrower Phase 2 will be re-landed as a fresh reviewed stack; this fix-forward design must not be merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1186
Important context: PRs #1192 and #1206 were merged before round-5 review fixes landed
While the round-5 review of PR #1206 (Phase 2) was in progress, both
#1206 (base
justinchuby-1186-memory-api-phase-1, containing Phase 2through round-4 fixes) and #1192 (base
main, "phases 1-2") weremerged, and both source branches were deleted. This happened outside
this session's control — the round-5 review findings below were
never applied to those merged PRs, so
maincurrently contains theround-4 state, which round-5 review explicitly found structurally
unsafe.
Since the original branch stack (
justinchuby-1186-memory-api-phase-1->
justinchuby-1186-memory-api-phase-2) no longer exists, this PRcarries the completed round-5 redesign as a direct follow-up against
main. It contains only the round-5 fix commit — verified tocherry-pick cleanly with zero conflicts, because the file contents at
the round-4 head (
6845826e) are byte-for-byte identical to whatsquash-merging left in
main(confirmed via direct diff of everytouched file). No Phase 1/Phase 2 content is duplicated; this is a
pure incremental fix on top of what's already in
main.What round-5 review found (structurally unsafe, not just buggy)
release_shared_mapping-style code removed live tracking (VAspans, granule refs) and gave the address/space back to the free
list before, or regardless of whether, the underlying CUDA
unmap/handle-return actually succeeded — a UAF/double-free-adjacent
race: another thread could reuse the same address while the first
release was still in flight or had failed.
0return conflated "nothing was mapped" with "releasefailed", so a caller (including the base-trait default path) had no
way to distinguish a successful no-op from a failure, and would
proceed as if it had succeeded either way.
CudaVmmAllocator's granule-release paths removed live tracking andreturned free-list space unconditionally, so a partial CUDA
failure (some granules unmapped, others not) could not be
represented — tracking/free-list state would diverge from the
actual mapping state.
MechanismId(a self-reported{ptr, TypeId}pair) isnot an unforgeable identity proof: an implementation can report a
foreign object's identity, ZST/address-reuse can produce spurious
collisions, and it can also wrongly reject legitimate transparent
wrappers.
The redesign (this commit)
Rather than continuing to patch the identity check, the release seam
was redesigned so capability release is structurally paired with
capability allocation instead of being validated by self-reported
identity:
ReleaseReport { unmapped_bytes: u64, complete: bool }replacesthe bare
u64return of every release-shaped capability method:DeviceAllocator::deallocate_with_unmapped,VirtualBacking::deallocate_committed,VirtualBacking::decommit_allocation_range, andSharedMapping::release_shared_mapping. Failure is nowErr(MemoryError); a genuine "0 bytes were mapped, and releasecompleted" is
Ok(ReleaseReport::complete(0)); a partial CUDAfailure is
Ok(ReleaseReport::partial(n)).0and "failed" can nolonger be confused at any point in the chain (closes finding 2).
DeviceMemoryMechanism, one owned capability bundle, replacesthe forgeable identity check entirely. Its constructors (
eager,with_virtual_backing,with_shared_mapping,with_virtual_backing_and_shared_mapping) each take exactly oneArc<T>and populate the allocator/virtual-backing/shared-mappingfields via
Arc::cloneof that same object, requiring the sameconcrete
Tto implement every requested trait simultaneously. Acomposite/wrapper object whose
as_virtual_backing()overrideclaims a foreign backing can still compile against the minimal
with_memory(which only needsDeviceAllocator), butDeviceMemoryMechanism::eagernever calls that override, so theforeign capability can never surface. This closes finding 4
structurally — there is no self-reported identity left to forge,
so no check can be fooled by it. Proven by two rewritten tests:
with_memory_never_surfaces_a_composites_foreign_virtual_backingand
with_memory_never_surfaces_a_foreign_vmm_embedded_at_offset_zero(the latter specifically reproduces the offset-zero
#[repr(C)]-adversarial construction from the prior review round).Anyis no longer a supertrait bound on the baseDeviceAllocatortrait anywhere, so non-
'staticborrowed adapters remain legal(this also closes round-4's blocker 2, which is still in scope here
since it was never separately fixed/merged).
vmm_allocator.rs's release paths (deallocate_span,decommit_allocation_range, and theallocate_committedrollbackpath) now follow a reconcile-before-mutating-tracking discipline:
granule/span state is removed from
livetracking and returned tothe free list only after the underlying CUDA unmap/handle-return
call has succeeded for that granule/span.
release_granules_reconciledreturns both the bytes actually released and the set of granules
that remain owned (i.e., were not successfully released), so a
partial failure leaves the allocator's internal state consistent
with what's actually mapped -- nothing is double-counted, nothing is
silently given back that wasn't actually freed. This closes findings
1 and 3. Uncommitted-span rollback that cannot fully unwind is now
counted through a bounded
GLOBAL_ROLLBACK_LEAKScounter instead offorcing success by corrupting live/free-list state.
CudaExecutionProvidernow stores oneDeviceMemoryMechanisminstead of a separate
memory+OnceLock<vmm>pair, so exactly onemechanism is selected per provider, chosen once at
construction/binding and never retargeted after allocations exist
(this preserves the construction-time/
OnceLockanalysis from theearlier architectural clarification in this epic). The
with_memory/with_memory_and_virtual_backing/with_memory_and_shared_mapping/with_memory_and_capabilitiesfamily rejects only on a genuine after-VMM-selected conflict or a
device mismatch -- never by re-validating a capability's
self-reported identity, since there is no longer any such identity
to validate.
Compatibility
DeviceAllocator's minimal contract (device identity +allocate/deallocate) is unchanged; eager CPU/CUDA allocators, ORT
allocator integration, and third-party governor/allocator
implementations are unaffected except for the
ReleaseReportreturntype change on the one release-shaped method they may override
(default implementation preserved for allocators that don't need
mapped-zone accounting).
ProcessMemoryManager, noRAII allocation handle/deferred-free queue, no plugin ABI change, no
ExecutionProvider::allocator(). EP stream/context responsibilitiesare untouched.
epic-level decision that only a future, separately-labeled Phase 7
will do that once VMM fully subsumes it).
Validation
No Phase 3/4 work is included. This closes out the round-5 review
findings for #1186 Phase 2.