Skip to content

feat(rocjitsu): route AMDKFD_IOC_DBG_TRAP into a debug-session dispatcher - #8393

Merged
amd-arosa merged 32 commits into
developfrom
users/arosa/rj/rocgdb/03
Jul 15, 2026
Merged

amd-arosa merged 32 commits into
developfrom
users/arosa/rj/rocgdb/03

Conversation

@amd-arosa

@amd-arosa amd-arosa commented Jul 10, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

rocm-dbgapi drives every debugger action through the AMDKFD_IOC_DBG_TRAP ioctl.
rocjitsu's KFD emulation dropped that ioctl into the silent default: return 0
arm, so a debugger saw every operation "succeed" with no effect. This routes it
into a real per-process debug-session dispatcher that enforces the driver's
validation contract, and — for daemon-served workloads (mirage,
rocjitsu --daemon) — carries the debugger's notifier fd across the process
boundary so a later wave-stop can actually wake the debugger.

Resolves #8281.

Technical Details

Debug-session dispatcher

Adds a DBG_TRAP dispatcher that mirrors the kernel's
kfd_ioctl_set_debug_trap() validation ladder (amd/amdkfd/kfd_chardev.c)
before its op switch.

  • Add KfdProcess::DebugSession (enabled, runtime_state,
    exception_enable_mask, dbg_fd, owned_dbg_fd, debugger_pid) guarded by a
    per-process debug_mutex_; each field is documented against its kfd_priv.h
    counterpart.
  • Record the caller's Linux pid at open() so DBG_TRAP can resolve a
    self-debug target (args->pid == getpid()), mirroring the kernel's
    kfd_lookup_process_by_pid().
  • SimulatedKfd::debug_trap_ioctl enforces the pre-switch ladder: unknown pid →
    ESRCH; a foreign client driving a session it does not own → EPERM; an op
    without an enabled session → EINVAL; a HW-op before runtime-enable → EPERM;
    an address-watch op with a bad gpu_id → ENODEV; a second ENABLE →
    EALREADY.
  • ENABLE validates dbg_fd (fcntl(F_GETFD) → EBADF for an unusable
    descriptor, matching the kernel's fget()), allocates the session, and copies
    kfd_runtime_info (r_debug, ttmp_setup, runtime_state) back to the
    debugger — a truncating min(user_size, sizeof) copy that still reports the
    full struct size, as the kernel does. ENABLE now snapshots the runtime state
    under a single runtime_mutex_ acquisition (lock order debug_mutex_ →
    runtime_mutex_) so the marshaled runtime_state cannot disagree with
    r_debug/ttmp_setup.
  • DISABLE tears the session down. Recognized-but-unwired ops
    (QUERY_DEBUG_EVENT, SET_EXCEPTIONS_ENABLED, GET_DEVICE_SNAPSHOT, …)
    return -ENOSYS so a debugger can tell a stubbed-but-valid op apart from a
    genuinely unknown one, which still returns -EINVAL. Each op graduates out of
    the ENOSYS group as its handler lands.

Daemon transport — notifier fd via SCM_RIGHTS

In daemon mode the debugger's dbg_fd is a number in the client's fd table and
is meaningless to the daemon. The client hands the real descriptor over
out-of-band as SCM_RIGHTS ancillary data on DBG_TRAP ENABLE; the daemon
receives it in its own fd space and the rj_vm_execute_as() glue substitutes it
into the ioctl's dbg_fd — the same fd the real kernel would receive through the
ioctl.

  • Wired end-to-end through RemoteDriver (client), the C++ daemon
    (tools/rocjitsu), and the mirage Rust daemon (rocjitsu/src/daemon.rs), plus
    the DBG_TRAP embedded-pointer marshalling for the runtime-info and
    device-snapshot buffers.
  • UniqueFd — a small move-only RAII fd owner. In daemon mode the session owns
    the transferred descriptor and releases it deterministically on DISABLE or
    process teardown (no manual close, no leak on abnormal teardown). Local mode
    borrows the debugger's own fd and never closes it.
  • The daemon header read requires the full RPC header, dropping a truncated
    header (and reclaiming any fd that arrived with it) rather than acting on a
    partial read.

Driver logging

Adds a dedicated driver logger group and routes the DBG_TRAP handler's
diagnostics through it.

Notes

  • Cross-process attach is intentionally closed by the by-pid gate until feat(rocjitsu): authorize DBG_TRAP by real ptrace #8364:
    debugger_pid is only set by a successful ENABLE, so a first cross-process
    ENABLE can never pass the check. Self-debug bypasses this, so local mode —
    the only supported path today — is unaffected.
  • DBG_TRAP self-debug caches the caller pid at open(), so it requires a
    post-fork re-open. The interposer's reset_after_fork() drops the driver and
    forces a re-open, so this is satisfied in practice; documented on the handler.
  • The kfd_ioctl.h doc block lists EACCES for a HW-op before runtime-enable,
    but the kernel code path returns EPERM; this follows the code (what
    rocm-dbgapi observes).
  • Authorization is by-pid only; the real ptrace check is future work.
  • kfd_process.h now includes <sys/types.h> for pid_t (previously compiled
    only transitively).

JIRA ID

Resolves #8281.

Test Plan

18 DBG_TRAP tests (rocjitsu_tests):

  • Validation ladder (local self-debug): DbgTrapUnknownPidReturnsESRCH,
    DbgTrapOpBeforeEnableReturnsEINVAL, DbgTrapBareDisableReturnsEINVAL,
    DbgTrapEnablePopulatesRuntimeInfoThenDisable,
    DbgTrapDoubleEnableReturnsEALREADY, DbgTrapEnableBadFdReturnsEBADF,
    DbgTrapHwOpWithoutRuntimeReturnsEPERM, DbgTrapWatchBadGpuReturnsENODEV,
    DbgTrapAdmittedOpNotYetImplemented (recognized op → ENOSYS).
  • Concurrency: DbgTrapConcurrentEnableDisableIsRaceFree hammers
    ENABLE/DISABLE on one session from 8 threads × 250 iterations to exercise
    debug_mutex_ under ThreadSanitizer; every call returns a well-defined code
    (0/EALREADY/EINVAL), never a race or torn state.
  • Runtime-info marshalling / fd ownership:
    DbgTrapLocalDisableLeavesDebuggerFdOpen,
    DbgTrapEnableUndersizedRuntimeInfoTruncates,
    DbgTrapEnableOversizedRuntimeInfoPreservesTailInDaemonMode.
  • Daemon SCM_RIGHTS notifier transfer (DbgTrapDaemonTest, real
    rj_vm_execute_as path):
    EnableAdoptsTransferredNotifierFd,
    DisableClosesAdoptedNotifierFd, EnableWithoutTransferredFdReturnsEbadf,
    EnableSendsNotifierFdOverScmRights, ForeignClientCannotDriveAnothersSession.

Test Result

ctest -R "DbgTrap" → 18/18 pass. Clean build.

Submission Checklist

amd-arosa and others added 6 commits July 10, 2026 02:59
…cher

## Motivation

rocm-dbgapi drives every debugger action through the AMDKFD_IOC_DBG_TRAP ioctl.
rocjitsu's KFD emulation dropped that ioctl into the silent `default: return 0`
arm, so a debugger saw every operation "succeed" with no effect. Routing it into
a real per-process dispatcher that enforces the driver's validation contract is
the foundation the rest of the ROCgdb bring-up builds on: each later operation
fills in one arm against a correct error model.

## Technical Details

### Background

The kernel's `kfd_ioctl_set_debug_trap()` (amd/amdkfd/kfd_chardev.c,
amdgpu-6.16.13) runs a fixed validation ladder before its op switch: resolve the
target process by pid, require an attached debugger, require an enabled session,
require runtime-enable for the hardware-op subset, and validate gpu_id for the
address-watch ops. rocm-dbgapi depends on those exact error codes during attach.

### Changes

- Add `KfdProcess::DebugSession` (enabled, runtime_state, exception_enable_mask,
  dbg_fd, debugger_pid), mirroring the debug fields the kernel keeps on
  `kfd_process`.
- Implement `SimulatedDriver::debug_trap_ioctl` with the pre-switch ladder:
  unknown pid -> ESRCH; non-DISABLE cross-process op without a debugger -> EPERM;
  non-ENABLE op without an enabled session -> EINVAL; a DBG_HW_OP (which the real
  driver defines to include SET_FLAGS) before runtime-enable -> EPERM; an
  address-watch op with a bad gpu_id -> ENODEV.
- ENABLE allocates the session, records dbg_fd/exception mask, snapshots the
  runtime state, and copies `kfd_runtime_info` (r_debug, runtime_state,
  ttmp_setup) back to the debugger; DISABLE tears it down.
- Record the local process's client pid (getpid()) in open() so a self-debug
  target resolves, and add find_process_by_client_pid().

### Limits

- The twelve remaining operations return -ENOSYS until later PRs wire them up.
- Authorization here is by-pid only; the real ptrace cross-process check and the
  daemon transport for the embedded output pointers come in the next PR.

## Test Plan

- Add DbgTrap* tests in kfd_ioctl_test.cpp that exercise the ladder through the
  driver harness: unknown pid -> ESRCH; op before enable -> EINVAL; ENABLE
  populates kfd_runtime_info and DISABLE succeeds; double ENABLE -> EINVAL; a
  HW-op (SET_FLAGS) before runtime-enable -> EPERM; address watch with a bad gpu
  -> ENODEV; an admitted-but-unimplemented op (QUERY_DEBUG_EVENT) -> ENOSYS.
- Build rocjitsu_tests + the daemon CLI and run the full gtest suite.

## Test Result

- 7 new DbgTrap tests pass.
- Full suite: 1146/1146 pass (1139 prior + 7 new); no regressions.
…g partial copy), then reports back the full struct size.
@therock-pr-bot

therock-pr-bot Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
🌿 Branch Name ✅ Pass —
📝 PR Title/Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

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

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few comments

amd-arosa and others added 3 commits July 10, 2026 07:58
… -EBADF for an unusable descriptor, matching kfd_dbg_trap_enable(). The current handler accepts every value; four new tests even enable successfully with KFD_INVALID_FD. Please use a real eventfd in the success tests and add an invalid-fd case so the stored notification target cannot be unusable from the start.
@amd-arosa
amd-arosa requested a review from kuhar July 10, 2026 15:43

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few comments

@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.

Additional review comments from an agent:

  • Cross-process ENABLE is structurally impossible, not just "deferred" —
    simulated_kfd.cpp:1744-1746. The kernel's cross-process guard is a live ptrace check that admits the
    first cross-process ENABLE (the ptrace relationship predates any KFD session). The PR replaces it
    with sess.debugger_pid != caller.client_pid(), but debugger_pid is only set by a successful ENABLE
    (line 1796) — so on a first attach debugger_pid==0, the check returns EPERM, and it can never
    succeed. Self-debug bypasses this, so local mode (the only supported path now) is unaffected. But
    the PR comment "authorization is by-pid only" understates that cross-process is fully closed. Fix:
    exempt ENABLE from the by-pid check, or clearly document that cross-process attach is intentionally
    closed until #8364 — otherwise a future author will assume the skeleton partially works.
  • runtime_mutex_ dropped and reacquired within one ENABLE — simulated_kfd.cpp:1753-1756 /
    1806-1810. runtime_enabled is read in one critical section, then r_debug/mode_mask in a second, with
    runtime_mutex_ released in between (only debug_mutex_ held). A concurrent RUNTIME_ENABLE/DISABLE
    can change state between them, so the marshaled runtime_state can disagree with r_debug/ttmp_setup.
    Benign today but latent. Fix: hold runtime_mutex_ once across the whole ENABLE marshal (ordering is
    already debug_mutex_ → runtime_mutex_, so widening is safe).
  • Test coverage gaps — the 7 tests assert exact errnos per rung and check marshaled values (good),
    but nothing covers: self-vs-other pid resolution (ESRCH vs success), two-thread ENABLE/DISABLE race
    (the entire point of debug_mutex_), disable-without-enable → EINVAL, or fork staleness. Add at
    minimum the bare-DISABLE→EINVAL test and a threaded enable/disable smoke test under TSan.
  • fork() leaves client_pid_ stale in the child — cached at open() via getpid(); a self-debug
    DBG_TRAP on an inherited fd would ESRCH. Masked today because the interposer's reset_after_fork()
    nulls the driver so the child re-opens and re-caches. Optionally assert/document that DBG_TRAP
    requires a post-fork re-open.
  • pid_t used in kfd_process.h without #include <sys/types.h> — compiles only transitively;
    pre-existing, not introduced here.
  • The debugger_pid vs client_pid() comparison reads like "is the caller the debugger" — a one-line
    comment noting it only admits re-entrant ops from an already-attached debugger would prevent
    confusion (ties to MINOR #1).

pid_t is used throughout kfd_process.h (client_pid_, debugger_pid,
accessors) but was only available transitively via <unistd.h>. Include
its canonical header directly so the header is self-contained.
The non-self authorization check compares sess.debugger_pid against the
caller pid, which reads like a generic 'is the caller the debugger' test.
In practice it only re-admits re-entrant ops from an already-attached
debugger, and because a fresh session has debugger_pid == 0 it can never
admit a *first* cross-process ENABLE (returns EPERM). Document that this
structurally closes cross-process attach until it is implemented (#8364),
so a future author does not assume the skeleton partially works. No
behavior change; self-debug (local mode) is unaffected.
The ENABLE marshal read runtime_state_.enabled in one runtime_mutex_
critical section (for sess.runtime_state) and r_debug/mode_mask in a
second, releasing runtime_mutex_ in between. A concurrent RUNTIME_ENABLE
or _DISABLE could change state between the two reads, so the marshaled
runtime_state could disagree with the r_debug/ttmp_setup reported to the
debugger. Read all three fields under a single runtime_mutex_ acquisition
so the returned kfd_runtime_info is internally consistent. Lock order
(debug_mutex_ -> runtime_mutex_) is unchanged.
client_pid_ is cached from getpid() at open() time and is the identity
DBG_TRAP uses to resolve a self-debug target. A fork() child inherits the
cache stale (its getpid() differs), which would make a self-debug DBG_TRAP
on an inherited fd resolve to no process (ESRCH). This is masked because
the interposer's reset_after_fork() drops the driver, forcing the child to
re-open() and re-cache before any ioctl. Document the invariant at the
caching site so the dependency is explicit.
…able

Two gaps in the DBG_TRAP dispatch tests:
- DbgTrapBareDisableReturnsEINVAL: a DISABLE with no active session must
  return EINVAL. The by-pid gate exempts DISABLE from the cross-process
  authorization check, but the session-enabled requirement still applies,
  so this pins that a bare DISABLE is rejected rather than silently
  succeeding.
- DbgTrapConcurrentEnableDisableIsRaceFree: hammers ENABLE/DISABLE on one
  self-debug session from 8 threads to exercise debug_mutex_ (and the
  ENABLE runtime-state snapshot) under ThreadSanitizer. Losing racers see
  well-defined EALREADY/EINVAL and never a crash, torn state, or bogus
  errno.
…AP ops

debug_trap_ioctl()'s op switch handled only ENABLE/DISABLE and dropped
every other op into 'default: return -EINVAL'. Recognized ops that are
not yet wired up (QUERY_DEBUG_EVENT, SET_EXCEPTIONS_ENABLED, the DBG_HW_OPs,
SET_FLAGS, the query/snapshot ops) are valid requests the emulator simply
has not implemented, so EINVAL (invalid argument) is misleading. Return
-ENOSYS for them so a debugger can distinguish a stubbed-but-valid op from
a genuinely unknown one, which still yields EINVAL. This matches the
documented contract ('remaining ops return -ENOSYS until later changes
wire them up') and fixes DbgTrapAdmittedOpNotYetImplemented, which asserted
it. Each case graduates out of the ENOSYS group as its handler lands.
@amd-arosa
amd-arosa requested review from atgutier and kuhar July 10, 2026 21:22

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few comments

Comment thread emulation/mirage/rocjitsu/src/daemon.rs Resolved

@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.

The headline: the PR's framing overstates what landed

The expanded PR appears to wire up cross-process attach, but it does not. What actually landed is
(a) the SCM_RIGHTS transport for the debugger notifier fd, and (b) self-debug routed through the
daemon dispatch path. The cross-process authorization gate is unchanged in substance — the first
cross-process ENABLE is still structurally rejected with -EPERM. So this is a solid incremental
step, but the description implying cross-process attach now works is inaccurate. That mismatch
drives the top finding.

Prior findings — status

Prior finding: #2 runtime_mutex_ torn view in ENABLE
Status: ✅ Resolved — marshal now snapshots enabled/r_debug/mode_mask under one critical section
────────────────────────────────────────
Prior finding: #3 default: -ENOSYS vs kernel EINVAL
Status: ✅ Resolved — out-of-range ops now -EINVAL; -ENOSYS reserved for
recognized-but-unimplemented
────────────────────────────────────────
Prior finding: #4 test gaps
Status: 🟡 Partial — now covers bare-disable, 8-thread concurrency, transport round-trip (real
socketpair + sentinel through the transferred fd), foreign-client EPERM, rinfo over/undersize.
Still missing: a positive cross-process attach test (impossible today), daemon-mode concurrent
ENABLE/DISABLE with fd adoption, fork staleness
────────────────────────────────────────
Prior finding: #1 cross-process ENABLE impossible
Status: ⚠️ Not resolved (by design) — still EPERMs first attach; correctly documented in-code as
deferred to #8364, but the PR description doesn't say so
────────────────────────────────────────
Prior finding: #5 fork client_pid_ staleness
Status: ⚠️ Not resolved (documented, latent) — masked by interposer re-open

BLOCKER

Stale ABI-pin test will fail cargo test — mirage/rocjitsu_sys/src/lib.rs:489. The PR adds in_handle:
RjHandle to RjVmCmd, growing it 32→40 bytes (field-for-field matching the new rj_handle_t in_handle
in rj_vm_cmd_t), but the ABI-pinning assertion still says assert_eq!(size_of::(), 32).
This is exactly the test meant to catch ABI drift; it now fails. Fix: bump to 40 and update the
stale layout comment. (The FFI itself is correct — both sides are 40 bytes and identical.)

MAJOR

  1. Cross-process authorization substitute is absent — simulated_kfd.cpp:1759. The current gate only
    re-admits an already-attached debugger; there's no positive authorization for a first attach. The
    code is safe today only because the debugger_pid == 0 structural block makes first attach
    unreachable — a fragile property. When #8364 flips this on, the only remaining check is "does a
    KfdProcess with client_pid == args->pid exist" (find_process_by_client_pid scans the whole table
    with no relationship check), so any daemon client could attach as debugger to any other client's
    process. The kernel gates this on ptrace parentage. Before graduating cross-process, land an
    explicit substitute (SO_PEERCRED-based ptrace-permitted check on the daemon side) and document it as
    the security model — or if the daemon fully trusts its clients, state that explicitly.
  2. recvmsg missing MSG_CMSG_CLOEXEC — mirage/rocjitsu/src/daemon.rs:591. The received notifier fd
    lacks FD_CLOEXEC, leaking the debugger's pipe across any fork/exec in the daemon. Diverges from the
    C rpc_recv_msg (which uses MSG_CMSG_CLOEXEC) and from the CLOEXEC convention everywhere else in this
    file. Fix: pass libc::MSG_CMSG_CLOEXEC.

MINOR

  • caller.client_pid() read without synchronization — simulated_kfd.cpp:1735/1760/1814; client_pid_
    is non-atomic, written under process_mutex_, read lock-free in the dispatcher. No real race today
    (set-once before discoverable) but TSan-flaggable. Make it std::atomic<pid_t> or document the
    invariant.
  • Daemon fcntl(dbg_fd, F_GETFD) EBADF path is implicit across 3 files — rj_vm.cpp:256 + dispatcher
    :1806. The missing-transfer→EBADF guarantee relies on the client sending exactly KFD_INVALID_FD; a
    bogus small fd number with no SCM_RIGHTS transfer would validate an unrelated daemon fd and wrongly
    adopt it. Don't trust the payload dbg_fd in daemon mode — key EBADF on the absence of an adopted
    in_handle.
  • Duplicated DBG_TRAP embedded-pointer/inline-size ladder — the op→field mapping (ENABLE→rinfo_ptr,
    GET_DEVICE_SNAPSHOT→snapshot_buf_ptr) is spelled out ~4 times across remote_driver.cpp and
    rj_vm.cpp; they must stay consistent or the round-trip silently corrupts. Factor into one shared
    helper.
  • Single-fd SCM_RIGHTS extraction without overflow-close — daemon.rs:604; keeps only the last fd per
    cmsg and doesn't close surplus (the C ref counts and closes extras). Limited by the one-fd-sized
    control buffer (MSG_CTRUNC closes the rest), so unlikely to leak in the expected case, but not
    robust.
  • No MSG_CTRUNC handling/logging — daemon.rs:591; a truncated ancillary payload silently drops the
    notifier fd (ENABLE loses it) with no trace. At least tracing::warn!.

NIT

No test asserts -EINVAL for a genuinely out-of-range op (all negative op tests use recognized ops);
DebugSession doxygen in kfd_process.h cites AMD-internal kernel field names + a driver source path
rather than the public UAPI contract (confidentiality-hygiene); main.cpp fd-ownership "safety net"
is correct but the one-owner invariant is spread across 3 points — a one-line comment would prevent
a double-close regression; close() return ignored on fd reclaim (matches surrounding style).

The by-pid authorization gate in debug_trap_ioctl() exempted DISABLE, so
any client could tear down another process's debug session — and in daemon
mode close the SCM_RIGHTS notifier fd the session owns — purely by naming
the target's pid (untrusted user-mode input). Only self-debug should bypass
the gate; a DISABLE from a caller that is not the attached debugger now
returns -EPERM, matching every other non-self op.

Add DbgTrapCrossProcessDisableByNonOwnerReturnsEPERM: two daemon-mode
clients share one driver, the owner self-enables a session, and a second
client's DISABLE is refused while the session stays live. Mutation-verified
against the reintroduced exemption.
In daemon mode the DBG_TRAP ENABLE notifier fd only ever reaches the daemon
out-of-band via SCM_RIGHTS. execute_impl() substituted the transferred fd
into the payload only when one arrived; otherwise the client-supplied dbg_fd
integer survived into debug_trap_ioctl(), where fcntl() validated it against
the *daemon's* fd table. A client that omits the ancillary fd could thus name
any live daemon-owned descriptor and have the session adopt it (a confused-
deputy fd substitution).

Overwrite enable.dbg_fd unconditionally for daemon-mode ENABLE: the
transferred fd when present, otherwise KFD_INVALID_FD so the handler rejects
it with -EBADF.

Add EnableWithClientChosenFdNumberIsNotTrustedInDaemonNamespace: the client
names a live daemon eventfd with no SCM_RIGHTS transfer; ENABLE is refused and
the daemon's descriptor is neither adopted nor closed. Mutation-verified
against the removed scrub.
The ENABLE handler validated dbg_fd with fcntl(F_GETFD), which only proves the
descriptor is open — it accepts any fd, including a read-only one that can
never serve as the notification target the driver writes to wake the debugger.
This matters most for the SCM_RIGHTS fd a daemon client transfers: existence
alone is not enough to trust it.

Validate the access mode with fcntl(F_GETFL) and reject a non-writable fd with
-EBADF, matching the existing "unusable descriptor" contract.

Add DbgTrapEnableReadOnlyFdReturnsEBADF: a live O_RDONLY fd is refused and
leaves the session disabled. Mutation-verified against the F_GETFD check.

@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.

Mostly looks good. Other than my API/ABI ask here is an issue highlighted by an agent:

Fix A — daemon DBG_TRAP ENABLE must not adopt an untransferred fd

File: emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/rj_vm.cpp
Function: execute_impl

The bug: when a client sends DBG_TRAP ENABLE in daemon mode with no SCM_RIGHTS fd, the substitution
block is skipped, so dbg->enable.dbg_fd keeps the raw client-supplied number. The dispatcher then
runs fcntl(dbg_fd, F_GETFD) on that number against the daemon's fd table — if it happens to be a
live daemon fd (socket, memfd), the check passes and the daemon adopts and later closes its own fd.

Find this block:

bool adopting_notifier = false;
    if (driver->daemon_mode() && cmd->cmd == AMDKFD_IOC_DBG_TRAP && in_handle && *in_handle >= 0) {
      auto *dbg = static_cast<kfd_ioctl_dbg_trap_args *>(cmd->buf);
      if (dbg->op == KFD_IOC_DBG_TRAP_ENABLE) {
        dbg->enable.dbg_fd = static_cast<uint32_t>(*in_handle);
        adopting_notifier = true;
      }
    }

Replace with:

    bool adopting_notifier = false;
    if (driver->daemon_mode() && cmd->cmd == AMDKFD_IOC_DBG_TRAP) {
      auto *dbg = static_cast<kfd_ioctl_dbg_trap_args *>(cmd->buf);
      if (dbg->op == KFD_IOC_DBG_TRAP_ENABLE) {
        if (in_handle && *in_handle >= 0) {
          // A notifier fd was transferred over SCM_RIGHTS (already in the daemon's
          // fd space); substitute it for the client-side number in the payload.
          dbg->enable.dbg_fd = static_cast<uint32_t>(*in_handle);
          adopting_notifier = true;
        } else {
          // No fd was transferred. The client-supplied dbg_fd is a number in the
          // client's fd table and must never be validated/adopted against the
          // daemon's own fds. Force it invalid so the dispatcher's
          // fcntl(dbg_fd, F_GETFD) check returns -EBADF.
          dbg->enable.dbg_fd = static_cast<uint32_t>(-1);
        }
      }
    }

Why this is complete: the dispatcher (debug_trap_ioctl in simulated_kfd.cpp, case
KFD_IOC_DBG_TRAP_ENABLE) already does if (fcntl(dbg_fd, F_GETFD) == -1) return -EBADF;. Forcing
dbg_fd = -1 when nothing was transferred makes that existing check fail cleanly — no dispatcher
change needed. The whole block is guarded by driver->daemon_mode(), and local mode never routes
through execute_impl, so local debugging is untouched.
Regression test (add to tests/kfd_ioctl_test.cpp): daemon-mode rj_vm_execute_as_ex with a DBG_TRAP
ENABLE whose args->enable.dbg_fd is a valid daemon fd number (e.g. dup() of any open fd) and
in_handle == nullptr; assert the result is -EBADF and that the dup'd fd is still open afterward.
(The existing EnableWithoutTransferredFdReturnsEbadf only passes dbg_fd = -1, which fails fcntl by
coincidence rather than by this guard.)

@amd-arosa

Copy link
Copy Markdown
Contributor Author

Mostly looks good. Other than my API/ABI ask here is an issue highlighted by an agent:

Fix A — daemon DBG_TRAP ENABLE must not adopt an untransferred fd

File: emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/rj_vm.cpp Function: execute_impl

The bug: when a client sends DBG_TRAP ENABLE in daemon mode with no SCM_RIGHTS fd, the substitution block is skipped, so dbg->enable.dbg_fd keeps the raw client-supplied number. The dispatcher then runs fcntl(dbg_fd, F_GETFD) on that number against the daemon's fd table — if it happens to be a live daemon fd (socket, memfd), the check passes and the daemon adopts and later closes its own fd.

Find this block:

bool adopting_notifier = false;
    if (driver->daemon_mode() && cmd->cmd == AMDKFD_IOC_DBG_TRAP && in_handle && *in_handle >= 0) {
      auto *dbg = static_cast<kfd_ioctl_dbg_trap_args *>(cmd->buf);
      if (dbg->op == KFD_IOC_DBG_TRAP_ENABLE) {
        dbg->enable.dbg_fd = static_cast<uint32_t>(*in_handle);
        adopting_notifier = true;
      }
    }

Replace with:

    bool adopting_notifier = false;
    if (driver->daemon_mode() && cmd->cmd == AMDKFD_IOC_DBG_TRAP) {
      auto *dbg = static_cast<kfd_ioctl_dbg_trap_args *>(cmd->buf);
      if (dbg->op == KFD_IOC_DBG_TRAP_ENABLE) {
        if (in_handle && *in_handle >= 0) {
          // A notifier fd was transferred over SCM_RIGHTS (already in the daemon's
          // fd space); substitute it for the client-side number in the payload.
          dbg->enable.dbg_fd = static_cast<uint32_t>(*in_handle);
          adopting_notifier = true;
        } else {
          // No fd was transferred. The client-supplied dbg_fd is a number in the
          // client's fd table and must never be validated/adopted against the
          // daemon's own fds. Force it invalid so the dispatcher's
          // fcntl(dbg_fd, F_GETFD) check returns -EBADF.
          dbg->enable.dbg_fd = static_cast<uint32_t>(-1);
        }
      }
    }

Why this is complete: the dispatcher (debug_trap_ioctl in simulated_kfd.cpp, case KFD_IOC_DBG_TRAP_ENABLE) already does if (fcntl(dbg_fd, F_GETFD) == -1) return -EBADF;. Forcing dbg_fd = -1 when nothing was transferred makes that existing check fail cleanly — no dispatcher change needed. The whole block is guarded by driver->daemon_mode(), and local mode never routes through execute_impl, so local debugging is untouched. Regression test (add to tests/kfd_ioctl_test.cpp): daemon-mode rj_vm_execute_as_ex with a DBG_TRAP ENABLE whose args->enable.dbg_fd is a valid daemon fd number (e.g. dup() of any open fd) and in_handle == nullptr; assert the result is -EBADF and that the dup'd fd is still open afterward. (The existing EnableWithoutTransferredFdReturnsEbadf only passes dbg_fd = -1, which fails fcntl by coincidence rather than by this guard.)

that was already fixed in b5e8caf

Commit 442c1ef moved the client-provided input fd out of rj_vm_cmd_t and
into an out-of-band rj_vm_execute_as_ex(vm, pid, cmd, rj_handle_t *in_handle)
so the descriptor ABI could stay frozen for older runtime-loaded callers.
While we're still in beta that back-compat isn't worth a second "_ex" entry
point whose only purpose was to avoid growing the struct: in_handle already
uses -1 to mean "no fd", so a separate function bought nothing but a sideband.

Put the fd back in rj_vm_cmd_t::in_handle ([in,out], -1 when none) and delete
rj_vm_execute_as_ex. The VM still substitutes the transferred fd into DBG_TRAP
ENABLE, clears it to -1 on adoption, and scrubs an untrusted client dbg_fd to
KFD_INVALID_FD in daemon mode.

rocjitsu_sys mirrors the field, drops the optional vm_execute_as_ex symbol and
its fallback, and pins sizeof(RjVmCmd) at 40 to match the C struct.

@amd-arosa amd-arosa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lgtm

@amd-arosa
amd-arosa requested a review from atgutier July 14, 2026 19:36
…t copy the response tail into the caller's runtime-info buffer
@amd-arosa
amd-arosa merged commit 3544def into develop Jul 15, 2026
24 checks passed
@amd-arosa
amd-arosa deleted the users/arosa/rj/rocgdb/03 branch July 15, 2026 18:07
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.

feat(rocjitsu): route AMDKFD_IOC_DBG_TRAP into a debug-session dispatcher

3 participants