Re: [PATCH v5 4/6] ttm_pool_type_init() does not check return value

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing return value check of ttm_pool_mgr_init() combined with destructive error cleanup leads to double free and list poisoning panic.
--

--- Patch [4]: [PATCH v5 4/6] ttm_pool_type_init() does not check return value ---
commit 141193c84a0c35c0b77d8194cafc18a0ddfa41ee
Author: Óscar Megía López <[email protected]>

ttm_pool_type_init() does not check return value

This commit modifies ttm_pool_mgr_init() to check the return value of
ttm_pool_type_init(). If initialization fails, it cleans up the partially
allocated structures, frees the shrinker, and returns the error code.

Cc: <[email protected]> # 7.1.0
Fixes: 444e2a19d7fd ("ttm/pool: port to list_lru. (v2)")

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index b95c6548adf7f..8d9891ea8c6aa 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c

[ ... ]

> @@ -1537,6 +1585,8 @@ static inline u64 ttm_get_node_memory_size(int nid)
>  int ttm_pool_mgr_init(unsigned long num_pages)
>  {
>  	unsigned int i;
> +	int ret = 0;
> +	struct ttm_pool_type *types_free[3];
>  
>  	int nid;
>  	for_each_node(nid) {

[ ... ]

> +	mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
> +	if (!mm_shrinker)
> +		return -ENOMEM;
> +
>  	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
> -		ttm_pool_type_init(&global_write_combined[i], NULL,
> +		ret = ttm_pool_type_init(&global_write_combined[i], NULL,
>  				   ttm_write_combined, i);
> -		ttm_pool_type_init(&global_uncached[i], NULL, ttm_uncached, i);
> +		if (ret) {
> +			ttm_pool_type_fini_and_list_lru_destroy(i);
> +			shrinker_free(mm_shrinker);
> +			return ret;

[Severity: High]
If ttm_pool_mgr_init() fails and returns an error here, can this lead to
a double-free or panic during teardown?

Looking at the caller ttm_global_init() in drivers/gpu/drm/ttm/ttm_device.c,
the return value is ignored:

ttm_global_init(void)
{
        ...
        ttm_pool_mgr_init(num_pages);
        ttm_tt_mgr_init(num_pages, num_dma32);
        ...
}

Because the caller ignores the error, the system will continue running with
partially destroyed global state. When the module is eventually unloaded,
ttm_pool_mgr_fini() will run unconditionally:

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

Since mm_shrinker isn't set to NULL after the error path calls
shrinker_free(), won't ttm_pool_mgr_fini() trigger a double-free on the
shrinker?

Also, won't ttm_pool_mgr_fini() attempt to access list entries that have
already been poisoned or deleted by
ttm_pool_type_fini_and_list_lru_destroy() in the error path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4