Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] Premature shrinker registration leads to kernel crash on empty list traversal - [High] Unhandled memory allocation failure in ttm_pool_mgr_init() leaves global memory pools uninitialized Pre-existing issues: - [Critical] Concurrent shrinker invocation during module teardown causes invalid pointer dereference - [High] Missing error handling for list_lru_init_memcg() leading to NULL pointer dereference -- commit 979d1132acdad03dc6f5c6c5ed80def9a1603b9f Author: Dave Airlie <[email protected]> ttm/pool: initialise the shrinker earlier Later memcg enablement needs the shrinker initialised before the list lru, Just move it for now. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -1402,6 +1402,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 here and returns -ENOMEM, ttm_pool_mgr_init() exits early before initializing global_write_combined, global_uncached, and other pools. Since the caller ttm_global_init() ignores the return value of ttm_pool_mgr_init(), will this allow TTM initialization to continue with uninitialized global pools, resulting in a NULL pointer dereference later when list_lru_walk_one() accesses the zero-initialized list_lru structures? > + 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); > + [Severity: Critical] Does registering the shrinker at this point expose it before the shrinker_list is populated by ttm_pool_type_init() in the loop below? If the shrinker is invoked immediately, for example via memory pressure or a debugfs write, ttm_pool_shrink() will perform list_first_entry() on an empty shrinker_list: ttm_pool_shrinker_scan()->ttm_pool_shrink() { spin_lock(&shrinker_lock); pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list); list_move_tail(&pt->shrinker_list, &shrinker_list); spin_unlock(&shrinker_lock); } Could this result in a kernel panic by returning a garbage pointer from the empty list and passing it to list_lru_walk_node()? > for (i = 0; i < NR_PAGE_ORDERS; ++i) { > ttm_pool_type_init(&global_write_combined[i], NULL, > ttm_write_combined, i); [Severity: High] This is a pre-existing issue, but is it safe to ignore the return value of list_lru_init_memcg() called internally by ttm_pool_type_init()? ttm_pool_type_init() { ... list_lru_init_memcg(&pt->pages, mm_shrinker); } If list_lru_init_memcg() fails due to system memory pressure and returns an error code like -ENOMEM, pt->pages.node remains NULL while the pool type is still added to the active lists. Could subsequent operations like list_lru_add() dereference this NULL pointer? [ ... ] [Severity: Critical] This is a pre-existing issue, but looking at the teardown path in ttm_pool_mgr_fini(), the global pools are destroyed and removed from shrinker_list before the shrinker is unregistered: ttm_pool_mgr_fini() { for (i = 0; i < NR_PAGE_ORDERS; ++i) { ttm_pool_type_fini(&global_write_combined[i]); ... } shrinker_free(mm_shrinker); } If the shrinker is invoked concurrently during this window, it will again execute list_first_entry() on an empty shrinker_list in ttm_pool_shrink(). Unlike ttm_pool_fini(), ttm_pool_mgr_fini() does not call ttm_pool_synchronize_shrinkers() before destroying the pools. Does this leave a window open for an invalid pointer dereference during module teardown? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
