Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Harvesting tail pages of a bisected chunk during defragmentation leads to Use-After-Free. - [High] Missing __GFP_NOWARN and __GFP_RETRY_MAYFAIL in best-effort preallocation causes log spam and potential OOM killer invocation. --
--- Patch [9]: [PATCH v2 09/33] drm/ttm: Preallocate beneficial-order defrag pages outside the lock --- commit d825f68af024a3a3838c6e5d1ecfcf5ee8717374 Author: Matthew Brost <[email protected]> Subject: drm/ttm: Preallocate beneficial-order defrag pages outside the lock This commit introduces a mechanism to preallocate high-order pages outside the DMA reservation lock during defragmentation moves. By using a prealloc bag, it avoids stalling in direct reclaim or compaction while holding the lock, improving concurrent rendering performance. > 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 [ ... ] > @@ -929,6 +939,172 @@ static int ttm_pool_harvest_remaining(struct ttm_pool > *pool, > return 0; > } > > +unsigned int ttm_pool_prealloc_order(struct ttm_pool *pool) > +{ > + return ttm_pool_beneficial_order(pool); > +} > +EXPORT_SYMBOL(ttm_pool_prealloc_order); > + > +/* > + * Build the gfp flags used for the high-order, possibly reclaiming, > beneficial > + * order page allocations, matching the in-line defrag alloc path. > + */ > +static gfp_t ttm_pool_prealloc_gfp(struct ttm_pool *pool) > +{ > + gfp_t gfp = GFP_USER; > + > + /* > + * No highmem: prealloc applies caching in bulk via set_pages_array_*() > + * on the kernel mapping, so the pages must be permanently mapped. > + */ > + if (ttm_pool_uses_dma32(pool)) > + gfp |= GFP_DMA32; > + > + return gfp; > +} [Severity: High] Does this base GFP flag configuration risk invoking the OOM killer on a fragmented system? The preallocation performed in ttm_pool_prealloc_fill() appears to be an opportunistic, best-effort operation: ttm_pool_prealloc_fill() { ... struct page *p = ttm_pool_alloc_page(pool, gfp, order, false); Without including __GFP_NOWARN and __GFP_RETRY_MAYFAIL here, a failure to allocate these high-order pages will trigger aggressive reclaim. This could unnecessarily invoke the OOM killer and spam the logs, even though the pool can safely fall back to in-line allocation. Should these suppression flags be added to the preallocation gfp mask? [ ... ] > @@ -1178,8 +1374,8 @@ static int __ttm_pool_alloc(struct ttm_pool *pool, > struct ttm_tt *tt, > alloc->remaining_pages; > it.order = ttm_pool_alloc_find_order(it.order, alloc)) { > /* > - * Out of defrag budget: harvest the rest of the old tt as-is > and > - * stop (the tt is re-queued if the remainder is still > + * Out of defrag budget/prealloc: harvest the rest of the old tt > + * as-is and stop (the tt is re-queued if the remainder is still > * sub-optimal). > */ > if (ttm_pool_iter_defrag_exhausted(&it)) { [Severity: Critical] Can this lead to memory corruption if the budget is exhausted while the offset is misaligned with the chunks in the old tt? If the budget is exhausted here, ttm_pool_iter_harvest_rest() will call ttm_pool_harvest_remaining(), which reads the order from the current page: ttm_pool_harvest_remaining() { ... struct page *p = old_tt->pages[off]; unsigned int order = ttm_pool_page_order(pool, p); If off points to a tail page of a previously allocated larger chunk, ttm_pool_page_order() will return 0 because the private field is 0 on tail pages. This causes the code to falsely harvest it as an order-0 chunk. When the old tt is later freed, it frees the entire large chunk from the head page, leaving the new tt with pointers to freed memory. > + r = ttm_pool_iter_harvest_rest(&it); > + if (r) > + goto error_free_all; > + break; > + } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
