Re: [PATCH v5 5/6] ttm_pool_mgr_fini() does not destroy the list_lru

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