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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Deadlock due to leaked `pool_shrink_rwsem` read lock on early return in `ttm_pool_shrink()`.
- [High] Fatal NULL pointer dereference caused by bypassed global pool initialization upon `shrinker_alloc` failure.
--

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

ttm/pool: initialise the shrinker earlier (v2)

This commit moves the shrinker initialization earlier in the module load
sequence to accommodate future memcg enablement. It also adds a check in
the shrinker callback to handle uninitialized lists.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index e4dbf4c93091c..f12b68812081f 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -440,10 +440,14 @@ static unsigned int ttm_pool_shrink(int nid, unsigned long num_to_free)
>  
>  	down_read(&pool_shrink_rwsem);
>  	spin_lock(&shrinker_lock);
> -	pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
> -	list_move_tail(&pt->shrinker_list, &shrinker_list);
> +	pt = list_first_entry_or_null(&shrinker_list, typeof(*pt), shrinker_list);
> +	if (pt)
> +		list_move_tail(&pt->shrinker_list, &shrinker_list);
>  	spin_unlock(&shrinker_lock);
>  
> +	if (!pt)
> +		return 0;
> +

[Severity: High]
Does this early return leak the pool_shrink_rwsem read lock?

We acquire the lock just a few lines above via down_read(&pool_shrink_rwsem).
If the shrinker list is empty and pt is NULL, returning here skips the
up_read() call at the end of the function, which could lead to a deadlock
when another writer or reader attempts to acquire it.

[ ... ]

> @@ -1402,6 +1406,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 and returns early here, does it leave the global
pool structures completely uninitialized?

Because the caller ttm_global_init() ignores the return value of
ttm_pool_mgr_init(), driver initialization will continue. The newly moved
return bypasses the initialization loop for structures like
global_write_combined.

When the driver later attempts to allocate from these pools, or when
ttm_pool_mgr_fini() calls list_del() on the zero-filled uninitialized
structures, can this cause a NULL pointer dereference?

> +	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);
> +
>  	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
>  		ttm_pool_type_init(&global_write_combined[i], NULL,
>  				   ttm_write_combined, i);

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