Re: [PATCH v5 3/6] ttm_pool_fini() does not destroy list lru

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

Pre-existing issues:
- [High] ttm_pool_mgr_init() ignores the return value of ttm_pool_type_init(), which can lead to a NULL pointer dereference.
- [High] ttm_pool_shrink() leaks the pool_shrink_rwsem read lock when the shrinker list is empty.
- [High] ttm_global_init() ignores the return value of ttm_pool_mgr_init().
- [High] ttm_device_init() ignores the return value of ttm_pool_init(), causing a potential NULL pointer dereference.
- [Medium] Missing list_lru_destroy() in ttm_pool_mgr_fini() causes a memory leak for global pools on module unload.
- [Medium] Memory leak on the shrinker_alloc() error path in ttm_pool_mgr_init().
--

--- Patch [3]: [PATCH v5 3/6] ttm_pool_fini() does not destroy list lru ---
commit bab162cb05ce53ff29cbacb61b7c2543d1b98ecc
Author: Óscar Megía López <[email protected]>
ttm_pool_fini() does not destroy list lru

This commit adds a missing call to list_lru_destroy() in ttm_pool_fini()
after calling ttm_pool_type_fini(). It ensures the list lru structures
are properly destroyed when finalizing a ttm pool.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index e116b83db1356..b95c6548adf7f 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -1331,6 +1331,17 @@ void ttm_pool_fini(struct ttm_pool *pool)
>  	 * that no shrinker is concurrently freeing pages from the pool.
>  	 */
>  	ttm_pool_synchronize_shrinkers();
> +
> +	for (i = 0; i < TTM_NUM_CACHING_TYPES; ++i) {
> +		for (j = 0; j < NR_PAGE_ORDERS; ++j) {
> +			struct ttm_pool_type *pt;
> +
> +			pt = ttm_pool_select_type(pool, i, j);
> +			if (pt != &pool->caching[i].orders[j])
> +				continue;
> +			list_lru_destroy(&pt->pages);
> +		}
> +	}
>  }
>  EXPORT_SYMBOL(ttm_pool_fini);

[Severity: Medium]
This is a pre-existing issue, but does ttm_pool_mgr_fini() leak memory
on module unload?

When the module is unloaded, it calls ttm_pool_type_fini() to free the pages,
but it appears to miss calling list_lru_destroy() for the global pools:

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_mgr_fini() {
	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
		ttm_pool_type_fini(&global_write_combined[i]);
		...
	}
}

Will this permanently leak the memory allocated for the list_lru structures
during initialization?

[Severity: High]
This isn't a bug introduced by this patch, but does ttm_pool_mgr_init()
ignore the return value of ttm_pool_type_init()?

If list_lru_init() fails due to memory exhaustion, the pool's list_lru
remains uninitialized. The loop continues without aborting:

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_mgr_init() {
	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
		ttm_pool_type_init(&global_write_combined[i], NULL,
				   ttm_write_combined, i);
		...
	}
}

Later, when pages are freed, list_lru_add() might blindly dereference
a NULL lru->node array:

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_type_give() {
	rcu_read_lock();
	list_lru_add(&pt->pages, &p->lru, nid, NULL);
	rcu_read_unlock();
}

Could this cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but does ttm_pool_shrink() leak the
pool_shrink_rwsem read lock when the shrinker list is empty?

The read lock is acquired via down_read(&pool_shrink_rwsem). If the
shrinker_list is empty, it sets a flag and returns early without releasing
the lock:

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;
	} else {
		...
	}
	spin_unlock(&shrinker_lock);

	if (empty)
		return 0;
}

Could this deadlock the system when subsequent attempts try to acquire
a write lock in ttm_pool_synchronize_shrinkers()?

[Severity: High]
This isn't a bug introduced by this patch, but does ttm_global_init() mask
initialization failures?

The return value of ttm_pool_mgr_init() is discarded:

drivers/gpu/drm/ttm/ttm_device.c:ttm_global_init() {
	ttm_pool_mgr_init(num_pages);
	ttm_tt_mgr_init(num_pages, num_dma32);
}

If ttm_pool_mgr_init() fails, mm_shrinker might be left as NULL, which
could break the memory reclaim subsystem for TTM pools. Should this error
be propagated?

[Severity: High]
This is a pre-existing issue, but does ttm_device_init() ignore the return
value of ttm_pool_init()?

If ttm_pool_init() fails, it cleans up its partial state and returns an error
code, leaving bdev->pool uninitialized:

drivers/gpu/drm/ttm/ttm_device.c:ttm_device_init() {
	if (dev)
		nid = dev_to_node(dev);
	else
		nid = NUMA_NO_NODE;

	ttm_pool_init(&bdev->pool, dev, nid, alloc_flags);
}

During cleanup, ttm_device_fini() invokes ttm_pool_fini(), which calls
ttm_pool_type_fini(). This executes list_del(&pt->shrinker_list), which
might crash with a NULL pointer dereference because the list head was never
initialized. Can this lead to a crash on teardown?

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a memory leak on
the shrinker_alloc() error path in ttm_pool_mgr_init()?

If shrinker_alloc() fails, the function returns directly:

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_mgr_init() {
	mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
	if (!mm_shrinker)
		return -ENOMEM;
}

Since the global pool types were already initialized dynamically, will
returning directly here permanently leak the list_lru memory allocated
for those pools?

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