From: Yongqiang Sun <[email protected]> [ Upstream commit 01112e241e37f9ac98b6f418d93ce2e0b87b7ee0 ]
If kfd_dbg_trap_enable() fails while copying runtime_info to userspace, it had already activated the trap, set debug_trap_enabled, taken an extra process reference, and opened the debug event file. Return -EFAULT without unwinding that state, leaving inconsistent trap state and a refcount imbalance that could break later DISABLE/ENABLE. On copy_to_user failure, deactivate the trap and undo the rest of the enable setup before returning. Signed-off-by: Yongqiang Sun <[email protected]> Acked-by: Alex Deucher <[email protected]> Signed-off-by: Alex Deucher <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: ## Phase 1: Commit Message Forensics ### Step 1.1: Parse the Subject Line **Record:** `[drm/amdkfd]` `[Unwind]` — On `copy_to_user` failure in `kfd_dbg_trap_enable()`, undo partial debug-trap enable setup that was left behind. ### Step 1.2: Parse All Commit Message Tags **Record:** - **Fixes:** — none - **Reported-by:** — none - **Tested-by:** — none - **Reviewed-by:** — none - **Acked-by:** Alex Deucher `<[email protected]>` (AMD DRM maintainer) - **Link:** — none - **Cc: [email protected]:** — none (expected for manual review) - **Signed-off-by:** Yongqiang Sun `<[email protected]>` (author); Alex Deucher (committer) Notable: maintainer **Acked-by** only; no syzbot or user reports. ### Step 1.3: Analyze Commit Body **Record:** - **Bug:** If `copy_to_user()` fails after `kfd_dbg_trap_enable()` has activated the HW trap, set `debug_trap_enabled`, taken an extra `kref`, and opened `dbg_ev_file`, the function returned `-EFAULT` without undoing that state. - **Symptom:** Inconsistent trap state; refcount imbalance; later DISABLE/ENABLE can misbehave. - **Root cause:** Error path only called `kfd_dbg_trap_deactivate()` but did not mirror the rest of `kfd_dbg_trap_disable()` cleanup. - **Version info:** None in the message. ### Step 1.4: Detect Hidden Bug Fixes **Record:** Not disguised — explicitly an error-path unwind / resource- leak / state-machine fix. --- ## Phase 2: Diff Analysis ### Step 2.1: Inventory Changes **Record:** - **Files:** `drivers/gpu/drm/amd/amdkfd/kfd_debug.c` (+6 / −0) - **Function:** `kfd_dbg_trap_enable()` - **Scope:** Single-file, surgical error-path fix ### Step 2.2: Code Flow Change **Record:** On `copy_to_user()` failure in `kfd_dbg_trap_enable()`: - **Before:** `kfd_dbg_trap_deactivate(target, false, 0); r = -EFAULT;` — HW trap deactivated, but `dbg_ev_file`, `debug_trap_enabled`, extra `kref`, and `debugged_process_count` left as if enable succeeded. - **After:** Same deactivate, then `fput()` + NULL `dbg_ev_file`, `atomic_dec(debugged_process_count)`, `debug_trap_enabled = false`, `kfd_unref_process(target)`, then `-EFAULT`. ### Step 2.3: Bug Mechanism **Record:** - **Category:** Error-path resource leak + inconsistent state (reference counting + flag/file leak) - **Mechanism:** `kref_get()`, `fget()`, `debug_trap_enabled = true`, and `atomic_inc()` run before `copy_to_user()`. Failure left software state enabled while userspace received `-EFAULT`. ### Step 2.4: Fix Quality **Record:** Mirrors the corresponding cleanup in `kfd_dbg_trap_disable()` (lines 682–692). Minimal, obviously correct. Low regression risk — only runs on an already-failing path. Does not add `cancel_work_sync()` or clear `debugger_process`; same gap as the pre- existing partial `kfd_dbg_trap_deactivate()` unwind. --- ## Phase 3: Git History Investigation ### Step 3.1: Blame Changed Lines **Record:** Buggy `copy_to_user` error path from Jonathan Kim, 2022-04-05 (`218895820e6fcc`). `kfd_dbg_trap_enable()` with refcount/file/flag setup from `0ab2d7532b05a` (2023-06-09, “prepare per- process debug enable and disable”). Bug present since ~v6.5+; definitely in this tree. ### Step 3.2: Follow Fixes Tag **Record:** N/A — no `Fixes:` tag. ### Step 3.3: File History **Record:** Recent `kfd_debug.c` changes are other debugger fixes (watch bounds, MES debug, PASID). Fix commit `a50676d5a72a2` is on `all-next` but **not** on `stable/linux-6.18.y`. Standalone one-commit fix. ### Step 3.4: Author's Other Commits **Record:** Yongqiang Sun has limited amdkfd history in this tree (e.g. CWSR overflow fix). Alex Deucher is DRM/AMD maintainer and committed the fix. ### Step 3.5: Prerequisites **Record:** No series dependencies. `kfd_dbg_trap_enable()`, `kfd_dbg_trap_deactivate()`, and `kfd_unref_process()` all exist in this tree. Applies standalone. --- ## Phase 4: Mailing List and External Research ### Step 4.1: Original Patch Discussion **Record:** `b4 dig -c a50676d5a72a2` → https://patch.msgid.link/[email protected]. Single revision (no `-a` series). Alex Deucher replied with **Acked-by** in-thread. No NAKs found in mbox. No explicit `Cc: stable` nomination in thread. ### Step 4.2: Reviewers **Record:** `b4 dig -w` — CC'd to `[email protected]`. Alex Deucher reviewed and acked. ### Step 4.3: Bug Report **Record:** N/A — no `Reported-by` or `Link:` tags. Code-review / error- path analysis fix. ### Step 4.4: Related Patches **Record:** Standalone; not part of a multi-patch series. ### Step 4.5: Stable Mailing List **Record:** Not searched separately; no stable nomination found in patch thread. --- ## Phase 5: Code Semantic Analysis ### Step 5.1: Key Functions **Record:** `kfd_dbg_trap_enable()` (modified); related: `kfd_dbg_trap_deactivate()`, `kfd_dbg_trap_disable()`, `kfd_unref_process()`. ### Step 5.2: Callers **Record:** `kfd_dbg_trap_enable()` called from `kfd_chardev.c` on `KFD_IOC_DBG_TRAP_ENABLE` (ioctl path ~line 3029). Reached by ROCm/KFD GPU debugger tooling via `/dev/kfd`. ### Step 5.3: Callees **Record:** `fget`, `kfd_dbg_trap_activate`, `kref_get`, `atomic_inc`, `copy_to_user`, `kfd_dbg_trap_deactivate`, `fput`, `kfd_unref_process`. ### Step 5.4: Call Chain / Reachability **Record:** Userspace debugger → `KFD_IOC_DBG_TRAP` ioctl → `kfd_dbg_trap_enable()`. `copy_to_user()` fails on invalid/unmapped userspace buffers (buggy debugger, bad pointer, page fault under memory pressure). Not a general unprivileged attack surface, but reachable by authorized KFD clients. ### Step 5.5: Similar Patterns **Record:** `kfd_dbg_trap_disable()` already performs the full cleanup the fix adds. The error path was an incomplete subset of disable logic. --- ## Phase 6: Cross-Reference Against Local Tree ### Step 6.1: Does Buggy Code Exist? **Record:** **Yes.** Tree is **Linux 6.18.44** (`git describe HEAD` → `v6.18.44`, `VERSION=6 PATCHLEVEL=18 SUBLEVEL=44`). Buggy code at `kfd_debug.c:817-819`: ```817:819:drivers/gpu/drm/amd/amdkfd/kfd_debug.c if (copy_to_user(runtime_info, (void *)&target->runtime_info, copy_size)) { kfd_dbg_trap_deactivate(target, false, 0); r = -EFAULT; ``` ### Step 6.2: Backport Complications **Record:** **Clean apply expected.** Fix commit diff matches current file structure; only 6 lines in one hunk. ### Step 6.3: Related Fixes Already Present? **Record:** **No.** `git log stable/linux-6.18.y --grep="Unwind debug trap"` returns nothing. Fix exists on `all-next` (`a50676d5a72a2`) but not in this stable checkout. --- ## Phase 7: Subsystem and Maintainer Context ### Step 7.1: Subsystem Criticality **Record:** `drivers/gpu/drm/amd/amdkfd` — **IMPORTANT** (AMD GPU compute/ROCm KFD driver). Debug-trap path only; not core kernel, but affects production debugger workflows. ### Step 7.2: Subsystem Activity **Record:** Actively maintained — recent stable-relevant amdkfd fixes (debugger auth, overflows, CRIU, NULL deref). --- ## Phase 8: Impact and Risk Assessment ### Step 8.1: Who Is Affected **Record:** Users of AMD KFD GPU debugging (ROCm debugger, `KFD_IOC_DBG_TRAP_ENABLE`). Requires `CONFIG_HSA_AMD` / amdkfd. Not universal, but real for that population. ### Step 8.2: Trigger Conditions **Record:** `copy_to_user()` failure during debug-trap enable — uncommon but valid (bad userspace buffer). **Likelihood:** low in normal use, easy to hit with a buggy debugger or invalid pointer. **Privilege:** KFD device access required. ### Step 8.3: Failure Mode Severity **Record:** - Extra `kref` leak on `kfd_process` → process object retained longer than intended - `dbg_ev_file` leak → kernel `struct file` refcount leak - `debug_trap_enabled` stuck `true` while ioctl returned error → subsequent enable returns `-EALREADY` (verified: line 779-780) - `debugged_process_count` skew → wrong debugger session accounting - **Severity:** **MEDIUM-HIGH** for debugger users (broken retry/disable-enable cycle, resource leaks); **LOW** for non-debug workloads ### Step 8.4: Risk-Benefit **Record:** - **Benefit:** Restores correct error recovery; prevents leaks and stuck debug state - **Risk:** Very low — 6 lines on failure-only path, mirrors existing disable logic - **Ratio:** Favorable for stable --- ## Phase 9: Final Synthesis ### Step 9.1: Evidence Summary **FOR backport:** - Real bug in error path present since 2023 - Resource leaks (`kref`, `struct file`) - Functional breakage: `-EALREADY` on retry after failed enable - Small (6 lines), maintainer-acked - Buggy code confirmed in Linux 6.18.44; fix not yet applied - Clean backport **AGAINST backport:** - No user report or syzbot — code-review discovery - Affects debugger-only path, not general workloads - “Important issue” bar is borderline vs crash/security (no oops/UAF demonstrated) **Unresolved:** Whether `debugger_process` should also be cleared on failure (fix matches partial unwind intent, not full disable). ### Step 9.2: Stable Rules Checklist 1. Obviously correct and tested? **PASS** — mirrors `kfd_dbg_trap_disable()`; maintainer acked; no Tested-by 2. Fixes a real bug? **PASS** — refcount/file leak and stuck `debug_trap_enabled` 3. Important issue? **PASS (MEDIUM)** — resource leaks + broken debug enable/disable state machine 4. Small and contained? **PASS** — 6 lines, one function 5. No new features/APIs? **PASS** 6. Can apply to local tree? **PASS** — code exists; clean apply ### Step 9.3: Exception Categories **Record:** None (not device ID, quirk, DT, build, or docs). Standard error-path bug fix. ### Step 9.4: Decision Rationale For **Linux 6.18.y** specifically: the incomplete unwind in `kfd_dbg_trap_enable()` is present, the fix is absent, and the patch is a minimal error-path correction that prevents reference and file leaks and restores consistent debug-trap state after `-EFAULT`. This matches the kind of driver error-recovery fixes routinely accepted for stable, especially with AMD maintainer acknowledgment. --- ## Verification - **[Phase 1]** Parsed subject, tags, body from provided commit message and `git show a50676d5a72a2` - **[Phase 2]** Read diff and current `kfd_debug.c:772-825`, `kfd_dbg_trap_disable()` at 667-694 - **[Phase 3]** `git blame -L 817,820` → `218895820e6fcc` (2022); `git show 0ab2d7532b05a` → introduced enable setup (2023); `git log stable/linux-6.18.y --grep="Unwind debug trap"` → empty - **[Phase 3]** `git log --oneline -5 -- drivers/gpu/drm/amd/amdkfd/kfd_debug.c` - **[Phase 4]** `b4 dig -c a50676d5a72a2` → lore URL; `b4 dig -m /tmp/kfd_debug_trap.mbx` → Alex Deucher Acked-by in thread; `b4 dig -a` → no revisions - **[Phase 5]** `grep kfd_dbg_trap_enable` → caller in `kfd_chardev.c:3029`; `grep debug_trap_enabled` across amdkfd - **[Phase 6]** `git describe HEAD` → `v6.18.44`; Makefile `6.18.44`; confirmed buggy lines at 817-819 without fix - **[Phase 6]** `git log stable/linux-6.18.y..all-next -- drivers/gpu/drm/amd/amdkfd/kfd_debug.c` → fix commit listed, not in stable - **[Phase 6]** `git branch --contains a50676d5a72a2` → `all-next` and others, not `stable/linux-6.18.y` - **[Phase 7]** `git log --oneline -20 -- drivers/gpu/drm/amd/amdkfd/` → active subsystem - **[Phase 8]** Verified `-EALREADY` guard at line 779-780; verified `kfd_dbg_trap_disable()` cleanup pattern at 682-692 **YES** drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c index 986cb297de8f8..78e1dfdb27147 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c @@ -816,6 +816,12 @@ int kfd_dbg_trap_enable(struct kfd_process *target, uint32_t fd, if (copy_to_user(runtime_info, (void *)&target->runtime_info, copy_size)) { kfd_dbg_trap_deactivate(target, false, 0); + fput(target->dbg_ev_file); + target->dbg_ev_file = NULL; + if (target->debugger_process) + atomic_dec(&target->debugger_process->debugged_process_count); + target->debug_trap_enabled = false; + kfd_unref_process(target); r = -EFAULT; } -- 2.53.0
