Re: [PATCH 08/10] drm/xe: remove workaround for TTM internals

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Locking `ttm_bo->base.resv` while unlocking `&ttm_bo->base._resv` causes deadlocks and kernel panics for objects with shared reservation locks.
- [High] Execution continues unlocked if `dma_resv_trylock()` fails, leading to unsafe fence modification and panics.
--

commit 4a41e0d0f5b4b25aaadf25f72a1d222898154844
Author: Christian König <christian.koenig@amd.com>

drm/xe: remove workaround for TTM internals

This commit removes a workaround for TTM internals in
xe_ttm_bo_release_notify(). It replaces the custom
xe_ttm_bo_lock_in_destructor() function with a direct call to
dma_resv_trylock().

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 5843f850339c7..34eae56716076 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c

[ ... ]

> @@ -1680,8 +1655,11 @@ static void xe_ttm_bo_release_notify(struct ttm_buffer_object *ttm_bo)
>  	bo = ttm_to_xe_bo(ttm_bo);
>  	xe_assert(xe_bo_device(bo), !(bo->created && kref_read(&ttm_bo->base.refcount)));
>  
> -	if (!xe_ttm_bo_lock_in_destructor(ttm_bo))
> -		return;
> +	/*
> +	 * This should never fail since there are no other references to the BO
> +	 * any more.
> +	 */
> +	WARN_ON(!dma_resv_trylock(ttm_bo->base.resv));

[Severity: Critical]
Does this cause a locking mismatch between ttm_bo->base.resv and
&ttm_bo->base._resv?

For imported dma-buf objects, ttm_bo->base.resv points to the exporter's
shared reservation lock, while &ttm_bo->base._resv points to the local unused
reservation object.

The trylock now uses ttm_bo->base.resv, but the subsequent fence replacement
loop and the unlock in the same function still operate on &ttm_bo->base._resv:

	dma_resv_for_each_fence(&cursor, &ttm_bo->base._resv,
				DMA_RESV_USAGE_BOOKKEEP, fence) {
		...
	}
	dma_fence_put(replacement);

	dma_resv_unlock(&ttm_bo->base._resv);

Can this lead to the shared exporter lock being successfully acquired but never
unlocked (causing a deadlock), followed by a call to unlock the unheld local
_resv ww_mutex (triggering an immediate kernel panic)?

[Severity: High]
Is it safe to continue execution unconditionally if dma_resv_trylock() fails?

The previous implementation safely returned early if the lock failed. The new
implementation wraps the trylock in a WARN_ON(), which only logs a warning and
does not abort control flow.

For imported BOs, the reservation lock is shared with the exporter and other
drivers, meaning the lock can legitimately be contended and the trylock can
fail. Will this allow execution to proceed unconditionally to modify fences
and unlock the reservation without actually holding the lock?

>  
>  	/*
>  	 * Scrub the preempt fences if any. The unbind fence is already

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-ttm_2_drm_exec-v1-0-43685ac1286b@gmx.de?part=8