Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing backup handle guard in the backup loop causes a kernel oops when retrying a partially failed backup. - [High] Double DMA unmap and mismatched unmap sizes on backup failure retry. -- commit 8838e9f25b3625ff2d46011f99d3a64b02d5abdf Author: Matthew Brost <[email protected]> drm/ttm: Account for NULL and handle pages in ttm_pool_backup This patch modifies ttm_pool_backup to account for NULL pages and backup handles in the dma/purge loop. It adds a dedicated writeback pass before the dma/purge loop that walks allocations and skips NULL and handle entries, issuing set_pages_array_wb() once per contiguous run. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 3d5f2ae0a4564..ff043420d517d 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -1075,7 +1097,7 @@ long ttm_pool_backup(struct ttm_pool *pool, struct > ttm_tt *tt, > unsigned int order; > > page = tt->pages[i]; > - if (unlikely(!page)) { > + if (unlikely(!page || > ttm_backup_page_ptr_is_handle(page))) { > num_pages = 1; > continue; > } > > order = ttm_pool_page_order(pool, page); > num_pages = 1UL << order; > if (tt->dma_address) > ttm_pool_unmap(pool, tt->dma_address[i], > num_pages); [Severity: High] This isn't a bug introduced by this patch, but does this loop leave stale DMA addresses in tt->dma_address[i]? If the subsequent backup loop fails on the first page due to an allocation failure in ttm_backup_backup_page(): shandle = ttm_backup_backup_page(backup, page, flags->writeback, i, ...); if (shandle < 0) { ret = shandle; break; } The variable shrunken remains 0, and TTM_TT_FLAG_BACKED_UP is never set. Since the buffer object remains eligible for immediate retry by the shrinker, will the next pass trigger a double unmap on these same addresses? Additionally, since ttm_pool_split_for_swap() zeroes page->private right before the failure, will the retry incorrectly unmap order-0 pages instead of their original multi-order sizes? [ ... ] > for (i = 0; i < num_pages; ++i) { > s64 shandle; > > page = tt->pages[i]; > if (unlikely(!page)) > continue; > > ttm_pool_split_for_swap(pool, page); [Severity: High] This is a pre-existing issue, but is there a missing check for backup handles in this loop? If a buffer object is partially backed up, and userspace accesses it to trigger a page fault, the TTM_TT_FLAG_BACKED_UP flag is cleared. If the buffer object is later targeted by the shrinker again, the earlier loops in this function will correctly skip the existing backup handles due to the newly added ttm_backup_page_ptr_is_handle() guards. However, in this final backup loop, will the code extract an existing backup handle, bypass the !page check, and pass the handle directly to ttm_pool_split_for_swap(), resulting in an invalid pointer dereference? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
