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

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

On 5/21/26 17:08, Mikhail Gavrilov wrote:
> When dumping IB contents from a hung job, amdgpu_devcoredump_format()
> acquired the VM root PD's reservation via amdgpu_vm_lock_by_pasid() and
> then, for each IB, called amdgpu_bo_reserve() on the BO backing the IB.
> Both reservations are reservation_ww_class_mutex objects and neither
> used 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.
> 
> With amdgpu.gpu_recovery=0 the timeout handler refires every ~2 s and
> each invocation produces this splat, drowning the kernel ring buffer.
> 
> Now that amdgpu_vm_lock_by_pasid() takes a drm_exec context, lock the
> root PD and every IB BO together in a single drm_exec ticket.
> DRM_EXEC_IGNORE_DUPLICATES handles IB BOs that share a dma_resv (e.g.
> always-valid BOs, or two IBs backed by the same BO). Every lock is now
> a top-level acquire under one ww_acquire_ctx, so the recursive ww_mutex
> condition is gone, and the per-IB amdgpu_bo_reserve()/amdgpu_bo_unref()
> dance -- including a BO refcount leak on the amdgpu_bo_reserve() failure
> path -- is removed.
> 
> 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; with this change applied the splat is
> gone.

That commit message is a bit to long. It should describe the problem and the solution and not necessary how to reproduce it.

> 
> 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  | 105 ++++++++++++------
>  1 file changed, 71 insertions(+), 34 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..456ea9911d48 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"
>  
> @@ -214,13 +215,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 +340,84 @@ 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_bo_va_mapping *mapping;
> +		struct amdgpu_bo *abo;
> +		struct drm_exec exec;
> +		struct amdgpu_vm *vm;
> +		u64 va_start, offset;

It's probably a good idea to put the IB dumping into a separate function.

> +		bool locked = false;

Drop that variable and handling, it is superflous.

> +
> +		/*
> +		 * Lock the VM root PD and every IB BO together in a single
> +		 * drm_exec ticket. Reserving the IB BOs one by one while the
> +		 * root PD is held would be a recursive reservation_ww_class_mutex
> +		 * acquire without a ww_acquire_ctx, which trips lockdep and
> +		 * self-deadlocks for IB BOs that share their dma_resv with the
> +		 * root PD (always-valid BOs).
> +		 *
> +		 * Skip locking entirely on the sizing pass: it does not write
> +		 * IB content, so 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) {
> +			drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES,
> +				      1 + coredump->num_ibs);
> +			drm_exec_until_all_locked(&exec) {
> +				vm = amdgpu_vm_lock_by_pasid(adev, coredump->pasid,
> +							     &exec);
> +				drm_exec_retry_on_contention(&exec);
> +				if (!vm)
> +					break;

This should use goto error handling, when we can't find the VM we should abort here.

> +
> +				for (int i = 0; i < coredump->num_ibs; i++) {
> +					u64 pfn;
> +
> +					va_start = coredump->ibs[i].gpu_addr &
> +						   AMDGPU_GMC_HOLE_MASK;
> +					pfn = va_start / AMDGPU_GPU_PAGE_SIZE;
> +					mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn);
> +					if (!mapping)
> +						continue;

That's also an error, it could be that we just want to print the IB start address in that case.

> +
> +					abo = mapping->bo_va->base.bo;
> +					r = drm_exec_lock_obj(&exec, &abo->tbo.base);
> +					drm_exec_retry_on_contention(&exec);
> +					if (r)
> +						break;

Dito 

> +				}
> +				if (r)
> +					break;

And here as well.

> +			}
> +			if (vm && !r)
> +				locked = true;
> +			else
> +				drm_exec_fini(&exec);

Don't call drm_exec_fini() here.

Regards,
Christian.

> +		}
> +
> +		for (int i = 0; i < coredump->num_ibs; i++) {
> +			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 (!locked)
>  				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;
> +				goto output_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;
> +			abo = mapping->bo_va->base.bo;
> +			offset = va_start - mapping->start * AMDGPU_GPU_PAGE_SIZE;
>  
>  			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 +429,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 +443,21 @@ 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 (locked)
> +			drm_exec_fini(&exec);
>  	}
>  
>  	return count - iter.remain;