[PATCH] drm/amdgpu/userq: fix memory leak in MQD creation error paths

[PATCH] drm/amdgpu/userq: fix memory leak in MQD creation error paths

[Junrui Luo]

In mes_userq_mqd_create(), the memdup_user() allocations for
IP-specific MQD structs are not freed when subsequent VA validation
fails. The goto free_mqd label only cleans up the MQD BO object and
userq_props.

Fix by adding kfree() before each goto free_mqd on VA validation
failure in the COMPUTE, GFX, and SDMA branches.

Fixes: 9e46b8bb0539 ("drm/amdgpu: validate userq buffer virtual address and size")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index 8c74894254f7..faac21ee5739 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -324,8 +324,10 @@ static int mes_userq_mqd_create(struct amdgpu_usermode_queue *queue,
 
 		r = amdgpu_userq_input_va_validate(adev, queue, compute_mqd->eop_va,
 						   2048);
-		if (r)
+		if (r) {
+			kfree(compute_mqd);
 			goto free_mqd;
+		}
 
 		userq_props->eop_gpu_addr = compute_mqd->eop_va;
 		userq_props->hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_NORMAL;
@@ -365,12 +367,16 @@ static int mes_userq_mqd_create(struct amdgpu_usermode_queue *queue,
 
 		r = amdgpu_userq_input_va_validate(adev, queue, mqd_gfx_v11->shadow_va,
 						   shadow_info.shadow_size);
-		if (r)
+		if (r) {
+			kfree(mqd_gfx_v11);
 			goto free_mqd;
+		}
 		r = amdgpu_userq_input_va_validate(adev, queue, mqd_gfx_v11->csa_va,
 						   shadow_info.csa_size);
-		if (r)
+		if (r) {
+			kfree(mqd_gfx_v11);
 			goto free_mqd;
+		}
 
 		kfree(mqd_gfx_v11);
 	} else if (queue->queue_type == AMDGPU_HW_IP_DMA) {
@@ -390,8 +396,10 @@ static int mes_userq_mqd_create(struct amdgpu_usermode_queue *queue,
 		}
 		r = amdgpu_userq_input_va_validate(adev, queue, mqd_sdma_v11->csa_va,
 						   32);
-		if (r)
+		if (r) {
+			kfree(mqd_sdma_v11);
 			goto free_mqd;
+		}
 
 		userq_props->csa_addr = mqd_sdma_v11->csa_va;
 		kfree(mqd_sdma_v11);

---
base-commit: 0257f64bdac7fdca30fa3cae0df8b9ecbec7733a
change-id: 20260314-fixes-f4411ac85e22

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

Re: [PATCH] drm/amdgpu/userq: fix memory leak in MQD creation error paths

[Markus Elfring]

…
> Fix by adding kfree() before each goto free_mqd on VA validation
> failure in the COMPUTE, GFX, and SDMA branches.

How do you think about to benefit any more from application of an attribute
like __free(kfree)?
https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/cleanup.h#L157-L161

Regards,
Markus

Re: [PATCH] drm/amdgpu/userq: fix memory leak in MQD creation error paths

[Junrui Luo]

On Sat, Mar 14, 2026 at 09:04:08PM +0100, Markus Elfring wrote:
> …
> > Fix by adding kfree() before each goto free_mqd on VA validation
> > failure in the COMPUTE, GFX, and SDMA branches.
> 
> How do you think about to benefit any more from application of an attribute
> like __free(kfree)?
> https://elixir.bootlin.com/linux/v7.0-rc3/source/include/linux/cleanup.h#L157-L161

Hi Markus,

Thanks for the suggestion. I considered __free(kfree) but the cleanup.h
comment says scope-based cleanup and "goto" should not be mixed in the same
function. Since mes_userq_mqd_create() relies heavily on goto-based
unwinding, applying __free(kfree) only to the memdup pointers would
violate that guideline.

A full conversion to scope-based cleanup would require restructuring
the entire function, which seems beyond the scope of a bug fix.

Thanks,
Junrui Luo

Re: drm/amdgpu/userq: fix memory leak in MQD creation error paths

[Markus Elfring]

> A full conversion to scope-based cleanup would require restructuring
> the entire function, which seems beyond the scope of a bug fix.

If you would like to stick to the usage of goto labels so far,
I see further possibilities to avoid also duplicate source code for
the affected implementation of the function “mes_userq_mqd_create”.
https://elixir.bootlin.com/linux/v7.0-rc3/source/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c#L275-L434

Regards,
Markus

Claude review: drm/amdgpu/userq: fix memory leak in MQD creation error paths

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amdgpu/userq: fix memory leak in MQD creation error paths
Author: Junrui Luo <moonafterrain@outlook.com>
Patches: 3
Reviewed: 2026-03-16T11:58:18.192377

---

This is a single-patch fix for a real memory leak in `mes_userqueue.c`. The bug analysis is correct: when `amdgpu_userq_input_va_validate()` fails after a successful `memdup_user()`, the allocated IP-specific MQD struct is not freed before jumping to `free_mqd`. The fix is straightforward and appropriate.

**Verdict: Correct fix, with minor suggestions below.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amdgpu/userq: fix memory leak in MQD creation error paths

[Claude Code Review Bot]

Patch Review

**Bug analysis is correct.** Looking at the current tree (`mes_userqueue.c:309-398`), each branch does:
1. `memdup_user()` → allocates a struct (e.g., `compute_mqd`)
2. `amdgpu_userq_input_va_validate()` → may fail
3. `kfree()` → only reached on the success path

The `free_mqd` label does not free these IP-specific allocations, so the patch correctly adds `kfree()` before each `goto free_mqd`.

**All four error paths are addressed:**
- COMPUTE branch: `kfree(compute_mqd)` at line 192
- GFX branch: `kfree(mqd_gfx_v11)` at lines 204 and 211 (two validation calls)
- SDMA branch: `kfree(mqd_sdma_v11)` at line 223

**Minor observations:**

1. **Correctness concern in COMPUTE branch:** In the current tree at line 334, `userq_props->tmz_queue` and other properties are set *after* the validation call. The patch correctly places the `kfree()` before any of those assignments, so this is fine.

2. **Correctness concern in GFX branch:** At lines 361-364 in the tree, `userq_props->shadow_addr` and `userq_props->csa_addr` are assigned from `mqd_gfx_v11` *before* the validation calls. This means on the error path, `userq_props` contains stale VA values from user input. This isn't a problem introduced by this patch (and `userq_props` itself is freed by `free_mqd`), but it's worth noting.

3. **Style suggestion:** Rather than sprinkling `kfree()` at each validation failure, a cleaner approach would be to use a local `goto` label per branch (e.g., `free_compute_mqd`) or restructure so the `kfree()` is always done before any `goto`. However, this fix is minimal and correct, which is appropriate for a stable-targeted bugfix.

4. **Fixes tag:** The referenced commit `9e46b8bb0539` ("drm/amdgpu: validate userq buffer virtual address and size") is the commit that introduced the validation calls (and thus the leak), so the Fixes tag is correct.

**Overall: The patch is correct and ready to apply.** It fixes a genuine memory leak with minimal, targeted changes. The `Cc: stable` tag is appropriate since this is a straightforward resource leak fix.

---
Generated by Claude Code Patch Reviewer