Re: [PATCH v2] drm/amdgpu: fix recursive ww_mutex acquire in amdgpu_devcoredump_format

[Christian König <christian.koenig@amd.com>]

On 5/19/26 18:15, Mikhail Gavrilov wrote:
> When dumping IB contents from a hung job, amdgpu_devcoredump_format()
> acquires the VM root PD's reservation lock via amdgpu_vm_lock_by_pasid()
> and then, for each IB referenced by the job, calls amdgpu_bo_reserve()
> on the BO that backs the IB.  Both reservations are taken on
> reservation_ww_class_mutex objects but neither uses a ww_acquire_ctx,
> which trips lockdep:
> 
>   WARNING: possible recursive locking detected
>   --------------------------------------------
>   kworker/u128:0 is trying to acquire lock:
>   ffff88838b16e1f0 (reservation_ww_class_mutex){+.+.}-{4:4},
>     at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]
> 
>   but task is already holding lock:
>   ffff8882f82681f0 (reservation_ww_class_mutex){+.+.}-{4:4},
>     at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]
> 
>    Possible unsafe locking scenario:
>          CPU0
>          ----
>     lock(reservation_ww_class_mutex);
>     lock(reservation_ww_class_mutex);
> 
>    *** DEADLOCK ***
>    May be due to missing lock nesting notation
> 
>   Workqueue: events_unbound amdgpu_devcoredump_deferred_work [amdgpu]
>   Call Trace:
>    __ww_mutex_lock.constprop.0
>    ww_mutex_lock
>    amdgpu_bo_reserve
>    amdgpu_devcoredump_format+0x1594 [amdgpu]
>    amdgpu_devcoredump_deferred_work+0xea [amdgpu]
> 
> The two reservations are on different BOs in the captured trace, so the
> splat is a lockdep-correctness warning, not an observed deadlock.  It
> becomes a real self-deadlock whenever the IB BO shares its dma_resv
> with the root PD (the always-valid case, see
> amdgpu_vm_is_bo_always_valid()): amdgpu_bo_reserve(abo) re-acquires the
> same ww_mutex without a ticket and blocks forever.
> 
> Fix it in two steps:
> 
> 1. Collect per-IB BO references under the root PD's reservation, then
>    release the root before locking the IB BOs.  The walk over the VM
>    mapping tree must remain under the root lock (mappings can be torn
>    down without it), but the actual content copies do not.
> 
> 2. Lock all the IB BOs together using drm_exec(9) with a single
>    ww_acquire_ctx.  DRM_EXEC_IGNORE_DUPLICATES handles the case where
>    IB BOs share a dma_resv (e.g. always-valid BOs).  Each lock attempt
>    is now a top-level acquire under one ticket, with retry-on-
>    contention handled by drm_exec; the recursive ww_mutex condition
>    is gone.
> 
> The collect/lock/release logic is factored out into three small helpers
> (amdgpu_devcoredump_{collect,lock,release}_ib_refs) to keep the main
> function readable and within the kernel coding style indentation
> guideline.
> 
> This also fixes a BO refcount leak in the original code: when
> amdgpu_bo_reserve() failed, control jumped to free_ib_content without
> running amdgpu_bo_unref().  In the new structure the per-IB BO refs
> are released unconditionally in the cleanup helper.
> 
> Reproducer (~150 LoC libdrm_amdgpu): submit a single GFX IB containing
> PACKET3_INDIRECT_BUFFER chained at GPU VA 0 and wait for the fence.
> The TDR fires within ~10 s and the deferred coredump worker produces
> the splat above on every invocation.
> 
> v2: switch from per-IB amdgpu_bo_reserve() to drm_exec for the IB BO
>     locking as suggested by Christian König; the snapshot approach
>     for collecting BO references under the root PD's reservation is
>     retained.
> 
> Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump")
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c  | 187 ++++++++++++++----
>  1 file changed, 148 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> index d386bc775d03..9ac958cf09fd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> @@ -24,6 +24,7 @@
>  
>  #include <generated/utsrelease.h>
>  #include <linux/devcoredump.h>
> +#include <drm/drm_exec.h>
>  #include "amdgpu_dev_coredump.h"
>  #include "atom.h"
>  
> @@ -207,6 +208,108 @@ static void amdgpu_devcoredump_fw_info(struct amdgpu_device *adev,
>  	}
>  }
>  
> +struct amdgpu_devcoredump_ib_ref {
> +	struct amdgpu_bo	*bo;
> +	u64			offset;
> +};

> +
> +/*
> + * Walk the VM's mapping tree under the root PD's reservation to obtain the BO
> + * that backs each IB and pin it with a refcount. The root PD reservation is
> + * dropped before this function returns; the caller can then lock each IB BO
> + * via drm_exec without nesting reservations on reservation_ww_class_mutex.
> + *
> + * Returns an array of num_ibs entries (each ib_refs[i].bo may be NULL if its
> + * mapping was not found), or NULL on allocation failure / VM lookup failure.
> + * The caller must release the BO refs and free the array via
> + * amdgpu_devcoredump_release_ib_refs().
> + */
> +static struct amdgpu_devcoredump_ib_ref *
> +amdgpu_devcoredump_collect_ib_refs(struct amdgpu_device *adev,
> +				   struct amdgpu_coredump_info *coredump)
> +{
> +	struct amdgpu_devcoredump_ib_ref *ib_refs;
> +	struct amdgpu_bo_va_mapping *mapping;
> +	struct amdgpu_bo *root;
> +	struct amdgpu_vm *vm;
> +	u64 va_start;
> +
> +	ib_refs = kcalloc(coredump->num_ibs, sizeof(*ib_refs), GFP_KERNEL);
> +	if (!ib_refs)
> +		return NULL;
> +
> +	vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid);
> +	if (!vm) {
> +		kfree(ib_refs);
> +		return NULL;
> +	}
> +
> +	for (int i = 0; i < coredump->num_ibs; i++) {
> +		va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK;
> +		mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE);
> +		if (!mapping)
> +			continue;
> +
> +		ib_refs[i].bo = amdgpu_bo_ref(mapping->bo_va->base.bo);
> +		ib_refs[i].offset = va_start -
> +				    mapping->start * AMDGPU_GPU_PAGE_SIZE;
> +	}
> +
> +	amdgpu_bo_unreserve(root);
> +	amdgpu_bo_unref(&root);
> +
> +	return ib_refs;
> +}


That whole infrastructure is superflous. You just need to modify amdgpu_vm_lock_by_pasid() to take a drm_exec object to lock the root BO.

Regards,
Christian.


> +
> +static void
> +amdgpu_devcoredump_release_ib_refs(struct amdgpu_devcoredump_ib_ref *ib_refs,
> +				   int num_ibs)
> +{
> +	if (!ib_refs)
> +		return;
> +
> +	for (int i = 0; i < num_ibs; i++)
> +		if (ib_refs[i].bo)
> +			amdgpu_bo_unref(&ib_refs[i].bo);
> +	kfree(ib_refs);
> +}
> +
> +/*
> + * Lock all collected IB BOs together using a single drm_exec ticket. This
> + * eliminates the nested ww_mutex acquire that lockdep flags as recursive
> + * locking (and that becomes a real self-deadlock for IB BOs sharing their
> + * dma_resv with the root PD).
> + *
> + * Returns 0 if drm_exec was initialised and the BOs are locked; the caller
> + * must call drm_exec_fini() on success. Returns non-zero on failure, in which
> + * case drm_exec is already torn down.
> + */
> +static int
> +amdgpu_devcoredump_lock_ib_refs(struct drm_exec *exec,
> +				struct amdgpu_devcoredump_ib_ref *ib_refs,
> +				int num_ibs)
> +{
> +	int r = 0;
> +
> +	drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, num_ibs);
> +	drm_exec_until_all_locked(exec) {
> +		r = 0;
> +		for (int i = 0; i < num_ibs; i++) {
> +			if (!ib_refs[i].bo)
> +				continue;
> +			r = drm_exec_lock_obj(exec, &ib_refs[i].bo->tbo.base);
> +			drm_exec_retry_on_contention(exec);
> +			if (r)
> +				break;
> +		}
> +		if (r)
> +			break;
> +	}
> +	if (r)
> +		drm_exec_fini(exec);
> +	return r;
> +}
> +
>  static ssize_t
>  amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_info *coredump)
>  {
> @@ -214,13 +317,9 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf
>  	struct drm_printer p;
>  	struct drm_print_iterator iter;
>  	struct amdgpu_vm_fault_info *fault_info;
> -	struct amdgpu_bo_va_mapping *mapping;
>  	struct amdgpu_ip_block *ip_block;
>  	struct amdgpu_res_cursor cursor;
> -	struct amdgpu_bo *abo, *root;
> -	uint64_t va_start, offset;
>  	struct amdgpu_ring *ring;
> -	struct amdgpu_vm *vm;
>  	u32 *ib_content;
>  	uint8_t *kptr;
>  	int ver, i, j, r;
> @@ -343,43 +442,52 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf
>  		drm_printf(&p, "VRAM is lost due to GPU reset!\n");
>  
>  	if (coredump->num_ibs) {
> -		/* Don't try to lookup the VM or map the BOs when calculating the
> -		 * size required to store the devcoredump.
> +		struct amdgpu_devcoredump_ib_ref *ib_refs = NULL;
> +		struct drm_exec exec;
> +		bool ibs_locked = false;
> +
> +		/*
> +		 * Collect the BO that backs each IB under the root PD's
> +		 * reservation, drop the root reservation, then lock all the
> +		 * IB BOs together in one drm_exec ticket. This avoids nesting
> +		 * amdgpu_bo_reserve() inside the root PD's reservation, which
> +		 * would be a recursive reservation_ww_class_mutex acquire
> +		 * without a ww_acquire_ctx (lockdep splat, and a real
> +		 * self-deadlock for always-valid BOs that share their dma_resv
> +		 * with the root PD).
> +		 *
> +		 * Skip lookup/locking entirely on the sizing pass: it does not
> +		 * write IB content, and the size estimate doesn't depend on
> +		 * whether the BOs are reachable.
>  		 */
> -		if (sizing_pass)
> -			vm = NULL;
> -		else
> -			vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid);
> +		if (!sizing_pass) {
> +			ib_refs = amdgpu_devcoredump_collect_ib_refs(adev, coredump);
> +			if (ib_refs) {
> +				r = amdgpu_devcoredump_lock_ib_refs(&exec, ib_refs,
> +								    coredump->num_ibs);
> +				if (!r)
> +					ibs_locked = true;
> +			}
> +		}
> +
> +		for (int i = 0; i < coredump->num_ibs; i++) {
> +			struct amdgpu_bo *abo = ibs_locked ? ib_refs[i].bo : NULL;
> +			u64 offset = ibs_locked ? ib_refs[i].offset : 0;
> +			bool emit_content = sizing_pass;
>  
> -		for (int i = 0; i < coredump->num_ibs && (sizing_pass || vm); i++) {
>  			ib_content = kvmalloc_array(coredump->ibs[i].ib_size_dw, 4,
>  						    GFP_KERNEL);
>  			if (!ib_content)
>  				continue;
>  
> -			/* vm=NULL can only happen when 'sizing_pass' is true. Skip to the
> -			 * drm_printf() calls (ib_content doesn't need to be initialized
> -			 * as its content won't be written anywhere).
> -			 */
> -			if (!vm)
> +			if (!abo)
>  				goto output_ib_content;
>  
> -			va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK;
> -			mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE);
> -			if (!mapping)
> -				goto free_ib_content;
> -
> -			offset = va_start - (mapping->start * AMDGPU_GPU_PAGE_SIZE);
> -			abo = amdgpu_bo_ref(mapping->bo_va->base.bo);
> -			r = amdgpu_bo_reserve(abo, false);
> -			if (r)
> -				goto free_ib_content;
> -
>  			if (abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) {
>  				off = 0;
>  
>  				if (abo->tbo.resource->mem_type != TTM_PL_VRAM)
> -					goto unreserve_abo;
> +					goto output_ib_content;
>  
>  				amdgpu_res_first(abo->tbo.resource, offset,
>  						 coredump->ibs[i].ib_size_dw * 4,
> @@ -391,12 +499,13 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf
>  					off += cursor.size;
>  					amdgpu_res_next(&cursor, cursor.size);
>  				}
> +				emit_content = true;
>  			} else {
>  				r = ttm_bo_kmap(&abo->tbo, 0,
>  						PFN_UP(abo->tbo.base.size),
>  						&abo->kmap);
>  				if (r)
> -					goto unreserve_abo;
> +					goto output_ib_content;
>  
>  				kptr = amdgpu_bo_kptr(abo);
>  				kptr += offset;
> @@ -404,23 +513,23 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf
>  				       coredump->ibs[i].ib_size_dw * 4);
>  
>  				amdgpu_bo_kunmap(abo);
> +				emit_content = true;
>  			}
>  
>  output_ib_content:
>  			drm_printf(&p, "\nIB #%d 0x%llx %d dw\n",
>  				   i, coredump->ibs[i].gpu_addr, coredump->ibs[i].ib_size_dw);
> -			for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
> -				drm_printf(&p, "0x%08x\n", ib_content[j]);
> -unreserve_abo:
> -			if (vm)
> -				amdgpu_bo_unreserve(abo);
> -free_ib_content:
> +			if (emit_content) {
> +				for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
> +					drm_printf(&p, "0x%08x\n", ib_content[j]);
> +			}
>  			kvfree(ib_content);
>  		}
> -		if (vm) {
> -			amdgpu_bo_unreserve(root);
> -			amdgpu_bo_unref(&root);
> -		}
> +
> +		if (ibs_locked)
> +			drm_exec_fini(&exec);
> +
> +		amdgpu_devcoredump_release_ib_refs(ib_refs, coredump->num_ibs);
>  	}
>  
>  	return count - iter.remain;