Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Destroying the global list_lru structures before removing the TTM debugfs files introduces a NULL pointer dereference vulnerability. Pre-existing issues: - [High] ttm_pool_shrink() leaks a read lock on pool_shrink_rwsem when shrinker_list is empty. - [High] The `ttm_pool_shrinker_scan()` function can enter an infinite busy-loop if `allocated_pages` is positive but all `list_lru`s are empty. -- --- Patch [5]: [PATCH v5 5/6] ttm_pool_mgr_fini() does not destroy the list_lru --- commit 39533ac28eb17cc8fb3c32efa95aac77421f3675 Author: Óscar Megía López <[email protected]> ttm_pool_mgr_fini() does not destroy the list_lru This commit adds list_lru_destroy() after ttm_pool_type_fini() to ensure the list_lru structures are properly destroyed during pool manager teardown, resolving a resource leak. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 8d9891ea8c6aa..0d063df864f37 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -1678,16 +1678,7 @@ int ttm_pool_mgr_init(unsigned long num_pages) > */ > void ttm_pool_mgr_fini(void) > { [ ... ] > shrinker_free(mm_shrinker); > + ttm_pool_type_fini_and_list_lru_destroy(NR_PAGE_ORDERS); [Severity: High] Does this introduce a race condition with debugfs? Looking at the teardown sequence in ttm_global_release(), ttm_pool_mgr_fini() is called before debugfs_remove(ttm_debugfs_root). This leaves a window where the page_pool debugfs file remains accessible after the underlying list_lru structures are completely destroyed. If a user reads the page_pool debugfs file during this window, ttm_pool_debugfs_globals_show() iterates over the global pool arrays and calls list_lru_count(). Since list_lru_destroy() frees the internal nodes array and sets lru->node to NULL, won't this result in a NULL pointer dereference inside list_lru_count_node()? This is a pre-existing issue, but I noticed a lock leak in ttm_pool_shrink() that could be triggered during this same teardown window. When shrinker_list is empty, ttm_pool_shrink() has an early return path: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_shrink() { down_read(&pool_shrink_rwsem); spin_lock(&shrinker_lock); if ((shrinker_list.prev == &shrinker_list) && ...) { empty = 1; ... if (empty) return 0; ... } Since down_read(&pool_shrink_rwsem) is not released on this early return path, won't this leak the read lock and cause a deadlock during subsequent operations? This is also a pre-existing issue, but the shrinker scan loop appears vulnerable to an infinite busy-loop. In ttm_pool_shrinker_scan(), there is a loop checking allocated_pages: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_shrinker_scan() { do num_freed += ttm_pool_shrink(sc->nid, sc->nr_to_scan); while (num_freed < sc->nr_to_scan && atomic_long_read(&allocated_pages[sc->nid])); ... } Meanwhile, ttm_pool_type_take() isolates pages from the LRU before updating the counter: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_type_take() { ret = list_lru_walk_node(&pt->pages, ...); if (ret == 1 && p) { atomic_long_sub(1 << pt->order, &allocated_pages[nid]); ... } During the window where a page is removed from the LRU but allocated_pages has not yet been decremented, the LRU could be empty while allocated_pages is still positive. If ttm_pool_shrinker_scan() runs concurrently and sees ttm_pool_shrink() return 0, could it spin indefinitely without yielding the CPU, especially since there is no cond_resched() inside the loop? > WARN_ON(!list_empty(&shrinker_list)); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
