Re: [PATCH v2] drm/amdkfd: don't leak BOs when process teardown can't unmap them

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