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 <[email protected]> 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 <[email protected]> > 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/[email protected]?part=1
