Re: [PATCH v3 05/33] drm/ttm: Support defragmentation moves

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Data loss due to `ttm_bo_move_memcpy` and driver hardware copy routines ignoring `bo->defrag_old_tt`.
- [Critical] Asynchronous GPU copy Use-After-Free due to synchronous teardown on defrag move failure.
- [High] Victim BOs during multihop bounce moves are misclassified as defrag targets.
--

--- Patch [5]: [PATCH v3 05/33] drm/ttm: Support defragmentation moves ---
commit fba7dcbd46a60b9e14c15b0602b46ebb23a7bc37
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/ttm: Support defragmentation moves

This commit introduces infrastructure to re-back an already-resident buffer
object with fresh pages at a more beneficial order. It stashes the old
backing in a new defrag_old_tt pointer and creates a fresh one during the
move.

> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -125,49 +125,133 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>  {
>  	struct ttm_device *bdev = bo->bdev;
>  	bool old_use_tt, new_use_tt;
> +	bool defrag;
>  	int ret;
>  
>  	old_use_tt = !bo->resource || ttm_manager_type(bdev, bo->resource->mem_type)->use_tt;
>  	new_use_tt = ttm_manager_type(bdev, mem->mem_type)->use_tt;
>  
> -	ttm_bo_unmap_virtual(bo);
> +	/*
> +	 * Only the BO that is the actual defrag target (moved in place via
> +	 * ttm_bo_validate(), evict == false) is being defragmented. The same
> +	 * ctx->defrag is threaded into evictions triggered while allocating
> +	 * that target's new backing (ttm_bo_evict_alloc()), so guard against
> +	 * an unrelated victim BO (evict == true) being misclassified as a
> +	 * defrag move and needlessly reallocating its backing.
> +	 */
> +	defrag = ctx->defrag && !evict && new_use_tt && bo->ttm &&
> +		 ttm_tt_is_populated(bo->ttm);

[Severity: High]
Are victim buffer objects misclassified as defragmentation targets during
multihop bounce moves?

When a victim BO's eviction requires a multihop bounce, ttm_bo_evict()
calls ttm_bo_bounce_temp_buffer(), which subsequently calls
ttm_bo_handle_move_mem() with evict = false.

If ctx->defrag is true from the surrounding defragmentation pass, this
condition will evaluate to true for the bounce move.

Could this unnecessarily reallocate the victim's backing, and subsequently
trigger the WARN_ON_ONCE below if a bounce is requested?

[ ... ]

> +	/*
> +	 * A defrag move deferred its virtual unmap until the new backing was
> +	 * successfully populated; tear the userspace mappings down now, before
> +	 * the move swaps in the new pages, so the next fault re-establishes
> +	 * them against the new backing.
> +	 */
> +	if (defrag)
> +		ttm_bo_unmap_virtual(bo);
> +
>  	ret = bdev->funcs->move(bo, evict, ctx, mem, hop);

[Severity: Critical]
Does this result in data loss if the driver falls back to the generic copy
routines?

If a driver opts into defragmentation via ctx->defrag = true,
ttm_bo_handle_move_mem() allocates a new TT backing (bo->ttm) and stashes
the old data in bo->defrag_old_tt before calling the driver's move callback.

If the driver falls back to the generic ttm_bo_move_memcpy() or uses a
hardware copy that directly relies on bo->ttm:

drivers/gpu/drm/ttm/ttm_bo_util.c:ttm_bo_move_memcpy() {
    ...
    dst_iter = ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm);
    ...
    src_iter = ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm);
    ...
}

Won't both the source and destination initialize using the newly allocated
uninitialized bo->ttm, copying garbage onto itself, while the actual data
in bo->defrag_old_tt is ignored and eventually freed?

[ ... ]

>  out_err:
> -	if (!old_use_tt)
> +	if (bo->defrag_old_tt) {
> +		/* Failed defrag move: restore the original backing. */
> +		ttm_bo_tt_destroy(bo);

[Severity: Critical]
Can this synchronously free the backing memory while an asynchronous GPU copy
is still executing?

During a defrag move, the driver's move callback may launch an asynchronous
hardware copy to populate the new TT backing.

If a subsequent cleanup step fails (for example, ttm_bo_move_accel_cleanup()
returning -ENOMEM), the failure falls through to this out_err block.

If the memory is synchronously freed here using ttm_bo_tt_destroy(), won't
the GPU continue executing the copy into the now-freed system pages,
causing memory corruption?

> +		bo->ttm = bo->defrag_old_tt;
> +		bo->defrag_old_tt = NULL;
> +	} else if (!old_use_tt) {
>  		ttm_bo_tt_destroy(bo);
> +	}
>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711025619.2540575-1-matthew.brost@intel.com?part=5