[PATCH 0/5] drm/amdgpu: five independent fixes in the KMS, userq, UVD and CS paths

[PATCH 0/5] drm/amdgpu: five independent fixes in the KMS, userq, UVD and CS paths

[Junrui Luo via B4 Relay]

Five independent fixes; no dependency between them, they can be applied
or dropped individually.

Patch 1 frees fpriv->prt_va on the amdgpu_driver_open_kms() error path.
amdgpu_vm_fini() releases mappings but never a struct amdgpu_bo_va, so a
failure to map the CSA or the seq64 buffer leaks the bo_va and the
dma_fence stub reference it holds.  postclose_kms() already gets this
right; only the open() unwind was missing it.

Patch 2 rejects a mapping without a backing BO in
amdgpu_userq_input_va_validate().  A PRT mapping is routed through
fpriv->prt_va, whose base.bo is NULL, yet a queue_va/rptr_va/wptr_va
inside it passes validation and latches userq_va_mapped.  The next unmap
of any PRT mapping in that VM then dereferences bo_va->base.bo in
amdgpu_userq_gem_va_unmap_validate().

Patch 3 bounds the retry loop in amdgpu_userq_ensure_ev_fence().  Every
failure ahead of amdgpu_evf_mgr_rearm() leaves the restore worker giving
up with only a drm_file_err(), so the waiting thread reschedules and
flushes forever in TASK_UNINTERRUPTIBLE - unkillable and out of reach of
the OOM killer.  The eviction fence sequence number is used as the loop's
progress condition instead.

Patch 4 applies the decode arm's handle ownership test to the UVD destroy
arm.  handles[] and filp[] are per-device, and destroy clears every slot
matching the handle from the command stream without checking the owner,
so one render node client can tear down another's UVD session and leave a
stale filp behind.

Patch 5 releases the userptr HMM ranges in amdgpu_cs_parser_fini().
amdgpu_cs_parser_bos() returns with them live and only two sites free
them; every error edge in between leaks a struct amdgpu_hmm_range plus a
kvmalloc_array() of one hmm_pfn per page of the userptr mapping,
allocated GFP_KERNEL and not charged to the caller's memcg.  An IB
address with no VM mapping is enough to reach one of those edges, so it
is repeatable at will from an unprivileged fd.

Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
Junrui Luo (5):
      drm/amdgpu: free prt_va on the open_kms error path
      drm/amdgpu: reject PRT mappings as user queue buffer VAs
      drm/amdgpu/userq: bound the eviction fence rearm retry loop
      drm/amdgpu: enforce UVD handle ownership on destroy
      drm/amdgpu: free userptr HMM ranges on the CS error path

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c          | 10 +++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c         |  5 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c       | 29 +++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h       |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 ++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c         | 14 ++++++++++--
 6 files changed, 65 insertions(+), 7 deletions(-)
---
base-commit: c4f76bf5e107bcda6e496f1c4060c55af091fa79
change-id: 20260810-amdgpu-fixes-b6ab1059a034

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

[PATCH 0/5] drm/amdgpu: five independent fixes in the KMS, userq, UVD and CS paths

[Junrui Luo]

Five independent fixes; no dependency between them, they can be applied
or dropped individually.

Patch 1 frees fpriv->prt_va on the amdgpu_driver_open_kms() error path.
amdgpu_vm_fini() releases mappings but never a struct amdgpu_bo_va, so a
failure to map the CSA or the seq64 buffer leaks the bo_va and the
dma_fence stub reference it holds.  postclose_kms() already gets this
right; only the open() unwind was missing it.

Patch 2 rejects a mapping without a backing BO in
amdgpu_userq_input_va_validate().  A PRT mapping is routed through
fpriv->prt_va, whose base.bo is NULL, yet a queue_va/rptr_va/wptr_va
inside it passes validation and latches userq_va_mapped.  The next unmap
of any PRT mapping in that VM then dereferences bo_va->base.bo in
amdgpu_userq_gem_va_unmap_validate().

Patch 3 bounds the retry loop in amdgpu_userq_ensure_ev_fence().  Every
failure ahead of amdgpu_evf_mgr_rearm() leaves the restore worker giving
up with only a drm_file_err(), so the waiting thread reschedules and
flushes forever in TASK_UNINTERRUPTIBLE - unkillable and out of reach of
the OOM killer.  The eviction fence sequence number is used as the loop's
progress condition instead.

Patch 4 applies the decode arm's handle ownership test to the UVD destroy
arm.  handles[] and filp[] are per-device, and destroy clears every slot
matching the handle from the command stream without checking the owner,
so one render node client can tear down another's UVD session and leave a
stale filp behind.

Patch 5 releases the userptr HMM ranges in amdgpu_cs_parser_fini().
amdgpu_cs_parser_bos() returns with them live and only two sites free
them; every error edge in between leaks a struct amdgpu_hmm_range plus a
kvmalloc_array() of one hmm_pfn per page of the userptr mapping,
allocated GFP_KERNEL and not charged to the caller's memcg.  An IB
address with no VM mapping is enough to reach one of those edges, so it
is repeatable at will from an unprivileged fd.

Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
Junrui Luo (5):
      drm/amdgpu: free prt_va on the open_kms error path
      drm/amdgpu: reject PRT mappings as user queue buffer VAs
      drm/amdgpu/userq: bound the eviction fence rearm retry loop
      drm/amdgpu: enforce UVD handle ownership on destroy
      drm/amdgpu: free userptr HMM ranges on the CS error path

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c          | 10 +++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c         |  5 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c       | 29 +++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h       |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 ++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c         | 14 ++++++++++--
 6 files changed, 65 insertions(+), 7 deletions(-)
---
base-commit: c4f76bf5e107bcda6e496f1c4060c55af091fa79
change-id: 20260810-amdgpu-fixes-b6ab1059a034

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

[PATCH 1/5] drm/amdgpu: free prt_va on the open_kms error path

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

amdgpu_driver_open_kms() creates fpriv->prt_va with amdgpu_vm_bo_add()
before mapping the CSA and the seq64 buffer. If either mapping fails
the function jumps to error_vm, which only calls amdgpu_vm_fini() and
then frees fpriv. amdgpu_vm_fini() releases the amdgpu_bo_va_mapping
objects reachable from vm->freed and the vm->va rbtree, but it never
frees a struct amdgpu_bo_va, so the bo_va allocated for prt_va and the
dma_fence stub reference it holds are both lost.

The success path does get this right: amdgpu_driver_postclose_kms()
reserves the root PD and calls amdgpu_vm_bo_del(adev, fpriv->prt_va)
before amdgpu_vm_fini(). Only the open() unwind is missing it.

Drop the bo_va on the error path as well, reserving the root PD as
amdgpu_vm_bo_del() requires.

Fixes: b85891bd6d1b ("drm/amdgpu: IOCTL interface for PRT support v4")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 242c48e85912..7ef1c1dcc207 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1553,6 +1553,11 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 		pasid = 0;
 	}
 
+	if (fpriv->prt_va &&
+	    !WARN_ON(amdgpu_bo_reserve(fpriv->vm.root.bo, true))) {
+		amdgpu_vm_bo_del(adev, fpriv->prt_va);
+		amdgpu_bo_unreserve(fpriv->vm.root.bo);
+	}
 	amdgpu_vm_fini(adev, &fpriv->vm);
 
 error_pasid:

-- 
2.51.2

[PATCH 1/5] drm/amdgpu: free prt_va on the open_kms error path

[Junrui Luo]

amdgpu_driver_open_kms() creates fpriv->prt_va with amdgpu_vm_bo_add()
before mapping the CSA and the seq64 buffer. If either mapping fails
the function jumps to error_vm, which only calls amdgpu_vm_fini() and
then frees fpriv. amdgpu_vm_fini() releases the amdgpu_bo_va_mapping
objects reachable from vm->freed and the vm->va rbtree, but it never
frees a struct amdgpu_bo_va, so the bo_va allocated for prt_va and the
dma_fence stub reference it holds are both lost.

The success path does get this right: amdgpu_driver_postclose_kms()
reserves the root PD and calls amdgpu_vm_bo_del(adev, fpriv->prt_va)
before amdgpu_vm_fini(). Only the open() unwind is missing it.

Drop the bo_va on the error path as well, reserving the root PD as
amdgpu_vm_bo_del() requires.

Fixes: b85891bd6d1b ("drm/amdgpu: IOCTL interface for PRT support v4")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 242c48e85912..7ef1c1dcc207 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1553,6 +1553,11 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 		pasid = 0;
 	}
 
+	if (fpriv->prt_va &&
+	    !WARN_ON(amdgpu_bo_reserve(fpriv->vm.root.bo, true))) {
+		amdgpu_vm_bo_del(adev, fpriv->prt_va);
+		amdgpu_bo_unreserve(fpriv->vm.root.bo);
+	}
 	amdgpu_vm_fini(adev, &fpriv->vm);
 
 error_pasid:

-- 
2.51.2

Re: [PATCH 1/5] drm/amdgpu: free prt_va on the open_kms error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for memory leak and use-after-free on `error_vm` path regarding `csa_va`.
- [High] Bypassing `drm_exec_fini()` on the error path out of the `drm_exec_until_all_locked()` loop causes a memory leak and WW context leak.
--

--- Patch [1]: [PATCH 1/5] drm/amdgpu: free prt_va on the open_kms error path ---
commit 89bac7ae7a8efcfa06425874df2dcd14db52ca52
Author: Junrui Luo <moonafterrain@outlook.com>

drm/amdgpu: free prt_va on the open_kms error path

amdgpu_driver_open_kms() creates fpriv->prt_va with amdgpu_vm_bo_add()
before mapping the CSA and the seq64 buffer. If either mapping fails
the function jumps to error_vm, which only calls amdgpu_vm_fini() and
then frees fpriv. amdgpu_vm_fini() releases the amdgpu_bo_va_mapping
objects reachable from vm->freed and the vm->va rbtree, but it never
frees a struct amdgpu_bo_va, so the bo_va allocated for prt_va and the
dma_fence stub reference it holds are both lost.

Fixes: b85891bd6d1b ("drm/amdgpu: IOCTL interface for PRT support v4")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 242c48e85912f..7ef1c1dcc2079 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1509,6 +1509,6 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0);
>  	drm_exec_until_all_locked(&exec) {
>  		r = amdgpu_vm_lock_pd(&fpriv->vm, &exec, 0);
>  		drm_exec_retry_on_contention(&exec);
>  		if (unlikely(r))
>  			goto error_vm;

[Severity: High]
This is a pre-existing issue, but does this goto error_vm break out of the
loop and bypass the drm_exec_fini() call located immediately after?

Skipping drm_exec_fini() leaks the exec.objects array allocated by
drm_exec_init() and fails to call ww_acquire_fini(), which leaks the WW context
and triggers lockdep splats.

[ ... ]

> @@ -1553,6 +1553,11 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  		pasid = 0;
>  	}
>  
> +	if (fpriv->prt_va &&
> +	    !WARN_ON(amdgpu_bo_reserve(fpriv->vm.root.bo, true))) {
> +		amdgpu_vm_bo_del(adev, fpriv->prt_va);
> +		amdgpu_bo_unreserve(fpriv->vm.root.bo);
> +	}

[Severity: High]
This is a pre-existing issue, but if amdgpu_seq64_map() fails earlier, the
code jumps to the error_vm label. While this patch correctly frees prt_va,
does it fail to free csa_va?

Since csa_va was already successfully mapped and linked into the global static
CSA BO's vm_bo list, and fpriv is subsequently freed, does this dangling
csa_va pointer contain a vm backpointer pointing to freed memory?

Any subsequent traversal of the global CSA BO's mappings (e.g., during
eviction) could trigger a use-after-free.

Additionally, if amdgpu_map_static_csa() or amdgpu_seq64_map() fails
internally, do they call amdgpu_vm_bo_del() without zeroing *bo_va?

amdgpu_csa.c:amdgpu_map_static_csa() {
    ...
    amdgpu_vm_bo_del(adev, *bo_va);
    goto error;
}

Could this leave a dangling pointer in fpriv that prevents a safe
if (fpriv->csa_va) cleanup on the error path?

>  	amdgpu_vm_fini(adev, &fpriv->vm);
>  
>  error_pasid:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=1

[PATCH 2/5] drm/amdgpu: reject PRT mappings as user queue buffer VAs

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

amdgpu_userq_input_va_validate() resolves a user-supplied queue_va,
rptr_va or wptr_va to a VM mapping and latches userq_va_mapped on
the owning bo_va. It only checks that a mapping exists and that
the requested span is contained in it, never that the mapping has
a backing BO. PRT mappings do not: amdgpu_gem_va_ioctl() routes
every AMDGPU_VM_PAGE_PRT map through fpriv->prt_va, created via
amdgpu_vm_bo_add(adev, vm, NULL), so base.bo stays NULL while
amdgpu_vm_bo_insert_map() still sets mapping->bo_va.

A VA inside such a mapping therefore passes validation and marks
fpriv->prt_va as userq mapped. The flag is never cleared. On the
next unmap of any PRT mapping in that VM, amdgpu_vm_bo_unmap() sees
userq_va_mapped and calls amdgpu_userq_gem_va_unmap_validate(), which
reads bo_va->base.bo->tbo.base.resv before its ip_mask guard, leading
to a NULL pointer dereference.

Fix by rejecting a mapping without a backing BO in the validation
helper, so the invariant amdgpu_userq_gem_va_unmap_validate() relies
on holds by construction. A sparse mapping has no memory behind it and
cannot serve as a ring, rptr or wptr buffer.

Fixes: 2e7ceac0ea41 ("drm/amdgpu: validate userq va for GEM unmap")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 6d3ed55e9ab4..bec107216811 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -259,6 +259,14 @@ int amdgpu_userq_input_va_validate(struct amdgpu_device *adev,
 	if (!va_map)
 		return -EINVAL;
 
+	/*
+	 * A PRT mapping has no backing BO and so can't carry the eviction
+	 * fence which amdgpu_userq_gem_va_unmap_validate() waits on. Reject it
+	 * here, otherwise that helper dereferences a NULL bo on GEM unmap.
+	 */
+	if (!va_map->bo_va->base.bo)
+		return -EINVAL;
+
 	/* Lookup guarantees start_page is mapped; ensure full span is covered. */
 	if ((end_addr >> AMDGPU_GPU_PAGE_SHIFT) <= va_map->last) {
 		va_map->bo_va->userq_va_mapped = true;

-- 
2.51.2

[PATCH 2/5] drm/amdgpu: reject PRT mappings as user queue buffer VAs

[Junrui Luo]

amdgpu_userq_input_va_validate() resolves a user-supplied queue_va,
rptr_va or wptr_va to a VM mapping and latches userq_va_mapped on
the owning bo_va. It only checks that a mapping exists and that
the requested span is contained in it, never that the mapping has
a backing BO. PRT mappings do not: amdgpu_gem_va_ioctl() routes
every AMDGPU_VM_PAGE_PRT map through fpriv->prt_va, created via
amdgpu_vm_bo_add(adev, vm, NULL), so base.bo stays NULL while
amdgpu_vm_bo_insert_map() still sets mapping->bo_va.

A VA inside such a mapping therefore passes validation and marks
fpriv->prt_va as userq mapped. The flag is never cleared. On the
next unmap of any PRT mapping in that VM, amdgpu_vm_bo_unmap() sees
userq_va_mapped and calls amdgpu_userq_gem_va_unmap_validate(), which
reads bo_va->base.bo->tbo.base.resv before its ip_mask guard, leading
to a NULL pointer dereference.

Fix by rejecting a mapping without a backing BO in the validation
helper, so the invariant amdgpu_userq_gem_va_unmap_validate() relies
on holds by construction. A sparse mapping has no memory behind it and
cannot serve as a ring, rptr or wptr buffer.

Fixes: 2e7ceac0ea41 ("drm/amdgpu: validate userq va for GEM unmap")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 6d3ed55e9ab4..bec107216811 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -259,6 +259,14 @@ int amdgpu_userq_input_va_validate(struct amdgpu_device *adev,
 	if (!va_map)
 		return -EINVAL;
 
+	/*
+	 * A PRT mapping has no backing BO and so can't carry the eviction
+	 * fence which amdgpu_userq_gem_va_unmap_validate() waits on. Reject it
+	 * here, otherwise that helper dereferences a NULL bo on GEM unmap.
+	 */
+	if (!va_map->bo_va->base.bo)
+		return -EINVAL;
+
 	/* Lookup guarantees start_page is mapped; ensure full span is covered. */
 	if ((end_addr >> AMDGPU_GPU_PAGE_SHIFT) <= va_map->last) {
 		va_map->bo_va->userq_va_mapped = true;

-- 
2.51.2

Re: [PATCH 2/5] drm/amdgpu: reject PRT mappings as user queue buffer VAs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch prevents PRT mappings from being validated as user queue VAs to avoid NULL pointer dereferences, but userspace can bypass this restriction via a Time-Of-Check to Time-Of-Use (TOCTOU) race, leading to the exact same NULL pointer dereferences.
- [High] GPU coredump crashes with a NULL pointer dereference if an Indirect Buffer (IB) resides in a PRT mapping.
- [High] Providing a PRT mapping during KFD queue creation permanently leaks `queue_refcount`, leading to a denial of service.
- [High] Userspace can completely bypass the user queue eviction synchronization by using `AMDGPU_VA_OP_CLEAR` or `AMDGPU_VA_OP_REPLACE`.
--

--- Patch [2]: [PATCH 2/5] drm/amdgpu: reject PRT mappings as user queue buffer VAs ---
commit 2673e3bf87d8321b443028125d445c8f3dba144c
Author: Junrui Luo <moonafterrain@outlook.com>

drm/amdgpu: reject PRT mappings as user queue buffer VAs

This commit fixes a NULL pointer dereference by rejecting PRT mappings during
user queue VA validation. It ensures that only mappings with backing BOs can
be used as ring, rptr, or wptr buffers.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 6d3ed55e9ab4b..bec107216811c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c

[ ... ]

> @@ -259,6 +259,14 @@ int amdgpu_userq_input_va_validate(struct amdgpu_device *adev,
>  	if (!va_map)
>  		return -EINVAL;
>  
> +	/*
> +	 * A PRT mapping has no backing BO and so can't carry the eviction
> +	 * fence which amdgpu_userq_gem_va_unmap_validate() waits on. Reject it
> +	 * here, otherwise that helper dereferences a NULL bo on GEM unmap.
> +	 */
> +	if (!va_map->bo_va->base.bo)
> +		return -EINVAL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this validation fully
prevent the NULL pointer dereference?

Userspace might bypass this restriction via a Time-Of-Check to Time-Of-Use
(TOCTOU) race. After amdgpu_userq_input_va_validate() completes, the VM root
PD is temporarily unreserved before mes_userq_mqd_create() is called. A
concurrent thread could call AMDGPU_GEM_VA_IOCTL to unmap the wptr_va and
remap it as a PRT mapping.

When mes_userq_create_wptr_mapping() or amdgpu_userq_fence_read_wptr() 
executes:

drivers/gpu/drm/amd/amdgpu/mes_userqueue.c:mes_userq_create_wptr_mapping() {
    wptr_mapping = amdgpu_vm_bo_lookup_mapping(vm, wptr >> PAGE_SHIFT);
    ...
    obj = wptr_mapping->bo_va->base.bo;
    ret = drm_exec_lock_obj(&exec, &obj->tbo.base);
}

Could the mapping now be a PRT mapping, causing obj to be NULL and triggering
a kernel panic when drm_exec_lock_obj() dereferences it?

[Severity: High]
This is also a pre-existing issue, but does the synchronization mechanism this
patch aims to protect actually cover all unmap paths?

Userspace can submit an AMDGPU_GEM_VA_IOCTL with AMDGPU_VA_OP_CLEAR or
AMDGPU_VA_OP_REPLACE instead of AMDGPU_VA_OP_UNMAP. This routes to
amdgpu_vm_bo_clear_mappings(), which removes the mappings without checking
userq_va_mapped:

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_bo_clear_mappings() {
    list_for_each_entry_safe(tmp, next, &removed, list) {
        amdgpu_vm_it_remove(tmp, &vm->va);
        ...
        tmp->bo_va = NULL;
        list_add(&tmp->list, &vm->freed);
        trace_amdgpu_vm_bo_unmap(NULL, tmp);
    }
}

Does this bypass the amdgpu_userq_gem_va_unmap_validate() wait entirely,
allowing userspace to unmap active hardware queues while the GPU is still
accessing them?

[Severity: High]
This is a pre-existing issue, but there appears to be another unhandled edge
case with PRT mappings causing a similar NULL pointer dereference elsewhere.

If userspace submits an Indirect Buffer (IB) pointing to a PRT mapping and
triggers a GPU reset, the GPU coredump handler iterates over the IBs to dump
them. In amdgpu_devcoredump_print_ibs():

drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c:amdgpu_devcoredump_print_ibs() {
    mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn);
    if (!mapping)
        continue;

    abo = mapping->bo_va->base.bo;
    r = drm_exec_lock_obj(&exec, &abo->tbo.base);
}

Since abo is NULL for PRT mappings, will this unconditionally crash the system
during the coredump?

[Severity: High]
This is a pre-existing issue, but there seems to be one more unhandled PRT
mapping flaw in the KFD subsystem.

If userspace calls a KFD queue creation IOCTL providing a PRT mapping for
one of the queue buffers, kfd_queue_buffer_get() successfully increments the
refcount:

drivers/gpu/drm/amd/amdkfd/kfd_queue.c:kfd_queue_buffer_get() {
    mapping = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
    ...
    *pbo = amdgpu_bo_ref(mapping->bo_va->base.bo);
    mapping->bo_va->queue_refcount++;
}

Because *pbo is set to NULL, kfd_queue_unref_bo_va() skips the decrement
during queue destruction:

drivers/gpu/drm/amd/amdkfd/kfd_queue.c:kfd_queue_unref_bo_va() {
    if (*bo) {
        struct amdgpu_bo_va *bo_va;
        ...
        if (bo_va && bo_va->queue_refcount)
            bo_va->queue_refcount--;
    }
}

Does this permanently leak the queue_refcount and prevent the mapping from
ever being unmapped?

>  	/* Lookup guarantees start_page is mapped; ensure full span is covered. */
>  	if ((end_addr >> AMDGPU_GPU_PAGE_SHIFT) <= va_map->last) {
>  		va_map->bo_va->userq_va_mapped = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=2

[PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

amdgpu_userq_ensure_ev_fence() loops until the eviction fence is both
present and unsignaled.  The only producer of such a fence is
amdgpu_evf_mgr_rearm(), which runs as the very last step of
amdgpu_userq_vm_validate().  Every failure point ahead of it - the
kzalloc() in the rearm itself, amdgpu_hmm_range_alloc(), the
ttm_bo_validate() calls, the GART binding of the wptr BOs - makes
amdgpu_userq_restore_worker() give up with only a drm_file_err().
Nothing propagates that back, so the waiting thread reschedules the
worker and flushes it again, forever.

Both flush_delayed_work() and mutex_lock() sleep in
TASK_UNINTERRUPTIBLE, so the looping task cannot be killed and the OOM
killer cannot reclaim it. An unprivileged render node client
reaches this from both AMDGPU_USERQ and AMDGPU_USERQ_SIGNAL.

The eviction fence sequence number is already bumped by every
successful rearm, so use it as the loop's progress condition: if a
completed flush of the restore worker did not move it then no rearm
happened and retrying cannot help.  Return -ENOMEM in that case and
let both callers report it to userspace.

Fixes: a242a3e4b5be ("drm/amdgpu: simplify eviction fence suspend/resume")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c       | 21 +++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h       |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 +++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index bec107216811..208b53ae5bd1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue)
  * Ensures that a valid and not yet signaled eviction fence is attached to the
  * usermode queue before any queue operations proceed. If it is signalled, then
  * rearm a new eviction fence.
+ *
+ * Returns 0 with @uq_mgr->userq_mutex held, or -ENOMEM with the mutex released
+ * when the restore worker could not rearm the fence.
  */
-void
+int
 amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
 			     struct amdgpu_eviction_fence_mgr *evf_mgr)
 {
 	struct dma_fence *ev_fence;
+	int seq, prev_seq = -1;
 
 retry:
 	/* Flush any pending resume work to create ev_fence */
@@ -463,7 +467,16 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
 	ev_fence = amdgpu_evf_mgr_get_fence(evf_mgr);
 	if (dma_fence_is_signaled(ev_fence)) {
 		dma_fence_put(ev_fence);
+		seq = atomic_read(&evf_mgr->ev_fence_seq);
 		mutex_unlock(&uq_mgr->userq_mutex);
+		/*
+		 * The sequence number is only bumped by a successful rearm, so
+		 * if the flush above ran the worker without moving it then the
+		 * restore failed and looping again would never terminate.
+		 */
+		if (seq == prev_seq)
+			return -ENOMEM;
+		prev_seq = seq;
 		/*
 		 * Looks like there was no pending resume work,
 		 * add one now to create a valid eviction fence
@@ -472,6 +485,8 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
 		goto retry;
 	}
 	dma_fence_put(ev_fence);
+
+	return 0;
 }
 
 
@@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	if (r)
 		goto clean_mqd;
 
-	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	if (r)
+		goto erase_doorbell;
 
 	/* don't map the queue if scheduling is halted */
 	if (!adev->userq_halt_for_enforce_isolation ||
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index 6412a7f7b6ef..c35909bf7ceb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -164,8 +164,8 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr);
 
 void amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr);
 
-void amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
-				  struct amdgpu_eviction_fence_mgr *evf_mgr);
+int amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
+				 struct amdgpu_eviction_fence_mgr *evf_mgr);
 
 u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev);
 bool amdgpu_userq_enabled(struct drm_device *dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index 7e80442ec3e5..1c287ce59736 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
 		goto put_queue;
 
 	/* We are here means UQ is active, make sure the eviction fence is valid */
-	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	if (r) {
+		/* The fence is not initialized yet, so unwind it by hand */
+		amdgpu_userq_fence_put_fence_drv_array(fence);
+		amdgpu_userq_fence_driver_put(fence->fence_drv);
+		kvfree(fence->fence_drv_array);
+		kfree(fence);
+		goto put_queue;
+	}
 
 	/* Create the new fence */
 	amdgpu_userq_fence_init(queue, fence, wptr);

-- 
2.51.2

[PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

[Junrui Luo]

amdgpu_userq_ensure_ev_fence() loops until the eviction fence is both
present and unsignaled.  The only producer of such a fence is
amdgpu_evf_mgr_rearm(), which runs as the very last step of
amdgpu_userq_vm_validate().  Every failure point ahead of it - the
kzalloc() in the rearm itself, amdgpu_hmm_range_alloc(), the
ttm_bo_validate() calls, the GART binding of the wptr BOs - makes
amdgpu_userq_restore_worker() give up with only a drm_file_err().
Nothing propagates that back, so the waiting thread reschedules the
worker and flushes it again, forever.

Both flush_delayed_work() and mutex_lock() sleep in
TASK_UNINTERRUPTIBLE, so the looping task cannot be killed and the OOM
killer cannot reclaim it. An unprivileged render node client
reaches this from both AMDGPU_USERQ and AMDGPU_USERQ_SIGNAL.

The eviction fence sequence number is already bumped by every
successful rearm, so use it as the loop's progress condition: if a
completed flush of the restore worker did not move it then no rearm
happened and retrying cannot help.  Return -ENOMEM in that case and
let both callers report it to userspace.

Fixes: a242a3e4b5be ("drm/amdgpu: simplify eviction fence suspend/resume")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c       | 21 +++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h       |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 +++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index bec107216811..208b53ae5bd1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue)
  * Ensures that a valid and not yet signaled eviction fence is attached to the
  * usermode queue before any queue operations proceed. If it is signalled, then
  * rearm a new eviction fence.
+ *
+ * Returns 0 with @uq_mgr->userq_mutex held, or -ENOMEM with the mutex released
+ * when the restore worker could not rearm the fence.
  */
-void
+int
 amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
 			     struct amdgpu_eviction_fence_mgr *evf_mgr)
 {
 	struct dma_fence *ev_fence;
+	int seq, prev_seq = -1;
 
 retry:
 	/* Flush any pending resume work to create ev_fence */
@@ -463,7 +467,16 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
 	ev_fence = amdgpu_evf_mgr_get_fence(evf_mgr);
 	if (dma_fence_is_signaled(ev_fence)) {
 		dma_fence_put(ev_fence);
+		seq = atomic_read(&evf_mgr->ev_fence_seq);
 		mutex_unlock(&uq_mgr->userq_mutex);
+		/*
+		 * The sequence number is only bumped by a successful rearm, so
+		 * if the flush above ran the worker without moving it then the
+		 * restore failed and looping again would never terminate.
+		 */
+		if (seq == prev_seq)
+			return -ENOMEM;
+		prev_seq = seq;
 		/*
 		 * Looks like there was no pending resume work,
 		 * add one now to create a valid eviction fence
@@ -472,6 +485,8 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
 		goto retry;
 	}
 	dma_fence_put(ev_fence);
+
+	return 0;
 }
 
 
@@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	if (r)
 		goto clean_mqd;
 
-	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	if (r)
+		goto erase_doorbell;
 
 	/* don't map the queue if scheduling is halted */
 	if (!adev->userq_halt_for_enforce_isolation ||
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index 6412a7f7b6ef..c35909bf7ceb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -164,8 +164,8 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr);
 
 void amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr);
 
-void amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
-				  struct amdgpu_eviction_fence_mgr *evf_mgr);
+int amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
+				 struct amdgpu_eviction_fence_mgr *evf_mgr);
 
 u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev);
 bool amdgpu_userq_enabled(struct drm_device *dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index 7e80442ec3e5..1c287ce59736 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
 		goto put_queue;
 
 	/* We are here means UQ is active, make sure the eviction fence is valid */
-	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
+	if (r) {
+		/* The fence is not initialized yet, so unwind it by hand */
+		amdgpu_userq_fence_put_fence_drv_array(fence);
+		amdgpu_userq_fence_driver_put(fence->fence_drv);
+		kvfree(fence->fence_drv_array);
+		kfree(fence);
+		goto put_queue;
+	}
 
 	/* Create the new fence */
 	amdgpu_userq_fence_init(queue, fence, wptr);

-- 
2.51.2

Re: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] The retry loop in amdgpu_userq_ensure_ev_fence() fails permanently with -ENOMEM when ev_fence_seq wraps to -1.

Pre-existing issues:
- [High] Wait dependencies are permanently lost if an error path unwinds the IOCTL or if the IOCTL restarts.
- [High] Missing cancellation of queue->hang_detect_work in the amdgpu_userq_create() error path leads to Use-After-Free.
- [High] Leaking fence_drv references stored in fence_drv_array when a fence is destroyed on error paths or queue teardown.
- [High] Missing rcu_assign_pointer and premature reference drop when updating evf_mgr->ev_fence causes deadlock and uninitialized memory access.
--

--- Patch [3]: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop ---
commit ad19a8a5be75c8d4ceb626d159cd7abccb2976c4
Author: Junrui Luo <moonafterrain@outlook.com>

drm/amdgpu/userq: bound the eviction fence rearm retry loop

This patch bounds a previously infinite loop in the userq eviction fence
rearm logic by verifying that the sequence number has advanced. It returns
-ENOMEM if the rearm worker fails to make progress.

> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue)
>  int
>  amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  			     struct amdgpu_eviction_fence_mgr *evf_mgr)
>  {
>  	struct dma_fence *ev_fence;
> +	int seq, prev_seq = -1;
>  
>  retry:
[ ... ]
>  	if (dma_fence_is_signaled(ev_fence)) {
>  		dma_fence_put(ev_fence);
> +		seq = atomic_read(&evf_mgr->ev_fence_seq);
>  		mutex_unlock(&uq_mgr->userq_mutex);
> +		/*
> +		 * The sequence number is only bumped by a successful rearm, so
> +		 * if the flush above ran the worker without moving it then the
> +		 * restore failed and looping again would never terminate.
> +		 */
> +		if (seq == prev_seq)
> +			return -ENOMEM;

[Severity: Medium]
Could this condition match incorrectly if the sequence number wraps around?

Since ev_fence_seq is a 32-bit atomic counter incremented on successful
rearms, it will eventually wrap to -1 (0xFFFFFFFF).

If prev_seq is initialized to -1, seq == prev_seq would evaluate to true
on the first loop iteration when the counter wraps.

Does this cause amdgpu_userq_ensure_ev_fence() to immediately return
-ENOMEM without attempting to schedule the resume worker, permanently
wedging the user queue manager?

> +		prev_seq = seq;
[ ... ]
> @@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  	if (r)
>  		goto clean_mqd;
>  
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r)
> +		goto erase_doorbell;

[Severity: High]
This is a pre-existing issue, but can the goto erase_doorbell error path
cause a use-after-free of the queue structure?

Looking at amdgpu_userq_create(), the queue is inserted into
adev->userq_doorbell_xa earlier in the function, making it globally
visible.

If a hardware fault occurs and schedules queue->hang_detect_work, and then
this error path frees the queue without calling
cancel_delayed_work_sync(&queue->hang_detect_work), will the deferred work
access the freed queue later?

>  	/* don't map the queue if scheduling is halted */
[ ... ]
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  		goto put_queue;
>  
>  	/* We are here means UQ is active, make sure the eviction fence is valid */
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r) {
> +		/* The fence is not initialized yet, so unwind it by hand */
> +		amdgpu_userq_fence_put_fence_drv_array(fence);
> +		amdgpu_userq_fence_driver_put(fence->fence_drv);
> +		kvfree(fence->fence_drv_array);
> +		kfree(fence);
> +		goto put_queue;
> +	}

[Severity: High]
This is a pre-existing issue, but does unwinding the fence here permanently
lose synchronization dependencies?

In amdgpu_userq_fence_alloc(), xa_extract() destructively removes fence
drivers from queue->fence_drv_xa and moves them into fence_drv_array.

If we abort here, the fence and its array are freed, but the dependencies
are never restored to the xarray. If the IOCTL restarts (for example, from
-ERESTARTSYS later), won't it create a new fence missing all previous wait
dependencies?

[Severity: High]
This is also a pre-existing issue, but does amdgpu_userq_fence_free()
leak these extracted dependencies on normal release paths?

While the hand-unwind path above correctly calls
amdgpu_userq_fence_put_fence_drv_array(fence), the standard cleanup
routine misses this:

drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c:amdgpu_userq_fence_free() {
    ...
    kvfree(userq_fence->fence_drv_array);
    kfree(userq_fence);
    ...
}

Since amdgpu_userq_fence_free() frees the array memory without dropping the
refcounts of the fence_drv objects inside it, do they leak permanently?

>  
>  	/* Create the new fence */
>  	amdgpu_userq_fence_init(queue, fence, wptr);
[ ... ]

While reviewing the eviction fence code related to this patch, I noticed
another pre-existing issue:

[Severity: High]
This is a pre-existing issue, but does this direct assignment without
rcu_assign_pointer() risk a deadlock and uninitialized memory access?

drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c:amdgpu_evf_mgr_rearm() {
    ...
    dma_fence_put(evf_mgr->ev_fence);
    evf_mgr->ev_fence = &ev_fence->base;
    ...
}

evf_mgr->ev_fence is an RCU annotated pointer. Dropping the reference
to the old fence before the assignment could leave the pointer referencing a
zero-refcount object if preempted here.

A concurrent reader using dma_fence_get_rcu_safe() might enter an infinite
loop if kref_get_unless_zero() fails. Furthermore, without a release
barrier, concurrent readers might see the new fence before it is fully
initialized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=3

Re: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

[Christian König]

On 8/10/26 18:13, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <moonafterrain@outlook.com>
> 
> amdgpu_userq_ensure_ev_fence() loops until the eviction fence is both
> present and unsignaled.  The only producer of such a fence is
> amdgpu_evf_mgr_rearm(), which runs as the very last step of
> amdgpu_userq_vm_validate().  Every failure point ahead of it - the
> kzalloc() in the rearm itself, amdgpu_hmm_range_alloc(), the
> ttm_bo_validate() calls, the GART binding of the wptr BOs - makes
> amdgpu_userq_restore_worker() give up with only a drm_file_err().
> Nothing propagates that back, so the waiting thread reschedules the
> worker and flushes it again, forever.
> 
> Both flush_delayed_work() and mutex_lock() sleep in
> TASK_UNINTERRUPTIBLE, so the looping task cannot be killed and the OOM
> killer cannot reclaim it. An unprivileged render node client
> reaches this from both AMDGPU_USERQ and AMDGPU_USERQ_SIGNAL.
> 
> The eviction fence sequence number is already bumped by every
> successful rearm, so use it as the loop's progress condition: if a
> completed flush of the restore worker did not move it then no rearm
> happened and retrying cannot help.  Return -ENOMEM in that case and
> let both callers report it to userspace.
> 
> Fixes: a242a3e4b5be ("drm/amdgpu: simplify eviction fence suspend/resume")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>

Absolutely clear NAK!

This functions needs to loop forever should the rearm worker fails to re-arm the fence.

The only thing which could potentially get out of that is to kill the process or maybe that the eviction fence is signaled with an error.

Regards,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c       | 21 +++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h       |  4 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 +++++++++-
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index bec107216811..208b53ae5bd1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue)
>   * Ensures that a valid and not yet signaled eviction fence is attached to the
>   * usermode queue before any queue operations proceed. If it is signalled, then
>   * rearm a new eviction fence.
> + *
> + * Returns 0 with @uq_mgr->userq_mutex held, or -ENOMEM with the mutex released
> + * when the restore worker could not rearm the fence.
>   */
> -void
> +int
>  amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  			     struct amdgpu_eviction_fence_mgr *evf_mgr)
>  {
>  	struct dma_fence *ev_fence;
> +	int seq, prev_seq = -1;
>  
>  retry:
>  	/* Flush any pending resume work to create ev_fence */
> @@ -463,7 +467,16 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  	ev_fence = amdgpu_evf_mgr_get_fence(evf_mgr);
>  	if (dma_fence_is_signaled(ev_fence)) {
>  		dma_fence_put(ev_fence);
> +		seq = atomic_read(&evf_mgr->ev_fence_seq);
>  		mutex_unlock(&uq_mgr->userq_mutex);
> +		/*
> +		 * The sequence number is only bumped by a successful rearm, so
> +		 * if the flush above ran the worker without moving it then the
> +		 * restore failed and looping again would never terminate.
> +		 */
> +		if (seq == prev_seq)
> +			return -ENOMEM;
> +		prev_seq = seq;
>  		/*
>  		 * Looks like there was no pending resume work,
>  		 * add one now to create a valid eviction fence
> @@ -472,6 +485,8 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  		goto retry;
>  	}
>  	dma_fence_put(ev_fence);
> +
> +	return 0;
>  }
>  
>  
> @@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  	if (r)
>  		goto clean_mqd;
>  
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r)
> +		goto erase_doorbell;
>  
>  	/* don't map the queue if scheduling is halted */
>  	if (!adev->userq_halt_for_enforce_isolation ||
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> index 6412a7f7b6ef..c35909bf7ceb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> @@ -164,8 +164,8 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr);
>  
>  void amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr);
>  
> -void amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
> -				  struct amdgpu_eviction_fence_mgr *evf_mgr);
> +int amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr,
> +				 struct amdgpu_eviction_fence_mgr *evf_mgr);
>  
>  u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev);
>  bool amdgpu_userq_enabled(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 7e80442ec3e5..1c287ce59736 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  		goto put_queue;
>  
>  	/* We are here means UQ is active, make sure the eviction fence is valid */
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r) {
> +		/* The fence is not initialized yet, so unwind it by hand */
> +		amdgpu_userq_fence_put_fence_drv_array(fence);
> +		amdgpu_userq_fence_driver_put(fence->fence_drv);
> +		kvfree(fence->fence_drv_array);
> +		kfree(fence);
> +		goto put_queue;
> +	}
>  
>  	/* Create the new fence */
>  	amdgpu_userq_fence_init(queue, fence, wptr);
> 

[PATCH 4/5] drm/amdgpu: enforce UVD handle ownership on destroy

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

amdgpu_uvd_cs_msg() validates that a decode message references a handle
owned by the submitting client, rejecting a mismatch between
adev->uvd.filp[i] and ctx->parser->filp. The handles[] and filp[] tables
are per-device and shared by every drm_file that opens the render node.

The destroy message performs no such check: it walks the whole table and
clears every slot matching the handle taken from the command stream
buffer. A client can therefore destroy a handle owned by another client,
clearing the victim's slot and tearing down its session in UVD firmware,
so subsequent decode submissions fail with -ENOENT. Since
amdgpu_uvd_free_handles() only reaps slots whose handle is non-zero, the
cleared slot also retains a stale filp pointer until reused.

Apply the decode arm's ownership test to the destroy arm. The kunmap
is hoisted above the loop, matching the create and decode arms, so
the new error return cannot leak the amdgpu_bo_kmap() reference.
Kernel-initiated teardown goes through amdgpu_uvd_send_msg() and never
runs the parser.

Fixes: 5146419e6feb ("drm/amdgpu: make UVD handle checking more strict")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index e8b0c62f72be..8d3e5435cf52 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -918,9 +918,19 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
 
 	case 2:
 		/* it's a destroy msg, free the handle */
-		for (i = 0; i < adev->uvd.max_handles; ++i)
-			atomic_cmpxchg(&adev->uvd.handles[i], handle, 0);
 		amdgpu_bo_kunmap(bo);
+
+		for (i = 0; i < adev->uvd.max_handles; ++i) {
+			if (atomic_read(&adev->uvd.handles[i]) != handle)
+				continue;
+
+			if (adev->uvd.filp[i] != ctx->parser->filp) {
+				DRM_ERROR("UVD handle collision detected!\n");
+				return -EINVAL;
+			}
+
+			atomic_cmpxchg(&adev->uvd.handles[i], handle, 0);
+		}
 		return 0;
 
 	default:

-- 
2.51.2

[PATCH 4/5] drm/amdgpu: enforce UVD handle ownership on destroy

[Junrui Luo]

amdgpu_uvd_cs_msg() validates that a decode message references a handle
owned by the submitting client, rejecting a mismatch between
adev->uvd.filp[i] and ctx->parser->filp. The handles[] and filp[] tables
are per-device and shared by every drm_file that opens the render node.

The destroy message performs no such check: it walks the whole table and
clears every slot matching the handle taken from the command stream
buffer. A client can therefore destroy a handle owned by another client,
clearing the victim's slot and tearing down its session in UVD firmware,
so subsequent decode submissions fail with -ENOENT. Since
amdgpu_uvd_free_handles() only reaps slots whose handle is non-zero, the
cleared slot also retains a stale filp pointer until reused.

Apply the decode arm's ownership test to the destroy arm. The kunmap
is hoisted above the loop, matching the create and decode arms, so
the new error return cannot leak the amdgpu_bo_kmap() reference.
Kernel-initiated teardown goes through amdgpu_uvd_send_msg() and never
runs the parser.

Fixes: 5146419e6feb ("drm/amdgpu: make UVD handle checking more strict")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index e8b0c62f72be..8d3e5435cf52 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -918,9 +918,19 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
 
 	case 2:
 		/* it's a destroy msg, free the handle */
-		for (i = 0; i < adev->uvd.max_handles; ++i)
-			atomic_cmpxchg(&adev->uvd.handles[i], handle, 0);
 		amdgpu_bo_kunmap(bo);
+
+		for (i = 0; i < adev->uvd.max_handles; ++i) {
+			if (atomic_read(&adev->uvd.handles[i]) != handle)
+				continue;
+
+			if (adev->uvd.filp[i] != ctx->parser->filp) {
+				DRM_ERROR("UVD handle collision detected!\n");
+				return -EINVAL;
+			}
+
+			atomic_cmpxchg(&adev->uvd.handles[i], handle, 0);
+		}
 		return 0;
 
 	default:

-- 
2.51.2

Re: [PATCH 4/5] drm/amdgpu: enforce UVD handle ownership on destroy

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] TOCTOU race condition in UVD destroy ownership check allows bypassing the security check.
- [High] Partial driver state modification during UVD destroy message validation leads to a permanent firmware resource leak.
- [Medium] Un-rate-limited DRM_ERROR in user-triggerable path allows kernel log flooding.

Pre-existing issues:
- [High] Flawed loop logic and lockless design in UVD handle creation allows clients to create duplicate handles and shadow existing sessions.
- [High] Integer overflow in amdgpu_uvd_cs_msg_decode allows bypassing buffer size validation, leading to out-of-bounds DMA writes by firmware.
--

--- Patch [4]: [PATCH 4/5] drm/amdgpu: enforce UVD handle ownership on destroy ---
commit 51bb2295b5cb365884456f0b29a575d54bdae4a7
Author: Junrui Luo <moonafterrain@outlook.com>
Subject: drm/amdgpu: enforce UVD handle ownership on destroy

This patch updates the UVD command submission to validate handle ownership
during destroy operations, similar to the decode path. It checks that the
file pointer associated with the handle matches the submitting client. The
mapping release is also hoisted above the loop to prevent reference leaks on
error.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> index e8b0c62f72be2..8d3e5435cf528 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> @@ -918,9 +918,19 @@ static int amdgpu_uvd_cs_msg(struct amdgpu_uvd_cs_ctx *ctx,
>  
>  	case 2:
>  		/* it's a destroy msg, free the handle */
> -		for (i = 0; i < adev->uvd.max_handles; ++i)
> -			atomic_cmpxchg(&adev->uvd.handles[i], handle, 0);
>  		amdgpu_bo_kunmap(bo);
> +
> +		for (i = 0; i < adev->uvd.max_handles; ++i) {
> +			if (atomic_read(&adev->uvd.handles[i]) != handle)
> +				continue;
> +
> +			if (adev->uvd.filp[i] != ctx->parser->filp) {

[Severity: High]
Does this check introduce a time-of-check to time-of-use race condition?

In the case 0 (create) path, the slot is claimed via atomic_cmpxchg before
filp[i] is updated:

drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c:amdgpu_uvd_cs_msg() {
    ...
    if (!atomic_cmpxchg(&adev->uvd.handles[i], 0, handle)) {
        adev->uvd.filp[i] = ctx->parser->filp;
        return 0;
    }
    ...
}

If a victim thread is preempted right after the atomic_cmpxchg but before
updating filp[i], filp[i] still retains its old value. If an attacker
concurrently submits a destroy command and their filp matches that old value,
can they pass this new ownership check and destroy the victim's newly allocated
session?

> +				DRM_ERROR("UVD handle collision detected!\n");

[Severity: Medium]
Is it possible for a user to flood the kernel log with this error message?

Since this path is entirely controlled by user inputs, an attacker could
repeatedly submit destroy messages for handles they do not own, triggering
this un-rate-limited DRM_ERROR in a tight loop.

> +				return -EINVAL;
> +			}
> +
> +			atomic_cmpxchg(&adev->uvd.handles[i], handle, 0);

[Severity: High]
Can returning -EINVAL here cause a permanent leak of UVD hardware sessions?

If multiple slots happen to contain the same handle, this loop will clear the
first matching slot using atomic_cmpxchg. When it reaches the second matching
slot (owned by a different client), the filp check fails and the function
returns -EINVAL.

This aborts the command submission before the destroy command is sent to the
firmware. However, because the first software slot is already zeroed out,
amdgpu_uvd_free_handles() will never clean it up, leaving the hardware
session leaked permanently.

> +		}
>  		return 0;
>  
>  	default:

[Severity: High]
This is a pre-existing issue, but does the loop logic in the case 0 (create)
path allow clients to create duplicate handles and shadow existing sessions?

drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c:amdgpu_uvd_cs_msg() {
    ...
    for (i = 0; i < adev->uvd.max_handles; ++i) {
        if (atomic_read(&adev->uvd.handles[i]) == handle) {
            ...
        }

        if (!atomic_cmpxchg(&adev->uvd.handles[i], 0, handle)) {
            adev->uvd.filp[i] = ctx->parser->filp;
            return 0;
        }
    }
    ...
}

The loop exits early upon finding a free slot, bypassing duplicate checks for
the remaining slots. If an attacker creates a handle that is already owned by
a victim, and the attacker obtains an earlier slot index, subsequent operations
by the victim will hit the attacker's slot first and fail the ownership check.

[Severity: High]
This is a pre-existing issue, but does the 32-bit multiplication of
user-controlled dimensions in amdgpu_uvd_cs_msg_decode() overflow?

drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c:amdgpu_uvd_cs_msg_decode() {
    ...
    image_size = (ALIGN(width, 16) * ALIGN(height, 16) * 3) / 2;
    ...
}

The width and height dimensions read from the UVD message do not seem to be
checked against upper bounds. By providing extremely large values, this
calculation can overflow 32-bit arithmetic to a very small value, bypassing
buffer size validation and allowing the firmware to perform out-of-bounds DMA
writes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=4

[PATCH 5/5] drm/amdgpu: free userptr HMM ranges on the CS error path

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

amdgpu_cs_parser_bos() allocates a struct amdgpu_hmm_range for every
userptr entry of the BO list and returns with them live. They are only
released in two places: the out_free_user_pages label in
amdgpu_cs_parser_bos() itself, and the invalidation check loop in
amdgpu_cs_submit().

Every error edge between those two points leaks. A failure in
amdgpu_cs_patch_jobs(), amdgpu_cs_vm_handling() or amdgpu_cs_sync_rings(),
or an early return from amdgpu_cs_submit() before its release loop, jumps
to error_fini and falls into amdgpu_cs_parser_fini(), which never walks
the BO list for userptr ranges. An IB address with no VM mapping is enough
to get there: amdgpu_cs_patch_ibs() returns the -EINVAL that
amdgpu_cs_find_mapping() hands back, so the leak is repeatable at will
from an unprivileged render node fd. Each leaked entry costs a struct
amdgpu_hmm_range plus its hmm_pfns array, a kvmalloc_array() of one entry
per page of the userptr mapping, allocated with plain GFP_KERNEL and so
not charged to the caller's memory cgroup.

Release the ranges in amdgpu_cs_parser_fini(), which every path out of
amdgpu_cs_ioctl() passes through. amdgpu_hmm_range_free() ignores a NULL
range, so the success path, where amdgpu_cs_submit() has already freed and
cleared them, is unaffected.

Fixes: fec8fdb54e8f ("drm/amdgpu: fix userptr HMM range handling v2")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 617f53f135f3..17c4fec21402 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1416,6 +1416,16 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
 				amdgpu_vm_bo_invalidate(bo, false);
 			}
 		}
+
+		/*
+		 * Release the ranges still live on the error paths;
+		 * amdgpu_cs_submit() already freed and cleared them when it
+		 * got far enough to check them for invalidation.
+		 */
+		amdgpu_bo_list_for_each_userptr_entry(e, parser->bo_list) {
+			amdgpu_hmm_range_free(e->range);
+			e->range = NULL;
+		}
 		amdgpu_bo_list_put(parser->bo_list);
 	}
 

-- 
2.51.2

[PATCH 5/5] drm/amdgpu: free userptr HMM ranges on the CS error path

[Junrui Luo]

amdgpu_cs_parser_bos() allocates a struct amdgpu_hmm_range for every
userptr entry of the BO list and returns with them live. They are only
released in two places: the out_free_user_pages label in
amdgpu_cs_parser_bos() itself, and the invalidation check loop in
amdgpu_cs_submit().

Every error edge between those two points leaks. A failure in
amdgpu_cs_patch_jobs(), amdgpu_cs_vm_handling() or amdgpu_cs_sync_rings(),
or an early return from amdgpu_cs_submit() before its release loop, jumps
to error_fini and falls into amdgpu_cs_parser_fini(), which never walks
the BO list for userptr ranges. An IB address with no VM mapping is enough
to get there: amdgpu_cs_patch_ibs() returns the -EINVAL that
amdgpu_cs_find_mapping() hands back, so the leak is repeatable at will
from an unprivileged render node fd. Each leaked entry costs a struct
amdgpu_hmm_range plus its hmm_pfns array, a kvmalloc_array() of one entry
per page of the userptr mapping, allocated with plain GFP_KERNEL and so
not charged to the caller's memory cgroup.

Release the ranges in amdgpu_cs_parser_fini(), which every path out of
amdgpu_cs_ioctl() passes through. amdgpu_hmm_range_free() ignores a NULL
range, so the success path, where amdgpu_cs_submit() has already freed and
cleared them, is unaffected.

Fixes: fec8fdb54e8f ("drm/amdgpu: fix userptr HMM range handling v2")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 617f53f135f3..17c4fec21402 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1416,6 +1416,16 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
 				amdgpu_vm_bo_invalidate(bo, false);
 			}
 		}
+
+		/*
+		 * Release the ranges still live on the error paths;
+		 * amdgpu_cs_submit() already freed and cleared them when it
+		 * got far enough to check them for invalidation.
+		 */
+		amdgpu_bo_list_for_each_userptr_entry(e, parser->bo_list) {
+			amdgpu_hmm_range_free(e->range);
+			e->range = NULL;
+		}
 		amdgpu_bo_list_put(parser->bo_list);
 	}
 

-- 
2.51.2

Re: [PATCH 5/5] drm/amdgpu: free userptr HMM ranges on the CS error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Data Race / Use-After-Free due to shared amdgpu_bo_list_entry
- [High] DRM scheduler API contract violation: aborting job after drm_sched_job_arm()
--

--- Patch [5]: [PATCH 5/5] drm/amdgpu: free userptr HMM ranges on the CS error path ---
commit c42f00dbd4932bbfbbb760f3bcc81040f9afbef4
Author: Junrui Luo <moonafterrain@outlook.com>

drm/amdgpu: free userptr HMM ranges on the CS error path

This commit fixes a memory leak in the amdgpu_cs ioctl error path by
releasing userptr HMM ranges in amdgpu_cs_parser_fini(). Previously, these
ranges were only freed on success or specific error paths, leading to leaks
when errors occurred between allocation and the release loop in
amdgpu_cs_submit().

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 617f53f135f35..17c4fec214028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1416,6 +1416,16 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser)
>  				amdgpu_vm_bo_invalidate(bo, false);
>  			}
>  		}
> +
> +		/*
> +		 * Release the ranges still live on the error paths;
> +		 * amdgpu_cs_submit() already freed and cleared them when it
> +		 * got far enough to check them for invalidation.
> +		 */
> +		amdgpu_bo_list_for_each_userptr_entry(e, parser->bo_list) {
> +			amdgpu_hmm_range_free(e->range);
> +			e->range = NULL;

[Severity: Critical]
This is a pre-existing issue, but does lockless modification of the shared
bo_list_entry introduce a data race or use-after-free here?

If userspace creates a BO list, obtains a handle, and invokes multiple
concurrent amdgpu_cs ioctls using the same BO list handle, the amdgpu_bo_list
objects are reference-counted and shared across those ioctls.

If one thread fails during the ioctl and reaches this newly modified
amdgpu_cs_parser_fini(), could it free e->range while another thread is
concurrently allocating or using it in amdgpu_cs_parser_bos()?

Thread 1:
amdgpu_cs_parser_bos() {
    ...
    e->range = amdgpu_hmm_range_alloc(NULL);
    ...
}

Thread 2 (on an error path):
amdgpu_cs_parser_fini() {
    ...
    amdgpu_bo_list_for_each_userptr_entry(e, parser->bo_list) {
        amdgpu_hmm_range_free(e->range);
        e->range = NULL;
    }
}

> +		}
>  		amdgpu_bo_list_put(parser->bo_list);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does amdgpu_cs_submit() violate
the DRM scheduler API contract by aborting jobs after they are armed?

Looking at amdgpu_cs_submit(), it arms jobs but can return an error before
pushing them:

amdgpu_cs_submit() {
    ...
    for (i = 0; i < p->gang_size; ++i)
        drm_sched_job_arm(&p->jobs[i]->base);
    ...
    if (r || !list_empty(&vm->individual.needs_update)) {
        r = -EAGAIN;
        mutex_unlock(&p->adev->notifier_lock);
        return r;
    }
    ...
}

If the function returns an error (e.g., if drm_sched_job_add_dependency() fails
or !list_empty(&vm->individual.needs_update) is true), the armed job is
aborted via drm_sched_job_cleanup() instead of being submitted.

Does calling drm_sched_job_cleanup() after drm_sched_job_arm() leave sequence
gaps on the scheduler entity timeline, potentially causing permanent GPU hangs
for subsequent jobs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=5