fix(rocjitsu): Fault invalid GPU accesses instead of dereferencing them - #10937
Open
atgutier wants to merge 1 commit into
Open
fix(rocjitsu): Fault invalid GPU accesses instead of dereferencing them#10937atgutier wants to merge 1 commit into
atgutier wants to merge 1 commit into
Conversation
In local mode a GPU address that misses the page table is reinterpreted as a host address by identity, and the resulting pointer is dereferenced with no validation at all outside an ASan build. An address invented by a defect elsewhere in the simulator is therefore a host SIGSEGV or, worse, a silent write into an unrelated live allocation -- neither attributable to the GPU access that caused it. Existence alone is not enough either: an atomic is performed in place, so a queue read pointer the application mapped PROT_READ kills the process, and a client-owned address the kernel refuses is answered out of simulator-private sparse storage the client can never observe, which presents as a hang attributed to nothing. Consumers compounded both, leaving an unreachable endpoint pending forever or fabricating the bytes and publishing a completion signal for a transfer that never happened. Validate before dereferencing, and raise a refusal as a KFD memory exception against the owning process the way hardware would, deferred out of the translation locks the driver's own lock order runs against. Existence is a one-byte kernel probe; writability is the kernel's own record of the mapping, read through raw syscalls because this path holds the page-table lock while the interposed open() and close() take the descriptor lock a GEM_VA ioctl already holds when it calls into the page table. A shared mapping lock spans the check and the store, and every mapping change takes it exclusively -- those the interposer sees and those the driver makes for itself. Protected, absent and undetermined stay distinct, so descriptor pressure is never reported as a protection violation; a client-owned address is answered only by the client rather than by sparse storage; an atomic whose range is not wholly backed is refused rather than torn across the bytes that happen to exist; and every consumer stops where hardware would, ending at the faulted page instead of modifying the pages behind it and halting a faulted queue instead of retrying an endpoint that will never resolve. The mapping lock is a util::ObservableSharedMutex, which counts blocked writers so a test can wait for exclusion to be a fact rather than infer it from a sleep that an implementation taking no lock at all would also satisfy.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens rocjitsu local-mode memory handling so unresolved/invalid GPU virtual addresses (and unwritable host pages behind identity translations) are refused and surfaced to the runtime as KFD memory-exception events, rather than being dereferenced as raw host pointers (preventing host SIGSEGVs, silent corruption, and un-attributable hangs). It also introduces a process-wide host mapping lock to serialize mapping mutations against in-flight atomics and updates SDMA/CP behavior to halt on faulted packets instead of retrying indefinitely or publishing false completion.
Changes:
- Add fault-aware GPU memory access outcomes (complete/unavailable/faulted), validate identity translations before dereference, and defer fault delivery outside translation locks.
- Introduce a global per-process mapping lock (observable blocked-writer count) and apply it in both the interposer and driver mapping syscalls.
- Update command processor SDMA handling to halt on faulted packets (and suppress completion publication), plus add extensive regression tests for fault reporting and lock behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| emulation/rocjitsu/tests/simulated_kfd_test.cpp | Adds tests for mapping-lock exclusion against driver mmap/munmap and for memory-exception event reporting (including GPU attribution). |
| emulation/rocjitsu/tests/shared_infra_test.cpp | Updates copy_block assertions to match new CopyOutcome API. |
| emulation/rocjitsu/tests/cdna5_sdma_test.cpp | Adds SDMA regression tests for wrapped ranges, clipped metadata, faulted endpoints, and unpublishable read pointers. |
| emulation/rocjitsu/tests/amdgpu_vm_test.cpp | Adds extensive GpuMemory tests for identity validation, fault classification, atomic refusal semantics, and deferred fault dispatch ordering. |
| emulation/rocjitsu/lib/util/include/util/observable_shared_mutex.h | Introduces an observable shared mutex used to prove writer blocking in tests. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/gpu_memory.h | Implements identity-page validation, writability probing, fault dispatch/defer, new AccessOutcome/CopyOutcome, and checked atomics/copies. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.h | Adds per-queue “faulted” state and changes write_gpu_block to return AccessOutcome. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.cpp | Uses checked block/atomic APIs, halts SDMA queues on fault, and avoids publishing completion for faulted work. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.h | Adds per-GPU fault reporter plumbing and GPU-id access helper. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp | Binds per-GPU reporters, implements memory-fault event delivery, and wraps driver mapping calls with the host mapping lock. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/libc_passthrough.h | Adds mremap to passthrough table. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/libc_passthrough.cpp | Resolves mremap symbol in passthrough table. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/interposer.cpp | Serializes mmap/munmap/mprotect/mremap with the mapping lock in the interposer. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/host_mapping_lock.h | Adds the process-wide observable shared mutex accessor for mapping serialization. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/events.h | Introduces MemoryFault payload struct and EventState::signal_memory_fault API. |
| emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/events.cpp | Implements signal_memory_fault and populates KFD memory-exception data in WAIT_EVENTS. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+37
to
+41
| explicit ExclusiveGuard(ObservableSharedMutex &owner) : owner_(owner) { | ||
| owner_.blocked_writers_.fetch_add(1, std::memory_order_release); | ||
| lock_ = std::unique_lock(owner_.mutex_); | ||
| owner_.blocked_writers_.fetch_sub(1, std::memory_order_release); | ||
| } |
Comment on lines
+118
to
+122
| /// @brief Report that @p addr could not be serviced for @p vmid. | ||
| /// @details Called from simulation threads, so implementations must be | ||
| /// thread-safe and must not re-enter GpuMemory. Each GpuMemory binds its own | ||
| /// reporter, so the implementation knows which device faulted without being | ||
| /// told; a shared reporter would have to guess, and would name the wrong one. |
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.
In local mode a GPU address that misses the page table is reinterpreted as a host address by identity, and the resulting pointer is dereferenced with no validation at all outside an ASan build. An address invented by a defect elsewhere in the simulator is therefore a host SIGSEGV or, worse, a silent write into an unrelated live allocation -- neither attributable to the GPU access that caused it. Existence alone is not enough either: an atomic is performed in place, so a queue read pointer the application mapped PROT_READ kills the process, and a client-owned address the kernel refuses is answered out of simulator-private sparse storage the client can never observe, which presents as a hang attributed to nothing. Consumers compounded both, leaving an unreachable endpoint pending forever or fabricating the bytes and publishing a completion signal for a transfer that never happened.
Validate before dereferencing, and raise a refusal as a KFD memory exception against the owning process the way hardware would, deferred out of the translation locks the driver's own lock order runs against. Existence is a one-byte kernel probe; writability is the kernel's own record of the mapping, read through raw syscalls because this path holds the page-table lock while the interposed open() and close() take the descriptor lock a GEM_VA ioctl already holds when it calls into the page table. A shared mapping lock spans the check and the store, and every mapping change takes it exclusively -- those the interposer sees and those the driver makes for itself. Protected, absent and undetermined stay distinct, so descriptor pressure is never reported as a protection violation; a client-owned address is answered only by the client rather than by sparse storage; an atomic whose range is not wholly backed is refused rather than torn across the bytes that happen to exist; and every consumer stops where hardware would, ending at the faulted page instead of modifying the pages behind it and halting a faulted queue instead of retrying an endpoint that will never resolve. The mapping lock is a util::ObservableSharedMutex, which counts blocked writers so a test can wait for exclusion to be a fact rather than infer it from a sleep that an implementation taking no lock at all would also satisfy.
Closes #10835