Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Internal BO allocations (like IB and CWSR) are leaked during error unwinding because `kfd_process_alloc_gpuvm` uses `force = false` and permanently loses the reference. - [High] The force-free path can still fail under memory pressure due to `GFP_KERNEL` allocations, silently bypassing teardown WARNs and causing the same Use-After-Free deadlock. - [High] `amdgpu_amdkfd_gpuvm_free_memory_of_gpu` aborts early and leaks BOs if a signal is pending during error unwinding. -- --- Patch [1]: [PATCH v2] drm/amdkfd: don't leak BOs when process teardown can't unmap them --- commit d455d7ae97544007b84e3463aa9bc5371a4a8935 Author: Bocaj Gnuoy <[email protected]> drm/amdkfd: don't leak BOs when process teardown can't unmap them This commit fixes a memory leak and subsequent TTM eviction deadlock that occurs when BO unmapping fails during process teardown due to memory exhaustion. It adds a force flag to amdgpu_amdkfd_gpuvm_free_memory_of_gpu() to bypass the mapping check and detach attachments instead of aborting. Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5672 > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index 0a7c1900da959..6d5126aa6fe74 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c [ ... ] > @@ -814,7 +814,7 @@ static int kfd_process_alloc_gpuvm(struct > kfd_process_device *pdd, > > err_map_mem: > amdgpu_amdkfd_gpuvm_free_memory_of_gpu(kdev->adev, *mem, pdd->drm_priv, > - NULL); > + NULL, false); [Severity: High] This isn't a bug introduced by this patch, but does this error unwinding path leak internal BO allocations? If amdgpu_amdkfd_gpuvm_free_memory_of_gpu() fails here due to the memory being exhausted during unmapping (similar to the user-space BOs), the force = false flag means it will safely return -EBUSY but abort the cleanup. Since the pointers are then unconditionally cleared: err_alloc_mem: *mem = NULL; *kptr = NULL; Does the reference to the BO get permanently lost, causing the exact same use-after-free sequence this patch is addressing? > err_alloc_mem: > *mem = NULL; > *kptr = NULL; [ ... ] > @@ -1119,18 +1119,29 @@ static void kfd_process_device_free_bos(struct > kfd_process_device *pdd) [ ... ] > } > > - amdgpu_amdkfd_gpuvm_free_memory_of_gpu(pdd->dev->adev, mem, > - pdd->drm_priv, NULL); > + r = amdgpu_amdkfd_gpuvm_free_memory_of_gpu(pdd->dev->adev, mem, > + pdd->drm_priv, NULL, > + true); > + if (r) > + pr_err("Failed to free BO on process teardown: %d\n", > r); [Severity: High] This is a pre-existing issue, but does this still leak BOs and cause a TTM eviction deadlock if amdgpu_amdkfd_gpuvm_free_memory_of_gpu() fails for other reasons? Looking at amdgpu_amdkfd_gpuvm_free_memory_of_gpu(), it can still fail during reservation: ret = reserve_bo_and_cond_vms(mem, NULL, BO_VM_ALL, &ctx); if (unlikely(ret)) return ret; This reservation can fail under memory exhaustion if the GFP_KERNEL allocations in drm_exec_init() fail. Since the caller drops the IDR handle regardless of the error, will the dangling bo_va pointers still poison the eviction LRU? [Severity: High] This is also a pre-existing issue, but what happens if a signal is pending during process teardown? The same reservation path in reserve_bo_and_cond_vms() uses an interruptible wait: drm_exec_init(&ctx->exec, DRM_EXEC_INTERRUPTIBLE_WAIT | DRM_EXEC_IGNORE_DUPLICATES, 0); If a signal (e.g. SIGKILL) is pending, this fails with -ERESTARTSYS. Does this cause amdgpu_amdkfd_gpuvm_free_memory_of_gpu() to skip the remaining cleanup and leak the attachments, leading to the same use-after-free sequence when the handle is dropped? > kfd_process_device_remove_obj_handle(pdd, id); > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
