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

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

Pre-existing issues:
- [High] The patch misses applying force=true to the error unwind path of kfd_process_alloc_gpuvm(), which can permanently leak internal BOs on unmap failure.
- [High] Interruptible lock waiting in reserve_bo_and_cond_vms causes early return on pending signals, bypassing the forced free and silently leaking the BO.
--

--- Patch [1]: [PATCH] drm/amdkfd: don't leak BOs when process teardown can't unmap them ---
commit 01e590b61624d149d5da9ac22540c56bed4f9a55
Author: Bocaj Gnuoy <[email protected]>
Subject: drm/amdkfd: don't leak BOs when process teardown can't unmap them

This commit addresses a memory leak and subsequent TTM eviction deadlock
that occurs when a process runs out of memory during teardown. It adds a
force flag to amdgpu_amdkfd_gpuvm_free_memory_of_gpu() to ensure BOs are
freed even if unmapping fails. The early -EBUSY return is retained for ioctl
paths where userspace can retry.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5672

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 1e71829e0fc6b..dc1fa664fecac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> [ ... ]
> @@ -1922,9 +1922,26 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>  	 */
>  
>  	if (mapped_to_gpu_memory > 0) {
> -		pr_debug("BO VA 0x%llx size 0x%lx is still mapped.\n",
> -				mem->va, bo_size);
> -		return -EBUSY;
> +		/*
> +		 * Refusing to free a mapped BO is only meaningful while the
> +		 * process can still unmap it. On process teardown (@force)
> +		 * there is no such chance: the caller drops the last handle
> +		 * to @mem regardless, so bailing out here leaks the BO onto
> +		 * process_info->kfd_bo_list / userptr_inval_list. Those lists
> +		 * are then destroyed non-empty in
> +		 * amdgpu_amdkfd_gpuvm_destroy_cb(), leaving a BO in TTM's
> +		 * eviction LRU whose bo_vas point into the freed amdgpu_vm.
> +		 * The next client to trigger eviction deadlocks in
> +		 * amdgpu_vm_bo_move(). Tear the mappings down instead - the
> +		 * VM is going away right after us anyway.
> +		 */
> +		if (!force) {
> +			pr_debug("BO VA 0x%llx size 0x%lx is still mapped.\n",
> +				 mem->va, bo_size);
> +			return -EBUSY;
> +		}
> +		pr_warn("Force-freeing BO VA 0x%llx size 0x%lx still mapped %u time(s)\n",
> +			mem->va, bo_size, mapped_to_gpu_memory);
>  	}
>  
>  	/* At this point the BO is guaranteed to be freed, so unpin the

[Severity: High]
This is a pre-existing issue, but can amdgpu_amdkfd_gpuvm_free_memory_of_gpu()
still silently leak the BO if a signal interrupts the lock wait?

Later in this function, the BO is removed from the tracking list before an
interruptible lock wait is performed:

amdgpu_amdkfd_gpuvm_free_memory_of_gpu()
    mutex_lock(&process_info->lock);
    if (!list_empty(&mem->validate_list))
        list_del_init(&mem->validate_list);
    mutex_unlock(&process_info->lock);

    ret = reserve_bo_and_cond_vms(mem, NULL, BO_VM_ALL, &ctx);
    if (unlikely(ret))
        return ret;

If reserve_bo_and_cond_vms() returns early (e.g., -ERESTARTSYS) due to a
pending signal, the function bails out. Because the BO was already removed
from validate_list, it is left orphaned.

Additionally, looking at the related caller kfd_process_free_gpuvm_map(),
the map pointer is cleared before the free is attempted:

kfd_process_free_gpuvm_map()
    if (map && !iosys_map_is_null(map)) {
        amdgpu_amdkfd_gpuvm_unmap_bo_from_kernel(mem);
        iosys_map_clear(map);
    }

If the free is aborted by a signal, subsequent teardown passes will skip
this map and ignore this BO. Does this sequence also create a risk of a
silent memory leak and TTM eviction deadlock when userspace triggers an
error and receives a concurrent signal?

> [ ... ]
> 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);
>  err_alloc_mem:
>  	*mem = NULL;
>  	*kptr = NULL;

[Severity: High]
This is a pre-existing issue, but should the error unwind path in
kfd_process_alloc_gpuvm() use force = true?

When kfd_process_alloc_gpuvm() hits an error and jumps to err_map_mem, it
calls amdgpu_amdkfd_gpuvm_free_memory_of_gpu() with force = false. It then
immediately zeroes out the *mem pointer.

If the free fails (for instance, an unmap failure under memory exhaustion
where force = false causes an -EBUSY return), this BO appears to be
permanently leaked since *mem is cleared and no later retry is possible.

Could this lead to the same unrecoverable TTM eviction deadlock that this
patch is fixing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1