Re: [PATCH 03/10] ttm/pool: initialise the shrinker earlier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Premature shrinker registration leads to kernel crash on empty list traversal
- [High] Unhandled memory allocation failure in ttm_pool_mgr_init() leaves global memory pools uninitialized

Pre-existing issues:
- [Critical] Concurrent shrinker invocation during module teardown causes invalid pointer dereference
- [High] Missing error handling for list_lru_init_memcg() leading to NULL pointer dereference
--

commit 979d1132acdad03dc6f5c6c5ed80def9a1603b9f
Author: Dave Airlie <airlied@redhat.com>

ttm/pool: initialise the shrinker earlier

Later memcg enablement needs the shrinker initialised before the list lru,
Just move it for now.

> 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
> @@ -1402,6 +1402,17 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>  	spin_lock_init(&shrinker_lock);
>  	INIT_LIST_HEAD(&shrinker_list);
>  
> +	mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
> +	if (!mm_shrinker)
> +		return -ENOMEM;
> +

[Severity: High]
If shrinker_alloc() fails here and returns -ENOMEM, ttm_pool_mgr_init()
exits early before initializing global_write_combined, global_uncached, and
other pools.

Since the caller ttm_global_init() ignores the return value of
ttm_pool_mgr_init(), will this allow TTM initialization to continue with
uninitialized global pools, resulting in a NULL pointer dereference later
when list_lru_walk_one() accesses the zero-initialized list_lru structures?

> +	mm_shrinker->count_objects = ttm_pool_shrinker_count;
> +	mm_shrinker->scan_objects = ttm_pool_shrinker_scan;
> +	mm_shrinker->batch = TTM_SHRINKER_BATCH;
> +	mm_shrinker->seeks = 1;
> +
> +	shrinker_register(mm_shrinker);
> +

[Severity: Critical]
Does registering the shrinker at this point expose it before the
shrinker_list is populated by ttm_pool_type_init() in the loop below?

If the shrinker is invoked immediately, for example via memory pressure or
a debugfs write, ttm_pool_shrink() will perform list_first_entry() on an
empty shrinker_list:

ttm_pool_shrinker_scan()->ttm_pool_shrink() {
        spin_lock(&shrinker_lock);
        pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
        list_move_tail(&pt->shrinker_list, &shrinker_list);
        spin_unlock(&shrinker_lock);
}

Could this result in a kernel panic by returning a garbage pointer from the
empty list and passing it to list_lru_walk_node()?

>  	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
>  		ttm_pool_type_init(&global_write_combined[i], NULL,
>  				   ttm_write_combined, i);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
list_lru_init_memcg() called internally by ttm_pool_type_init()?

ttm_pool_type_init() {
        ...
        list_lru_init_memcg(&pt->pages, mm_shrinker);
}

If list_lru_init_memcg() fails due to system memory pressure and returns an
error code like -ENOMEM, pt->pages.node remains NULL while the pool type is
still added to the active lists. Could subsequent operations like
list_lru_add() dereference this NULL pointer?

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but looking at the teardown path
in ttm_pool_mgr_fini(), the global pools are destroyed and removed from
shrinker_list before the shrinker is unregistered:

ttm_pool_mgr_fini() {
        for (i = 0; i < NR_PAGE_ORDERS; ++i) {
                ttm_pool_type_fini(&global_write_combined[i]);
                ...
        }
        shrinker_free(mm_shrinker);
}

If the shrinker is invoked concurrently during this window, it will again
execute list_first_entry() on an empty shrinker_list in ttm_pool_shrink().

Unlike ttm_pool_fini(), ttm_pool_mgr_fini() does not call
ttm_pool_synchronize_shrinkers() before destroying the pools. Does this
leave a window open for an invalid pointer dereference during module
teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706024122.853329-1-airlied@gmail.com?part=3