Re: [PATCH] drm/ttm: Account for NULL pages in ttm_pool_backup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing HighMem check causes NULL pointer dereference in CPA subsystem on 32-bit x86.
- [High] Severe performance degradation and IPI storms due to unbatched TLB flushes.

Pre-existing issues:
- [High] Double dma_unmap_page() occurs if backup fails on the first page and is retried.
--

commit 45896340105924fcd05f25382e4001c011252d32
Author: Matthew Brost <matthew.brost@intel.com>

drm/ttm: Account for NULL pages in ttm_pool_backup

This commit fixes a NULL pointer dereference in ttm_pool_backup by
switching from the batched set_pages_array_wb() to set_pages_wb() after
validating that each page is not NULL.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 278bbe7a11add..2636bf1005925 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c

[ ... ]

> +		order = ttm_pool_page_order(pool, page);
> +		num_pages = 1UL << order;
>  
> +#ifdef CONFIG_X86
> +		/* Anything returned to the system needs to be cached. */
> +		if (tt->caching != ttm_cached)
> +			set_pages_wb(page, 1 << order);

[Severity: High]
When shrinking GPU buffers under memory pressure on 32-bit x86 systems, can
this cause a NULL pointer dereference in the CPA subsystem?

The previous implementation using set_pages_array_wb() had an internal check
for HighMem pages. Since page_address(page) returns NULL for HighMem pages,
passing them directly to set_pages_wb() calls set_memory_wb(0, numpages),
which triggers a warning and -EFAULT in __cpa_process_fault().

Should there be a !PageHighMem(page) check here?

[Severity: High]
Additionally, does replacing the batched set_pages_array_wb() with per-chunk
set_pages_wb() calls inside this loop cause an IPI storm?

For highly fragmented buffer objects, calling set_pages_wb() on every
allocation chunk will trigger a cross-CPU IPI for TLB invalidation each time,
potentially leading to severe latency spikes or soft lockups.

> +#endif
> +
> +		if (tt->dma_address)
> +			ttm_pool_unmap(pool, tt->dma_address[i],
> +				       num_pages);

[Severity: High]
This is a pre-existing issue, but does this unmap lead to a double
dma_unmap_page() if the backup fails and is retried?

If ttm_backup_backup_page() returns ENOMEM for the first page, the loop
aborts and returns a negative error without setting TTM_TT_FLAG_BACKED_UP.

Because this loop has already unmapped the DMA addresses unconditionally
without clearing the pointers in the array, a subsequent retry from the
shrinker will execute this loop again, calling dma_unmap_page() on the same
IOVA addresses.

> +		if (flags->purge) {
> +			shrunken += num_pages;
> +			page->private = 0;
> +			__free_pages_gpu_account(page, order, false);
> +			memset(tt->pages + i, 0,
> +			       num_pages * sizeof(*tt->pages));
>  		}
>  	}

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