[PATCH v2] drm/amdgpu: Fix resource leaks

[PATCH v2] drm/amdgpu: Fix resource leaks

[Ethan Tidmore]

There are multiple resource leaks due to ioremap() being used and
iounmap never being called in multiple possible error paths.

Change ioremap() to devm_ioremap() to fix all resource leaks at
once.

Detected by Smatch:
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:4834 amdgpu_device_init() warn:
'adev->rmmio' from ioremap() not released on lines:
4539,4549,4563,4574,4592,4834.

Fixes: d38ceaf99ed01 ("drm/amdgpu: add core driver (v4)")
Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
---
v2:
- Removed unused variable idx.

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 0acddcb04730..3bc685162974 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4520,7 +4520,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
 		atomic_set(&adev->pm.pwr_state[i], POWER_STATE_UNKNOWN);
 
-	adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
+	adev->rmmio = devm_ioremap(adev->dev, adev->rmmio_base, adev->rmmio_size);
 	if (!adev->rmmio)
 		return -ENOMEM;
 
@@ -4843,8 +4843,6 @@ static void amdgpu_device_unmap_mmio(struct amdgpu_device *adev)
 	/* Unmap all mapped bars - Doorbell, registers and VRAM */
 	amdgpu_doorbell_fini(adev);
 
-	iounmap(adev->rmmio);
-	adev->rmmio = NULL;
 	if (adev->mman.aper_base_kaddr)
 		iounmap(adev->mman.aper_base_kaddr);
 	adev->mman.aper_base_kaddr = NULL;
@@ -4927,7 +4925,7 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 
 void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 {
-	int i, idx;
+	int i;
 	bool px;
 
 	amdgpu_device_ip_fini(adev);
@@ -4970,13 +4968,6 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		vga_client_unregister(adev->pdev);
 
-	if (drm_dev_enter(adev_to_drm(adev), &idx)) {
-
-		iounmap(adev->rmmio);
-		adev->rmmio = NULL;
-		drm_dev_exit(idx);
-	}
-
 	if (IS_ENABLED(CONFIG_PERF_EVENTS))
 		amdgpu_pmu_fini(adev);
 	if (adev->discovery.bin)
-- 
2.53.0

Claude review: drm/amdgpu: Fix resource leaks

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amdgpu: Fix resource leaks
Author: Ethan Tidmore <ethantidmore06@gmail.com>
Patches: 1
Reviewed: 2026-02-27T13:15:06.912896

---

This is a single patch (v2) that converts `ioremap()`/`iounmap()` of `adev->rmmio` to `devm_ioremap()` to fix resource leaks on error paths in `amdgpu_device_init()`. While the Smatch warning about the error-path leaks is legitimate, this fix is **incorrect and introduces a use-after-free bug** that is worse than the original leak.

The fundamental problem is a **lifetime mismatch**: `devm_ioremap()` ties the resource lifetime to the PCI `struct device`, but `adev->rmmio` is accessed from the DRM device `.release` callback (`amdgpu_driver_release_kms` → `amdgpu_device_fini_sw`), which can run **long after** the PCI driver has unbound and devm resources have been freed.

**NAK** — this needs a different approach.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amdgpu: Fix resource leaks

[Claude Code Review Bot]

Patch Review

**The Smatch warning is valid.** There are real `ioremap()` leaks on early error returns in `amdgpu_device_init()`. However, `devm_ioremap()` is the wrong fix for the following reasons:

**1. Use-after-free via DRM device release path**

The PCI removal sequence is:

```
amdgpu_pci_remove()
  → drm_dev_unplug()
  → amdgpu_driver_unload_kms() → amdgpu_device_fini_hw()
  return
devres_release_all()    ← devm_ioremap mapping freed HERE
...
[later, when last DRM fd is closed]
amdgpu_driver_release_kms()
  → amdgpu_device_fini_sw()
    → amdgpu_device_ip_fini()   ← uses stale adev->rmmio!
```

`amdgpu_device_fini_sw()` is called from the DRM `.release` callback (`amdgpu_driver_release_kms` at `amdgpu_kms.c:1615`), which fires when the last DRM fd reference drops. If userspace still has the device open, this runs **after** `devres_release_all()` has already freed the `devm_ioremap` mapping. At that point `adev->rmmio` is a dangling pointer.

**2. Removal of `adev->rmmio = NULL` breaks guards**

The patch removes both:
```c
-	iounmap(adev->rmmio);
-	adev->rmmio = NULL;
```

But `adev->rmmio == NULL` is used as a validity check elsewhere, e.g., in `amdgpu_kms.c:91`:
```c
if (adev->rmmio == NULL)
    return;
```

After this patch, `adev->rmmio` is never NULLed, so this guard becomes ineffective even though the mapping may already have been freed by devm.

**3. Removal of `drm_dev_enter()` guard breaks hotplug coordination**

The removed code in `amdgpu_device_fini_sw()`:
```c
-	if (drm_dev_enter(adev_to_drm(adev), &idx)) {
-		iounmap(adev->rmmio);
-		adev->rmmio = NULL;
-		drm_dev_exit(idx);
-	}
```

This `drm_dev_enter()` guard coordinates with `amdgpu_device_unmap_mmio()` (the hotplug path). When the device is unplugged, `drm_dev_enter()` fails and the `iounmap` is skipped because `amdgpu_device_unmap_mmio()` already handled it. This careful coordination is lost with the patch.

**Suggested fix:** Instead of `devm_ioremap()`, the error paths in `amdgpu_device_init()` should be fixed with proper `goto` cleanup labels that call `iounmap(adev->rmmio)`. This addresses the Smatch warning without changing the resource lifetime semantics. Alternatively, if `devm_ioremap` is desired, the entire teardown logic across `fini_hw` / `fini_sw` / `unmap_mmio` needs to be reworked to ensure `adev->rmmio` is never accessed after the PCI driver unbinds, and `adev->rmmio` must be set to NULL when devm will handle the unmap.

---
Generated by Claude Code Patch Reviewer