Re: [PATCH v3 07/33] drm/ttm: Harvest beneficial-order pages on defragmentation moves

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Defragmentation of DMA-alloc or external buffer objects causes out-of-bounds memory corruption.
- [Low] Comment falsely documents non-existent capping and prealloc logic.
--

--- Patch [7]: [PATCH v3 07/33] drm/ttm: Harvest beneficial-order pages on defragmentation moves ---
commit 6003b76a02bde0abed96778f39a94035b5bc6acd
Author: Matthew Brost <matthew.brost@intel.com>

drm/ttm: Harvest beneficial-order pages on defragmentation moves
    
A defragmentation move relocates a populated tt into a freshly allocated
one so that sub-beneficial-order chunks can be upgraded to the device's
beneficial order.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 2598d2f33767e..52f5e64f05ddc 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c

[ ... ]

> +static int ttm_pool_harvest_remaining(struct ttm_pool *pool,
> +				      struct ttm_tt *old_tt, pgoff_t off,
> +				      struct ttm_pool_alloc_state *alloc)
> +{
> +	/*
> +	 * @off is always an old-tt chunk head here, never a tail page, so
> +	 * reading the per-chunk order below is safe. This holds because:
> +	 *  - Old chunks are self-aligned: greedy largest-power-of-two fill
> +	 *    places an order-k chunk at a 2^k-aligned offset, so any
> +	 *    beneficial-order-aligned offset is a chunk boundary.
> +	 *  - Fresh defrag allocations are capped at the beneficial order (see
> +	 *    ttm_pool_iter_acquire_page()), so they advance @off in beneficial
> +	 *    steps and never overshoot into the middle of a larger old chunk;
> +	 *    super-beneficial old chunks are always harvested whole by
> +	 *    ttm_pool_iter_reuse_old().
> +	 *  - We are only reached once the defrag budget/prealloc is exhausted,
> +	 *    which in the capped/prealloc config leaves @off beneficial-aligned
> +	 *    (or, in the sub-beneficial tail, already walking old boundaries).
> +	 */

[Severity: Low]
Are the invariants described in this comment block referring to code that is 
missing from this series?

The documentation states that allocations are capped and references 
ttm_pool_iter_acquire_page(), but that function does not appear to contain any
capping logic.

The comment also refers to a defrag budget/prealloc exhaustion, but those
mechanisms do not seem to be present in the code.

> +	while (alloc->remaining_pages) {
> +		struct page *p = old_tt->pages[off];

[ ... ]

> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index b645a18181843..2388ff794a594 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c

[ ... ]

> +void ttm_tt_defrag_disown_borrowed(struct ttm_tt *tt, const struct ttm_tt *src)
> +{
> +	pgoff_t i = 0, run_start = 0;
> +	bool in_run = false;

[ ... ]

> +	while (i < tt->num_pages) {
> +		struct page *sp = src->pages[i];
> +		unsigned int order = sp ? ttm_pool_page_order_nodma(sp) : 0;
> +		pgoff_t nr = 1UL << order;

[Severity: Critical]
Can this lead to a massive out-of-bounds memset and memcpy when
defragmentation targets a dma-alloc pool or externally allocated buffer object?

If a defragmentation move is initiated in ttm_bo_handle_move_mem() on a 
buffer object using a dma-alloc pool or with TTM_TT_FLAG_EXTERNAL set, page 
private data often contains a kernel pointer or arbitrary foreign data 
instead of an allocation order.

Since ttm_pool_page_order_nodma() treats this data as an integer, the 
resulting order calculation would yield an astronomically large chunk size 
for nr. This size is then used in the memset operation immediately below, 
and in the memcpy operation within ttm_pool_harvest_page():

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_harvest_page() {
    ...
    memcpy(alloc->pages, &old_tt->pages[off], nr * sizeof(*alloc->pages));
    ...
}

Does ttm_bo_handle_move_mem() need validation to block defragmentation 
on these configurations?

> +
> +		if (sp && tt->pages[i] == sp) {
> +			if (!in_run) {
> +				run_start = i;
> +				in_run = true;
> +			}
> +		} else if (in_run) {
> +			memset(&tt->pages[run_start], 0,
> +			       (i - run_start) * sizeof(*tt->pages));

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