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
