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