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

[Bocaj Gnuoy]

Correcting a factual error in my own changelog before anyone spends
review time on it.

The changelog states:

  "Note the free path itself does not allocate - it reserves the BO,
   detaches the attachments and drops the references - so it can
   complete even when the unmap could not."

The first clause is wrong. Immediately after the -EBUSY bypass,
amdgpu_amdkfd_gpuvm_free_memory_of_gpu() calls:

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

reserve_bo_and_cond_vms() uses drm_exec_prepare_obj(), and drm_exec
allocates its object array with kvmalloc_array()/kvrealloc(GFP_KERNEL)
and returns -ENOMEM on failure; drm_exec_prepare_obj() also calls
dma_resv_reserve_fences(). So the free path can allocate, and can fail
with -ENOMEM.

Bounded consequence
-------------------

The forced free can therefore still fail, after which
kfd_process_device_free_bos() drops the idr handle regardless - the
same ownership violation the patch addresses, one level deeper.

This is not detectable through the teardown WARNs. The BO is removed
from process_info's lists before the reservation is attempted:

	/* Make sure restore workers don't access the BO any more */
	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;

That ordering is deliberate, so reordering it is not a fix. The
consequence is that if the reservation fails, the attachments are
never detached and the free does not complete, but the lists that
amdgpu_amdkfd_gpuvm_destroy_cb() checks are already empty. The stale
lifetime condition implicated in the deadlock can therefore survive
without the WARNs firing. I have not driven that particular residual
failure through the eviction path and observed the deadlock, so I am
describing a reachable state, not a reproduced one.

Scope of the change
-------------------

The underlying violation is not introduced by this patch. Upstream
today can already reach it:

  mapped_to_gpu_memory == 0
    -> delist
    -> reservation fails
    -> free returns error
    -> teardown discards the handle

This patch additionally permits:

  mapped_to_gpu_memory > 0 after a failed unmap
    -> bypass -EBUSY
    -> delist
    -> reservation fails
    -> free returns error
    -> teardown discards the handle

So it widens the set of states that can reach the violation rather
than creating it. That is not offered as a justification, only as
scope.

What the testing does and does not show
---------------------------------------

The pr_err() this patch adds to kfd_process_device_free_bos() is the
only instrumentation in the patch that directly observes a
post-delisting free failure. It did not fire in any run, including
one with 1904 "failed to validate PT BOs" and 35 forced frees.

That is consistent with the drm_exec allocations being far smaller
than the page-table validation that failed, and therefore satisfiable
under the same pressure. It is a probability argument, not an
invariant.

The accurate evidentiary statement is: the fix reliably completed in
the tested pressure regime, but the implementation does not provide an
invariant guaranteeing teardown completion under arbitrary allocation
failure.

What this does and does not change
----------------------------------

The reproduced failure is unaffected: the unmap fails in
vm_validate_pt_pd_bos(), mapped_to_gpu_memory stays non-zero, the free
returns -EBUSY before list_del_init(&mem->validate_list), and the BO
is stranded. This patch removes that, and in every observed forced
teardown the cleanup completed and the machine-killing relaunch
stopped happening.

What the error invalidates is the completeness claim, not the safety
argument for forcing. Bypassing -EBUSY on irreversible teardown and
detaching the bo_vas when the reservation succeeds is still supported
by the evidence. What I can no longer claim is that the cleanup is
guaranteed to succeed merely because the failing page-table operation
was skipped.

v3 will correct the changelog and state this as an explicit
limitation. If the residual allocation failure should be repaired
rather than documented, there appear to be several possible
directions with different locking and lifetime implications - a
retry, a reservation path that does not allocate, or deferring
ownership so a later pass can free the BO, among others. I would
rather have maintainer guidance on which is wanted than guess at a
lifetime-ownership decision.

Thanks,
Bocaj