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 <[email protected]> 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/[email protected]?part=5
