From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/amdgpu: fix recursive ww_mutex acquire in amdgpu_devcoredump_format
Date: Tue, 05 May 2026 11:32:40 +1000
Message-ID: <review-patch1-20260429143743.50743-1-mikhail.v.gavrilov@gmail.com>
In-Reply-To: <20260429143743.50743-1-mikhail.v.gavrilov@gmail.com>
References: <20260429143743.50743-1-mikhail.v.gavrilov@gmail.com> <20260429143743.50743-1-mikhail.v.gavrilov@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Positive aspects:**

- The lockdep analysis in the commit message is excellent, clearly documenting the root cause and the real deadlock scenario (always-valid BOs sharing `dma_resv` with the root PD).
- The collect-then-release pattern in `amdgpu_devcoredump_collect_ib_refs()` is correct. `amdgpu_vm_bo_lookup_mapping()` requires the root PD reservation (confirmed by `dma_resv_assert_held()` assertions elsewhere in the tree), and `amdgpu_bo_ref()` is a lockless atomic refcount bump, so the collection phase is safe.
- After releasing the root PD, each `amdgpu_bo_reserve()` is an independent top-level ww_mutex acquire -- no nesting, no lockdep issue.
- `amdgpu_devcoredump_release_ib_refs()` properly unrefs all BOs, fixing the pre-existing leak the commit message describes.

**Bug 1 -- Sizing pass underestimates buffer size (truncation):**

The original code ran the IB content printing loop during the sizing pass:

```c
// Original -- sizing pass prints content (with uninitialized data, harmless for sizing)
for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
    drm_printf(&p, "0x%08x\n", ib_content[j]);
```

The new code guards the content printing with:

```c
if (!sizing_pass && ib_refs && ib_refs[i].bo) {
    for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
        drm_printf(&p, "0x%08x\n", ib_content[j]);
}
```

During the sizing pass, `sizing_pass` is true, so the content lines are **never counted**. But `amdgpu_devcoredump_deferred_work()` at line 546-548 allocates the buffer based on the sizing pass return value:

```c
coredump->formatted_size = amdgpu_devcoredump_format(NULL, ...);
coredump->formatted = kvzalloc(coredump->formatted_size, GFP_KERNEL);
```

The second (real) pass then tries to write IB header + content into a buffer that was only sized for headers. `drm_coredump_printer` respects `iter.remain` so there's no overflow, but IB content at the end of the dump will be **silently truncated** -- which defeats the purpose of the devcoredump.

**Fix:** The sizing pass should still account for the content lines. For example, always print the content loop during sizing (as the original did), or compute the size arithmetically without accessing `ib_content`.

**Bug 2 -- Uninitialized `ib_content` printed on error paths:**

When `amdgpu_bo_reserve()` fails, `ttm_bo_kmap()` fails, or the BO is `NO_CPU_ACCESS` but not in VRAM, the code jumps to `output_ib_content` **without having populated `ib_content`** (allocated via `kvmalloc_array` -- not zeroed). The guard at `output_ib_content`:

```c
if (!sizing_pass && ib_refs && ib_refs[i].bo) {
```

is still true in these cases because the BO was found (it's the *access* that failed), so uninitialized heap data gets formatted as hex into the devcoredump. In the original code, these error paths jumped to `free_ib_content`, skipping the output entirely.

This is both a correctness issue (misleading garbage in the dump) and technically an info leak to `/sys/class/devcoredump/` (root-readable, so low severity).

**Fix:** Track whether `ib_content` was successfully filled (e.g., a `bool ib_valid` flag, or NULL out the `ib_content` pointer on error paths), and only print content when it's valid. Alternatively, use `kvzalloc_array`/add `__GFP_ZERO` so failed reads produce zeroes instead of heap contents.

**Minor observations:**

- The `Cc: stable@vger.kernel.org # 7.1` tag looks incorrect -- this should probably be `# 6.x` since Linux is on 6.x kernels. Verify the version where the Fixes commit `7b15fc2d1f1a` first appeared.
- The behavioral change when VM lookup fails (original: skip loop entirely; new: print IB headers without content) is acceptable but worth noting in the commit message.
- Comments in the main function body (lines 412-426) are a bit verbose for kernel style, but the locking rationale is valuable to preserve.

---
Generated by Claude Code Patch Reviewer