Re: [PATCH v2 08/33] drm/ttm: Bound page (re)allocation per defragmentation move

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] `ttm_pool_harvest_remaining()` fetches uninitialized `page->private` on tail pages when the defragmentation budget is exhausted, causing a NULL pointer dereference or buddy allocator corruption.
- [Critical] Double-free and Undefined Behavior in `ttm_tt_defrag_disown_borrowed()` when defragmenting pools that use `dma_alloc`.
- [Medium] Negative `defrag_bytes_remaining` disables the defragmentation budget instead of treating it as exhausted, breaking batch budget tracking.
--

--- Patch [8]: [PATCH v2 08/33] drm/ttm: Bound page (re)allocation per defragmentation move ---
commit 9447c53b431957799260b5fae41a105a050c3394
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/ttm: Bound page (re)allocation per defragmentation move

This commit bounds page reallocations per defragmentation move to avoid
stalling concurrent work for unbounded stretches when dealing with very large
objects. It introduces an optional per-move byte budget to allow a large
object to be upgraded in slices across successive moves.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 5d2bbcce4eb89..a6847a16c47f1 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
[ ... ]
> @@ -885,14 +892,33 @@ static int ttm_pool_harvest_page(struct ttm_tt *old_tt, unsigned int order,
>  
>  static int ttm_pool_harvest_remaining(struct ttm_pool *pool,
>  				      struct ttm_tt *old_tt, pgoff_t off,
> -				      struct ttm_pool_alloc_state *alloc)
> +				      struct ttm_pool_alloc_state *alloc,
> +				      bool *suboptimal)
>  {
> +	unsigned int beneficial = ttm_pool_beneficial_order(pool);
> +	pgoff_t bnr = 1UL << beneficial;
> +
>  	while (alloc->remaining_pages) {
>  		struct page *p = old_tt->pages[off];
>  		unsigned int order = ttm_pool_page_order(pool, p);

[Severity: Critical]
Does this code safely handle the case where off falls inside a multi-page
chunk rather than exactly on a chunk head?

During defragmentation, new_tt allocates chunks independently of old_tt's
chunk boundaries. When the budget is exhausted, ttm_pool_harvest_remaining()
is called with an off that may land on a tail page.

If this fetches a tail page, ttm_pool_page_order() will read its uninitialized
private field. For DMA pools, this uninitialized private field is cast to a
struct ttm_pool_dma pointer and dereferenced, causing a kernel panic. For
non-DMA pools, it evaluates to order 0, corrupting the chunk boundaries and
potentially leading to a double-free.

>  		pgoff_t nr = 1UL << order;
[ ... ]
> @@ -1106,10 +1172,23 @@ static int __ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
>  		gfp_flags |= GFP_HIGHUSER;
>  
>  	it.gfp_flags = gfp_flags;
> +	it.defrag_capped = it.defrag_old_tt && alloc->defrag_bytes_remaining > 0;

[Severity: Medium]
Will this properly cap the allocation if the budget is overdrawn and becomes
negative?

When the allocator decrements defrag_bytes_remaining by the size of the newly
allocated chunks, a chunk can exceed the exact remaining budget, causing the
value to drop below zero.

If this overdrawn negative budget is passed to the next BO in the batch,
alloc->defrag_bytes_remaining > 0 will evaluate to false. The allocator will
then treat the negative budget as unlimited rather than exhausted, bypassing
the cap entirely and stalling concurrent work.

>  
>  	for (it.order = ttm_pool_alloc_find_order(MAX_PAGE_ORDER, alloc);
[ ... ]

Additionally, looking at ttm_tt_defrag_disown_borrowed() in
drivers/gpu/drm/ttm/ttm_tt.c:

drivers/gpu/drm/ttm/ttm_tt.c:ttm_tt_defrag_disown_borrowed() {
    ...
    unsigned int order = sp ? ttm_pool_page_order_nodma(sp) : 0;
    pgoff_t nr = 1UL << order;
    ...
}

[Severity: Critical]
Is it safe to hardcode ttm_pool_page_order_nodma() here when defragmenting
pools that use dma_alloc?

Drivers that use dma_alloc pools (e.g., amdgpu, vmwgfx) store a pointer to a
struct ttm_pool_dma in the page->private field. Casting this pointer to an
integer order results in a huge bit shift, which destroys the array stride.
This silently fails to disown the shared pages, leading to an exploitable
use-after-free or double-free when the old tt is destroyed.

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