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

[sashiko-bot@kernel.org]

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