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 <[email protected]> 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/[email protected]?part=1
