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
