Re: [PATCH v5 2/6] list_lru_init() does not check return value

[sashiko-bot@kernel.org]

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

New issues:
- [High] The return value of `ttm_pool_type_init()` is ignored in `ttm_pool_mgr_init()`, leading to kernel panics if `list_lru_init()` fails.
- [High] External callers of `ttm_pool_init()` and `ttm_pool_mgr_init()` ignore their error return values, leading to silent failures and crashes.

Pre-existing issues:
- [High] Missing `list_lru_destroy()` calls in normal teardown paths leak memory on every pool destruction and module unload.
--

--- Patch [2]: [PATCH v5 2/6] list_lru_init() does not check return value ---
commit 05689bd64e30d8c766538356d67e8a63f77ecc7c
Author: Óscar Megía López <megia.oscar@gmail.com>

list_lru_init() does not check return value

This patch adds an error check for list_lru_init() in ttm_pool_type_init(),
propagating the error code back to the caller if the initialization fails.

Cc: <stable@vger.kernel.org> # 7.1.0

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 73284497cc785..e116b83db1356 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -374,17 +374,23 @@ static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, int nid)
>  }
>  
>  /* Initialize and add a pool type to the global shrinker list */
> -static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
> +static int ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
>  			       enum ttm_caching caching, unsigned int order)

[Severity: High]
Since ttm_pool_type_init() now returns an error code, do the callers need
to be updated to check it?

Looking at ttm_pool_mgr_init(), it calls ttm_pool_type_init() in a loop
without checking the return value. If list_lru_init() fails, it skips adding
the node to shrinker_list.

During module unload, ttm_pool_mgr_fini() will call ttm_pool_type_fini(),
which executes list_del() on the uninitialized items. Will this result in a
NULL pointer dereference?

Also, ttm_pool_type_take() calls list_lru_walk_node() unconditionally on
pt->pages. Can this crash if it was never initialized?

[Severity: High]
Similarly, do external callers of ttm_pool_init() and ttm_pool_mgr_init()
need to check for errors now that list_lru_init() failures are propagated?

Callers like ttm_device_init() and amdgpu_ttm_pools_init() appear to ignore
the return values. If initialization fails, the device driver might later
attempt an allocation or unbind the device, accessing uninitialized list_lru
structures.

>  {
> +	int ret = 0;
> +
>  	pt->pool = pool;
>  	pt->caching = caching;
>  	pt->order = order;
> -	list_lru_init(&pt->pages);
> +	ret = list_lru_init(&pt->pages);

[Severity: High]
This is a pre-existing issue, but does normal teardown leak the list_lru
allocations?

list_lru_init() dynamically allocates node arrays. While ttm_pool_type_init()
sets it up, ttm_pool_type_fini() does not appear to call list_lru_destroy().

Neither ttm_pool_fini() nor ttm_pool_mgr_fini() call list_lru_destroy() on
normal device unbinds or module unloads. If a device is repeatedly bound and
unbound via sysfs, will this systematically leak memory and eventually exhaust
system memory?

> +	if (ret)
> +		return ret;
>  
>  	spin_lock(&shrinker_lock);
>  	list_add_tail(&pt->shrinker_list, &shrinker_list);
>  	spin_unlock(&shrinker_lock);
> +
> +	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811194224.121597-1-megia.oscar@gmail.com?part=2