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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Defragmentation of DMA-allocated or external TTs causes massive out-of-bounds memory writes due to misinterpreting page private data as an allocation order.
- [Critical] Misaligned chunks between the old and new TT lead to partial chunk harvesting, resulting in a severe use-after-free.
- [High] If the driver's move callback returns `-EMULTIHOP`, the defragmentation rollback is bypassed, causing a permanent memory leak of the original buffer object TT.
--

--- Patch [7]: [PATCH v2 07/33] drm/ttm: Harvest beneficial-order pages on defragmentation moves ---
commit 74e91ed102adee47c82989ab7b4f83fd70da47c2
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. Previously, if allocation could not complete entirely
at beneficial order, the move failed and all progress was discarded.
Instead, once allocation has successfully upgraded part of the object,
the remainder of the old tt is harvested to complete the new tt. Thread
the BO's old, still-populated tt into the page allocator via a new
ttm_operation_ctx::defrag_old_tt.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -218,10 +225,18 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>  	if (defrag)
>  		ttm_bo_unmap_virtual(bo);
>  
>  	ret = bdev->funcs->move(bo, evict, ctx, mem, hop);
>  	if (ret) {
>  		if (ret == -EMULTIHOP)
>  			return ret;
>  		goto out_err;
>  	}
>  
>  	if (bo->defrag_old_tt) {

[Severity: High]
If bdev->funcs->move() returns -EMULTIHOP, does returning immediately bypass
the out_err rollback path?

It looks like the original TT remains stashed in bo->defrag_old_tt instead of
being restored.

When the move is retried, it appears bo->defrag_old_tt would be overwritten
or cleared, causing the original buffer object TT and its unborrowed pages
to be permanently leaked.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
[ ... ]
> +static int ttm_pool_iter_reuse_old(struct ttm_pool_alloc_iter *it)
> +{
> +	unsigned int harvest_order;
> +	pgoff_t off;
> +	int r;
> +
> +	if (!it->defrag_old_tt || !it->beneficial_order)
> +		return TTM_POOL_ITER_FILL;
> +
> +	off = it->tt->num_pages - it->alloc->remaining_pages;
> +	harvest_order = ttm_pool_defrag_old_order(it->pool, it->defrag_old_tt,
> +						  off, &it->old_cursor);
> +
> +	if (harvest_order < it->beneficial_order &&
> +	    it->order >= it->beneficial_order)
> +		return TTM_POOL_ITER_FILL;
> +
> +	it->order = harvest_order;
> +	r = ttm_pool_harvest_page(it->defrag_old_tt, it->order, off, it->alloc);

[Severity: Critical]
What happens if the new TT allocation size is unaligned, causing the offset
into the old TT to land in the middle of a large chunk?

Could ttm_pool_iter_reuse_old() or ttm_pool_harvest_remaining() harvest a
partial chunk, such as just the tail page?

If it harvests a tail page, the page order would be read as 0 since the
private data is only populated on the head page.

> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> --- 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 defragmentation be initiated on a buffer object that uses a DMA-allocated
or external TT?

Looking at ttm_bo_handle_move_mem(), it does not appear to restrict
defragmentation to non-DMA pools.

If a DMA-allocated TT is defragmented, wouldn't ttm_pool_page_order_nodma()
incorrectly read the DMA pointer stored in the page private data as an
integer order?

This would result in a huge bitshift when calculating nr, which is then
used in the loop increment or the memset() calls, potentially causing severe
out-of-bounds memory writes.

> +		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));

[Severity: Critical]
Following up on the potential for harvesting an unaligned tail page (noted in
ttm_pool.c), if ttm_pool_iter_reuse_old() harvested a tail page, wouldn't
this logic clear the pointer in the old TT for that tail page, but leave the
head page pointer intact?

When the old TT is eventually destroyed, wouldn't ttm_pool_free_range() free
the entire chunk back to the system (since the head page still indicates a
large order) while the tail page is still actively mapped and used by the new
TT, leading to a severe use-after-free regression?

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