Skip to content

[rocjitsu] Stabilize simulated doorbell lifecycle - #10195

Merged
kuhar merged 1 commit into
developfrom
users/kuhar/fix-dorbell-sanitizers
Aug 20, 2026
Merged

kuhar merged 1 commit into
developfrom
users/kuhar/fix-dorbell-sanitizers

Conversation

@kuhar

@kuhar kuhar commented Aug 15, 2026

Copy link
Copy Markdown
Member

The simulated command processor used the client-visible doorbell mapping as its polling address. ROCr can replace that mapping while queues are live, leaving the poller with a stale address. The doorbell monitor could also outlive the final host queue and continue running during process teardown.

Keep one canonical per-GPU doorbell backing and a stable shared driver view for every command-processor access. Client remaps now only republish the client view, recycled queue slots are initialized through the stable view, and unmap or close reclaims both mappings and the retained descriptor. Stop and join the monitor when the final host queue is removed, and restart it when a new queue is registered.

Add lifecycle and remap-recycle regressions, split sanitizer-specific exclusions by observed behavior, and re-enable the HSA and daemon tests that now pass reliably.

@kuhar
kuhar requested a review from atgutier as a code owner August 15, 2026 01:09
Copilot AI lite review requested due to automatic review settings August 15, 2026 01:09
@kuhar
kuhar requested a review from a team as a code owner August 15, 2026 01:09
@github-actions github-actions Bot added github actions Pull requests that update GitHub Actions code project: rocjitsu labels Aug 15, 2026
@therock-pr-bot

therock-pr-bot Bot commented Aug 15, 2026 •

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
📝 PR Description ❌ Fail Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword.
Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples:
• JIRA ID : TESTAUTO-6039
• JIRA ID - #330
• JIRA ID #330
• JIRA ID (on separate line)
ROCM-25757
• ISSUE ID : TESTUTO-3334
• ISSUE ID #3334
• ISSUE ID - TESTAUTO-3433
• ISSUE ID (on separate line)
AIRUNTIME-2352
• ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234
• Closes #10
• Fixes octo-org/octo-repo#100
• Resolves: #123
• #123
• https://github.com/<org_name>/<repo_name>/issues/123
Current: no valid JIRA/ISSUE/closing-keyword reference found
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ PR Description

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR stabilizes the simulated KFD doorbell lifecycle in rocjitsu by separating the command processor’s polling address from the client-visible doorbell mapping, ensuring the poller uses a stable per-GPU backing even when ROCr remaps doorbells. It also tightens monitor thread teardown so the monitor is stopped and joined when the last host-accessible queue is removed, and restarted when a new queue is registered.

Changes:

  • Introduces a stable driver-side doorbell alias (doorbell_monitor_page) and retained backing (doorbell_memfd) so client remaps don’t orphan the command-processor poll address.
  • Stops/joins the doorbell monitor when the final host-accessible queue is destroyed, with tests asserting both “not running” and “not joinable”.
  • Adds regression tests and refines sanitizer exclusions in CI to reflect observed behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
emulation/rocjitsu/tests/simulated_kfd_test.cpp Adds a regression test for client doorbell remap + recycled slot initialization.
emulation/rocjitsu/tests/amdgpu_vm_test.cpp Strengthens lifecycle assertions to require the monitor thread be joined after teardown.
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.h Adds test-only joinability probe; updates lifecycle docs for the monitor.
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.cpp Implements “stop/join when idle” behavior and updates monitor logging.
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp Reworks doorbell mmap/unmap/close to keep a stable poller alias and reclaimed resources.
emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/kfd_process.h Extends per-GPU process state with retained doorbell fd + stable monitor mapping.
.github/workflows/rocjitsu-corpus-tests.yml Splits sanitizer exclusions so stable tests can be re-enabled where possible.
Suppressed comments (4)

emulation/rocjitsu/tests/simulated_kfd_test.cpp:165

  • Same as above: this assumes a 4KB doorbell page by masking with 0xFFF. Use the actual mapped page size so the test remains correct on non-4KB page systems.
  doorbell_slot = reinterpret_cast<uint64_t *>(static_cast<char *>(client_page) +
                                               (second_queue.doorbell_offset & 0xFFF));

emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.h:368

  • The ensure_doorbell_monitor() documentation still refers to reaping a “self-exited” monitor and a “final self-exit check”. The self-exit logic was removed from doorbell_poll_loop(), so this comment is now misleading. It should describe the current reason for the hw_queue_mutex_ restriction (joining/replacing a jthread can block while the poll loop holds hw_queue_mutex_).

This issue also appears in the following locations of the same file:

  • line 395
  • line 401
  /// @brief Start the doorbell monitor if one is not already running, reaping a
  /// previously self-exited thread first. Serialized by doorbell_thread_mutex_.
  /// @details Caller MUST NOT hold hw_queue_mutex_: this may join a monitor that
  /// self-exited, and that monitor's final self-exit check takes hw_queue_mutex_,
  /// so joining under it would deadlock. This also fixes the lock order to

emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.h:399

  • This lifecycle comment claims the poll loop retains an empty-queue self-exit backstop and that later operations reap a returned monitor. The self-exit path was removed from doorbell_poll_loop(), and teardown now stops/joins explicitly, so this comment should be updated to match the new behavior.
  // stopping and joining the monitor, so an in-progress scan can finish. The
  // lifecycle path then rechecks the queue set while startup is excluded; this
  // keeps a concurrent registration from losing its monitor. The loop retains its
  // empty-queue self-exit as a backstop, and the next lifecycle operation reaps a
  // monitor that returned before it could be joined.

emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/command_processor.h:403

  • The doorbell_running_ comment says the monitor clears the flag under doorbell_thread_mutex_ just before returning, but the poll loop no longer mutates doorbell_running_. This should be updated so future changes don’t rely on a behavior that isn’t implemented.
  // True while a monitor is running or about to run. The monitor clears it under
  // doorbell_thread_mutex_ just before returning, so ensure_doorbell_monitor() can
  // tell a live monitor from a self-exited one that still needs joining.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@atgutier atgutier left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some agent comments:

MAJOR

  1. simulated_kfd.cpp:2441 — daemon fd-passing still uses the old allocation scan

The PR makes PerGpuState::doorbell_memfd the canonical backing, including a new memfd_create branch
for "ROCr mmaps the doorbell without a prior KFD allocation" (:906). But dispatch_get_mmap_memfd()
still resolves the doorbell by scanning proc.allocations_ for KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL and
returns -1 if absent. Two sources of truth. On -1, remote_driver.cpp:1056 falls back to
MAP_ANONYMOUS | MAP_PRIVATE — the client rings a page the CP never reads. Silent hang, no error.
That divergent case is exactly the one the new branch was added to serve.

Fix: check gs.doorbell_memfd first, fall back to the scan.

  1. simulated_kfd.cpp:1019 — driver munmaps the client's previous doorbell view

if (old_client_page && old_client_page != ptr && old_client_size)
libc_passthrough().munmap(old_client_page, old_client_size);
mmap() never invalidates a caller's earlier mapping of the same object. In daemon mode
SimulatedKfd::mmap strips MAP_FIXED and forces addr=nullptr (:855), so a remap always lands
elsewhere and always kills the old view — and the runtime caches one doorbell pointer per node.
SIGSEGV on next ring. The new test only covers the MAP_FIXED-same-address path, so this branch is
untested.

Fix: track client views as a set, or only reclaim when the new mapping is MAP_FIXED over the old
(where the kernel already replaced it).

  1. Workflow re-enablement is under-justified — rocjitsu-corpus-tests.yml:194

kfd_process.h:47 adds a comment stating TSan keys shadow state by VA and "cannot correlate stores
and loads through these distinct views; sanitizer success is not evidence for the doorbell edge
protocol itself." The same PR re-enables HsaTest.* and four DaemonTest.* under TSan. For local mode
the client store and CP load were previously the same VA and are now deliberately different — so
passing may be a consequence of the split, not a fixed race. Separately, the "ROCr-owned LSan
allocations" rationale is plausible but only explains the daemon split;
HsaTest.InitSucceeded/GpuAgentFound are un-excluded from ASan/UBSan with no rationale, and nothing
here changes their ASan behavior. Also DaemonTest.HipVectorAdd is un-excluded from both while
HipMemcpyRoundTripFloat stays ASan-excluded — unexplained asymmetry.

Fix: scope LSan suppressions to ROCr frames rather than dropping whole tests; state the observed
failure behind each exclusion/un-exclusion in the workflow; move the TSan-blindness caveat next to
the re-enablement.

MINOR

  • command_processor.h:364,393,401 + command_processor.cpp:558 — four comments still describe the
    self-exit/reap machinery this PR deleted ("reaping a previously self-exited thread first", "retains
    its empty-queue self-exit as a backstop", "The monitor clears it… so ensure_doorbell_monitor() can
    tell a live monitor from a self-exited one"). The flag is now only cleared after a join, so
    !doorbell_running_ implies !doorbell_thread_.joinable(). Rewrite, and add
    assert(!doorbell_thread_.joinable()) in ensure_doorbell_monitor() — the code currently leans on
    jthread::operator= doing an implicit request_stop()+join().
  • command_processor.cpp:638 — stop_doorbell_monitor_if_idle() has an undocumented "must not hold
    hw_queue_mutex_" precondition. It's a recursive_mutex, so a caller holding it re-acquires, passes
    the check, then joins a thread blocked on that same mutex. Hard deadlock, no assert.
    ensure_doorbell_monitor() documents this (h:366); the new one doesn't.
  • simulated_kfd.cpp:974 — MAP_FIXED client mmap isn't checked for overlap with the driver's own
    monitor_ptr alias (same address space in local mode). Overlap would destroy the alias and dangle
    every HwQueue::doorbell_base. Reject with EINVAL next to the existing size check at :933.
  • simulated_kfd.cpp:259 — is_doorbell_range() only covers gs.doorbell_page, not the new monitor
    alias, leaving it unprotected from client mprotect/munmap.
  • simulated_kfd.cpp:925-1004 — manual fd cleanup (close_new_fd lambda + new_doorbell_fd flag)
    replicated across four early returns; util::UniqueHandle already exists in the project. Same for
    monitor_ptr/new_monitor_mapping.
  • simulated_kfd.cpp:1220 vs :650 — close() guards munmap on pointer and size; dispatch_munmap guards
    on pointer only.

NIT

  • kfd_process.h:45 — new fields use // not Doxygen; the TSan methodology paragraph doesn't belong in
    a plain-data struct.
  • amdgpu_vm_test.cpp:3202 — wait_for_monitor(false) polls up to 2 s for what is now a synchronous
    stop. Assert directly; the wait would hide a regression to async teardown.
  • No test exercises the concurrent register/stop window — both new tests are single-threaded and
    would pass without the recheck-under-doorbell_thread_mutex_ that is the design's key property.

@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from fd4771e to 41cc1f3 Compare August 18, 2026 02:03
@kuhar
kuhar requested a review from atgutier August 18, 2026 02:03

@atgutier atgutier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One agent issue and CI is failing:

[MAJOR] emulation/rocjitsu/tests/rocr-lsan.supp:4 — The new function-name suppressions cannot match this lane's leak stacks

Reasoning: The workflow relies on three function-name patterns to suppress ROCr process-exit allocations, but the installed Clang sanitizer lane reports allocation frames from libhsa-runtime64.so.1 only as module-plus-offset entries. None of the function names in lines 4–6 appears in those stacks, so the suppression file has no matchable frame. The newly enabled daemon test bodies succeed, but their child processes exit nonzero after LSan reports the unsuppressed allocations.

Evidence: .github/workflows/rocjitsu-corpus-tests.yml:228 exports this suppression file. In head job 95571406939, DaemonTest.HipMemcpyRoundTripFloat, DaemonTest.HipMemcpyRoundTripInt, DaemonTest.HipMemcpyDeviceToDevice, and DaemonTest.TwoIndependentClients run successful inner HIP tests and then report AddressSanitizer: 5712 byte(s) leaked in 100 allocation(s). The allocation frames are rendered only as libhsa-runtime64.so.1+0x..., so patterns such as rocr::AMD::hsa_amd_signal_create cannot match in this environment.

Suggested fix: Either provide a compatible external symbolizer and matching ROCr debug symbols, then verify that the intended owning frames appear, or keep these daemon tests excluded from this lane. A module-level suppression may match the current output, but it would suppress all ROCr-originating leaks and materially weaken coverage; it should be used only if that tradeoff is explicitly acceptable. Add a focused CI check that proves the suppression is consumed before re-enabling the full daemon set.

Confidence: High.

@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from 41cc1f3 to 3581683 Compare August 19, 2026 00:34
@kuhar
kuhar requested a review from atgutier August 19, 2026 00:34

@atgutier atgutier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent found another serious issue:

[MAJOR] emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp:1059 — Append-only
doorbell tracking can unmap unrelated replacement memory

Reasoning: The PR records every distinct client doorbell mapping in gs.doorbell_views. Linux
MAP_FIXED can subsequently replace one of those mappings with unrelated anonymous or file-backed
memory without informing SimulatedKfd. The historical record remains, so the driver continues
treating the address as a doorbell and eventually unmaps the unrelated replacement during KFD
teardown.

A concrete sequence is:

  1. Map doorbell view A.

  2. Map another doorbell view B.

  3. Replace A with unrelated memory using MAP_FIXED.

  4. A remains in doorbell_views.

  5. mprotect(A, ...) is incorrectly rejected, and ordinary munmap(A, ...) is routed to the simulated
    driver and rejected.

  6. Closing KFD calls the real munmap(A, ...), removing the unrelated replacement mapping.

Evidence:

  • /tmp/pr10195-head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp:1059
    appends every distinct returned address to doorbell_views.

  • /tmp/pr10195-head/emulation/rocjitsu/tests/simulated_kfd_test.cpp:143 explicitly demonstrates that
    replacing a returned client view with MAP_FIXED is an expected scenario.

  • /tmp/pr10195-head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/interposer.cpp:2323 lets
    that ordinary mapping reach the real mmap, but does not retire the overlapping doorbell record.

  • /tmp/pr10195-head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp:330
    still classifies every recorded historical address as a doorbell.

  • /tmp/pr10195-head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/interposer.cpp:2326
    consequently rejects mprotect.

  • /tmp/pr10195-head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp:1235
    finds the stale view and returns EPERM for munmap while the process is live.

  • /tmp/pr10195-head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp:690
    moves every recorded view into teardown ownership, and /tmp/pr10195-head/emulation/rocjitsu/lib/
    rocjitsu/src/rocjitsu/kmd/linux/simulated_kfd.cpp:702 unmaps them unconditionally.

This is PR-scoped in the two-view case. Before this PR, the driver retained only the most recently
mapped doorbell_page. After mapping A and then B, replacing A did not cause close to unmap A. The
new vector retains both A and B, introducing the stale historical-address behavior.

Suggested fix: Before allowing a non-doorbell MAP_FIXED operation, retire every overlapping client-
view record and remove its associated GPU mapping. Preserve the canonical memfd and private monitor
alias. Alternatively, stop treating replaceable client mappings as teardown-owned after returning
them to userspace.

Add a regression that creates A and B, replaces A with writable anonymous memory, verifies mprotect
and munmap operate normally, and verifies closing KFD does not remove or corrupt A.

Confidence: high.

@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from 3581683 to b13db2f Compare August 19, 2026 01:20
@kuhar

kuhar commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@atgutier addressed

@kuhar
kuhar requested a review from atgutier August 19, 2026 01:20

@atgutier atgutier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More agent majors:

[MAJOR] emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/
interposer.cpp:2323 — DBT guest mode bypasses doorbell-view retirement

Reasoning: In simulator-backed DBT guest mode, the interposer’s active
driver is a GuestKfd, not the underlying SimulatedKfd. The new
dynamic_cast<SimulatedKfd *> therefore fails. An anonymous or otherwise
non-KFD MAP_FIXED replacement succeeds through the real mmap, but the
simulator continues treating that address as a live doorbell view.
Subsequent mprotect calls are incorrectly rejected, and closing the guest
can unmap the unrelated replacement mapping.

Evidence:

  • interposer.cpp:1219-1230 installs GuestKfd as the active driver.

  • guest_kfd.cpp:1069-1070 forwards KFD mappings to the simulated execution
    driver.

  • guest_kfd.cpp:1135-1136 forwards doorbell-range queries to that same
    driver.

  • The changed replacement path at interposer.cpp:2323-2325 recognizes only
    a directly active SimulatedKfd.

  • The stale view remains protected at interposer.cpp:2332-2335 and is
    eventually unmapped during simulator cleanup at simulated_kfd.cpp:755-
    759.

Suggested fix: Make fixed-mapping replacement a virtual LinuxKfd
operation. Implement it in SimulatedKfd and have simulator-backed GuestKfd
forward it. Add a guest-simulator test that replaces a doorbell view and
verifies that the replacement can be protected normally and survives guest
close.

Confidence: high.

[MAJOR] emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/
simulated_kfd.cpp:1071 — First fixed doorbell mapping can replace its own
monitor alias

Reasoning: For the first doorbell mapping, the driver creates the
supposedly private monitor mapping before installing the requested client
MAP_FIXED mapping. If the requested target is currently free, the kernel
may select that exact address for the monitor. The following MAP_FIXED
call replaces it, and the driver publishes the same address as both the
client view and doorbell_monitor_page. A later runtime remap is then
rejected as an attempt to overwrite the monitor alias, defeating the PR’s
central remapping fix.

Evidence:

  • The overlap check at simulated_kfd.cpp:1041-1044 occurs before a monitor
    address has been allocated.

  • The kernel chooses the monitor address at simulated_kfd.cpp:1071-1072.

  • The client MAP_FIXED operation occurs afterward at
    simulated_kfd.cpp:1087-1088.

  • The possibly identical addresses are published at
    simulated_kfd.cpp:1112-1117.

  • The replacement path subsequently rejects any mapping overlapping the
    recorded monitor at simulated_kfd.cpp:353-356.

  • A local mmap probe reproduced this exact allocation sequence: the
    recently freed target, monitor mapping, and fixed client mapping all
    received the same address.

Suggested fix: When the initial client mapping is fixed, establish it
first and then allocate the monitor alias while the client range is
occupied. Keep both mappings under RAII until publication and explicitly
verify that their ranges do not overlap. Add a first-mapping regression
using a recently unmapped fixed target.

Confidence: high.

Also a minor:

[MINOR] emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/
simulated_kfd.cpp:2558 — Unknown GPU IDs return GPU 0’s canonical doorbell
descriptor

Reasoning: gpu_ordinal() returns zero when an ID is not found. The newly
added canonical-memfd lookup therefore returns GPU 0’s descriptor for any
malformed or stale encoded GPU ID once GPU 0 has a doorbell backing.

The fallback-to-zero helper is pre-existing, but this behavior is PR-
scoped: previously, dispatch_get_mmap_memfd() searched allocations for an
exact GPU-ID match and returned -1 when none existed. The new canonical
fast path bypasses that validation.

Evidence:

  • simulated_kfd.h:222-227 maps unknown IDs to ordinal zero.

  • simulated_kfd.cpp:2553-2562 uses that ordinal before validating the
    encoded ID and returns its canonical descriptor.

  • The old exact-match allocation fallback remains at
    simulated_kfd.cpp:2566-2571.

Suggested fix: Validate db_gpu_id with find_gpu() or use an optional-
returning ordinal lookup before both doorbell mmap and memfd lookup.
Reject unknown IDs with EINVAL or return no descriptor. Add a test
querying an unknown ID after GPU 0’s canonical backing has been published.

Confidence: high.

@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from b13db2f to 32503dd Compare August 19, 2026 03:22
@kuhar
kuhar requested a review from atgutier August 19, 2026 03:23

@atgutier atgutier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] /tmp/pr10195-head-32503/emulation/rocjitsu/lib/rocjitsu/src/
rocjitsu/kmd/linux/simulated_kfd.cpp:1313 — The private monitor alias is
not protected from munmap

Reasoning: dispatch_munmap() searches only client entries in
doorbell_views. If a munmap() range overlaps the new private
doorbell_monitor_page, the driver returns -ENOENT. The /tmp/pr10195-head-
32503/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/
interposer.cpp:2925 then falls through to real libc munmap(). Live queues
retain that address in HwQueue::doorbell_base, which the /tmp/pr10195-
head-32503/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/amdgpu/
command_processor.cpp:694 dereferences, potentially producing a SIGSEGV.

Evidence: The PR publishes the alias at lines 1144–1151 and explicitly
protects it from MAP_FIXED at lines 1042–1048 and from mprotect() through
is_doorbell_range(), but provides no equivalent munmap() protection.

This is PR-introduced. Normal ROCr teardown unmaps the returned client
view, not the undisclosed alias, so the trigger requires an application or
allocator to explicitly or broadly unmap a range containing the private
alias. Nevertheless, that defeats the PR’s stable-alias invariant and
leaves a live dangling pointer.

Suggested fix: While holding alloc_mutex_, reject any munmap() range
overlapping doorbell_monitor_page with EPERM, ensuring the interposer does
not fall through. Test exact, partial, and containing overlaps while a
queue is active.

Confidence: high on the failure mechanism; medium on how frequently
nonstandard applications would trigger it.

[MINOR] /tmp/pr10195-head-32503/emulation/rocjitsu/lib/rocjitsu/src/
rocjitsu/kmd/linux/simulated_kfd.cpp:369 — Partial MAP_FIXED replacement
retires the entire multi-page doorbell view

Reasoning: Any overlap with a DoorbellView erases the whole entry and
removes its entire GPU mapping at lines 373–379. However, MAP_FIXED
replaces only its requested page-rounded range. If one page of an 8-KiB
doorbell mapping is replaced, the other page remains mapped in the process
but is no longer tracked, protected by is_doorbell_range(), GPU-mapped, or
reclaimed during KFD close.

Evidence: The bundled runtime calculates modern 8-byte doorbell mappings
as 1024 × 8 bytes in /tmp/pr10195-head-32503/projects/rocr-runtime/
libhsakmt/src/queues.c:206, making them 8 KiB. The new replacement
regression uses only one host page and replaces the complete test mapping,
so it does not cover fragmentation.

This path is also PR-introduced. Normal ROCr maps and unmaps the complete
8-KiB range, so the problematic trigger is a partial, overlapping ordinary
MAP_FIXED.

Suggested fix: Either retain/split the unaffected fragments and remove
only the replaced GPU range, or explicitly reject partial overlap. Add an
8-KiB test that replaces one 4-KiB constituent page.

Confidence: high on the mechanism; medium on runtime frequency.

@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from 32503dd to 8cc203c Compare August 19, 2026 13:13
@kuhar
kuhar requested a review from atgutier August 19, 2026 13:13

@atgutier atgutier left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more agent comment and then I think it can be shipped:

MAJOR — A failed fixed doorbell mapping can destroy the caller’s existing mapping.
/tmp/pr10195-review-codex/head/emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/kmd/linux/
simulated_kfd.cpp:1093

For the first fixed doorbell mapping, the code performs the destructive client MAP_FIXED before
allocating the private monitor mapping. If that second mmap fails, cleanup at line 1121 unmaps the
newly installed client mapping and returns MAP_FAILED.

Any mapping or reservation previously occupying the requested address was already displaced by
MAP_FIXED and cannot be restored. Consequently, an ostensibly failed doorbell mmap can leave the
caller’s target range unmapped and discard live contents. This is reachable under VA/VMA or other
mapping-resource exhaustion.

The merge-base implementation avoided this failure ordering by completing its fallible temporary
mapping before the final client MAP_FIXED.

Recommended fix: finish monitor/backing allocation before issuing the client’s destructive fixed
mapping. Handle the case where the monitor allocation lands at the requested address by
establishing another alias before releasing it and performing the fixed map. Add deterministic
failure injection that begins with a populated mapping at the target, forces monitor-map failure,
and verifies the original mapping and contents remain intact.

Confidence: high; the destructive ordering and cleanup behavior are explicit.

@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from 8cc203c to f5372ff Compare August 20, 2026 01:41
The simulated command processor used the client-visible doorbell mapping as its polling address. ROCr can replace that mapping while queues are live, leaving the poller with a stale address. The doorbell monitor could also outlive the final host queue and continue running during process teardown.

Keep one canonical per-GPU doorbell backing and a stable shared driver view for every command-processor access. Client remaps now only republish the client view, recycled queue slots are initialized through the stable view, and unmap or close reclaims both mappings and the retained descriptor. Stop and join the monitor when the final host queue is removed, and restart it when a new queue is registered.

Add lifecycle and remap-recycle regressions, split sanitizer-specific exclusions by observed behavior, and re-enable the HSA and daemon tests that now pass reliably.
@kuhar
kuhar force-pushed the users/kuhar/fix-dorbell-sanitizers branch from f5372ff to 63a62a1 Compare August 20, 2026 02:51
@kuhar
kuhar merged commit 2190044 into develop Aug 20, 2026
52 of 53 checks passed
@kuhar
kuhar deleted the users/kuhar/fix-dorbell-sanitizers branch August 20, 2026 04:40
cmcknigh pushed a commit that referenced this pull request Aug 25, 2026
The simulated command processor used the client-visible doorbell mapping
as its polling address. ROCr can replace that mapping while queues are
live, leaving the poller with a stale address. The doorbell monitor
could also outlive the final host queue and continue running during
process teardown.

Keep one canonical per-GPU doorbell backing and a stable shared driver
view for every command-processor access. Client remaps now only
republish the client view, recycled queue slots are initialized through
the stable view, and unmap or close reclaims both mappings and the
retained descriptor. Stop and join the monitor when the final host queue
is removed, and restart it when a new queue is registered.

Add lifecycle and remap-recycle regressions, split sanitizer-specific
exclusions by observed behavior, and re-enable the HSA and daemon tests
that now pass reliably.
ajanicijamd pushed a commit that referenced this pull request Aug 25, 2026
The simulated command processor used the client-visible doorbell mapping
as its polling address. ROCr can replace that mapping while queues are
live, leaving the poller with a stale address. The doorbell monitor
could also outlive the final host queue and continue running during
process teardown.

Keep one canonical per-GPU doorbell backing and a stable shared driver
view for every command-processor access. Client remaps now only
republish the client view, recycled queue slots are initialized through
the stable view, and unmap or close reclaims both mappings and the
retained descriptor. Stop and join the monitor when the final host queue
is removed, and restart it when a new queue is registered.

Add lifecycle and remap-recycle regressions, split sanitizer-specific
exclusions by observed behavior, and re-enable the HSA and daemon tests
that now pass reliably.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants