public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO
@ 2026-05-17 13:17 Michael Bommarito
  2026-05-18  6:08 ` Claude review: " Claude Code Review Bot
  2026-05-18  6:08 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Michael Bommarito @ 2026-05-17 13:17 UTC (permalink / raw)
  To: Alex Deucher, Christian Koenig, David Francis, Sumit Semwal,
	David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel,
	linux-media, linaro-mm-sig
  Cc: Ziyi Guo

The AMDGPU_GEM_OP_GET_MAPPING_INFO branch of amdgpu_gem_op_ioctl()
holds three cleanup-tracked resources before calling kvcalloc():
the drm_gem_object reference from drm_gem_object_lookup(), the
drm_exec lock on the looked-up GEM via drm_exec_lock_obj(), and
the drm_exec lock on the per-process VM root page directory via
amdgpu_vm_lock_pd().  All three are released by the out_exec
label that every other error path in this function jumps to.
The kvcalloc() failure path returns -ENOMEM directly, skipping
out_exec and leaking all three.

The leaked per-process VM root PD dma_resv lock is the
load-bearing leak: any subsequent operation on the same VM
(further GEM ops, command-submission, eviction, TTM shrinker
callbacks) blocks on the held lock.  DRM_IOCTL_AMDGPU_GEM_OP is
DRM_AUTH | DRM_RENDER_ALLOW, so this is an unprivileged-local
denial of service against the caller's GPU context, reachable
by any process with /dev/dri/renderD* access.

Route the failure through out_exec so drm_exec_fini() and
drm_gem_object_put() run.

Reproduced on stock 7.0.0-10, Ryzen 7 5700U / Radeon Vega
(Lucienne): the failing ioctl returns -ENOMEM and a second
GET_MAPPING_INFO on the same fd then blocks in
drm_exec_lock_obj() on the leaked dma_resv.  SIGKILL on the
caller does not reap the task; the fd-release path during
process exit goes through amdgpu_gem_object_close() ->
drm_exec_prepare_obj() on the same lock, leaving the task in D
state until the box is rebooted.  The patched kernel was not
rebuilt and re-tested on this hardware; the fix is mechanical.
Tested on a single Lucienne / Vega box only.

Ziyi Guo posted an independent INT_MAX-bound check for
args->num_entries in the same branch [1]; the two patches are
complementary and can land in either order.

Fixes: 4d82724f7f2b ("drm/amdgpu: Add mapping info option for GEM_OP ioctl")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/20260208000255.4073363-1-n7l8m4@u.northwestern.edu/ # [1]
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 9ef80bca4102..8224fb499fdf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -1091,8 +1091,10 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 		 * be retried.
 		 */
 		vm_entries = kvcalloc(args->num_entries, sizeof(*vm_entries), GFP_KERNEL);
-		if (!vm_entries)
-			return -ENOMEM;
+		if (!vm_entries) {
+			r = -ENOMEM;
+			goto out_exec;
+		}
 
 		amdgpu_vm_bo_va_for_each_valid_mapping(bo_va, mapping) {
 			if (num_mappings < args->num_entries) {
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO
  2026-05-17 13:17 [PATCH] drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO Michael Bommarito
@ 2026-05-18  6:08 ` Claude Code Review Bot
  2026-05-18  6:08 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-18  6:08 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO
Author: Michael Bommarito <michael.bommarito@gmail.com>
Patches: 1
Reviewed: 2026-05-18T16:08:13.978576

---

This is a single-patch series fixing a genuine resource leak bug in `amdgpu_gem_op_ioctl()`. The analysis is correct: when `kvcalloc()` fails at line 1093, the original code returned `-ENOMEM` directly, bypassing the `out_exec` cleanup label. This leaked the `drm_exec` lock context (which holds the GEM object's `dma_resv` and the VM root PD's `dma_resv`) and the `drm_gem_object` reference.

The fix is mechanical and obviously correct — it sets `r = -ENOMEM` and jumps to `out_exec`, which calls `drm_exec_fini(&exec)` and `drm_gem_object_put(gobj)`, matching the cleanup pattern used by every other error path in this function.

The commit message is thorough and well-written. It identifies the specific resources leaked, explains the user-visible impact (unprivileged local DoS via stuck D-state task), references the related work by Ziyi Guo on `num_entries` validation, and correctly notes the fix was not runtime-tested on patched kernel. The `Fixes:` and `Cc: stable` tags are appropriate.

**Verdict: This patch looks correct and ready to apply.**

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO
  2026-05-17 13:17 [PATCH] drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO Michael Bommarito
  2026-05-18  6:08 ` Claude review: " Claude Code Review Bot
@ 2026-05-18  6:08 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-18  6:08 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** Looking at the function structure in `amdgpu_gem.c:1002-1141`:

1. At line 1016, `drm_gem_object_lookup()` acquires a reference on `gobj`.
2. At lines 1022-1036, `drm_exec_init()` + `drm_exec_lock_obj()` + `amdgpu_vm_lock_pd()` acquire locks tracked by `exec`.
3. The `out_exec` label at line 1137-1140 properly cleans up both via `drm_exec_fini(&exec)` and `drm_gem_object_put(gobj)`.

The original code at the `kvcalloc` failure:
```c
if (!vm_entries)
    return -ENOMEM;
```
skips both cleanup calls. The fix:
```c
if (!vm_entries) {
    r = -ENOMEM;
    goto out_exec;
}
```
correctly routes through the existing cleanup path.

**Comparison with sibling error paths:** Every other error path in this function either jumps to `out_exec` (lines 1028, 1033, 1055-1056, 1059-1060, 1065-1066) or calls `drm_exec_fini(&exec)` inline before the `break` (lines 1047, 1079, 1119, 1131). The fix is consistent with the `goto out_exec` pattern used for all pre-`break` errors.

**One minor observation (not blocking):** The commit message mentions the patch was `Assisted-by: Claude:claude-opus-4-7`. This tag is not a standard kernel tag — the kernel community hasn't settled on a convention for AI-assisted patches. Some maintainers may request removal or conversion. This is a social/process concern, not a code correctness issue.

**No issues found with the code change itself.** The fix is minimal, correct, and addresses a real unprivileged local DoS.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-05-18  6:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-17 13:17 [PATCH] drm/amdgpu: fix lock leak on ENOMEM in AMDGPU_GEM_OP_GET_MAPPING_INFO Michael Bommarito
2026-05-18  6:08 ` Claude review: " Claude Code Review Bot
2026-05-18  6:08 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox