Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Missing `up_read(&pool_shrink_rwsem)` on the early return path in `ttm_pool_shrink()` causes a lock leak and subsequent deadlocks. - [High] Callers of `ttm_pool_init()` are not updated to check its new `int` return value, leading to NULL pointer dereferences on initialization failure. - [Low] Assigning the `void` return value of `ttm_pool_type_init()` to an `int` variable causes a compilation failure. - [Low] Reinventing `list_empty()` with direct manual pointer comparisons. Pre-existing issues: - [High] Missing `list_lru_destroy()` in normal pool teardown paths (`ttm_pool_fini()` and `ttm_pool_mgr_fini()`) causes persistent memory leaks. -- --- Patch [1]: [PATCH v5 1/6] Memory leak error in qxl unbind --- commit 575a0762d1411d04c459167fe4dee31c7eee4aff Author: Óscar Megía López <[email protected]> Memory leak error in qxl unbind This commit attempts to fix a refcount corruption and memory leak in qxl unbind by changing ttm_pool_init to return an error, checking for an empty shrinker_list, and cleaning up list_lru structures on init failure. Link: https://lore.kernel.org/lkml/[email protected]/ Link: https://lore.kernel.org/dri-devel/[email protected]/ > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 1bf37023fed66..73284497cc785 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -457,13 +457,21 @@ static unsigned int ttm_pool_shrink(int nid, unsigned > long num_to_free) > LIST_HEAD(dispose); > struct ttm_pool_type *pt; > unsigned int num_pages; > + int empty = 0; > > down_read(&pool_shrink_rwsem); > spin_lock(&shrinker_lock); > - pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list); > - list_move_tail(&pt->shrinker_list, &shrinker_list); > + if ((shrinker_list.prev == &shrinker_list) && (shrinker_list.next == > &shrinker_list)) { [Severity: Low] Could we use list_empty(&shrinker_list) here instead of manually checking the prev and next pointers? > + empty = 1; > + } else { > + pt = list_first_entry(&shrinker_list, typeof(*pt), > shrinker_list); > + list_move_tail(&pt->shrinker_list, &shrinker_list); > + } > spin_unlock(&shrinker_lock); > > + if (empty) > + return 0; [Severity: High] Does this early return leak the pool_shrink_rwsem read lock? The down_read(&pool_shrink_rwsem) is acquired at the beginning of the function, but if the shrinker_list is empty, this path returns early without calling up_read(). This can cause any subsequent attempt to acquire a write lock (such as in ttm_pool_synchronize_shrinkers during module unload) to block indefinitely and deadlock the system. [ ... ] > @@ -1231,10 +1251,13 @@ long ttm_pool_backup(struct ttm_pool *pool, struct > ttm_tt *tt, > * > * Initialize the pool and its pool types. > */ > -void ttm_pool_init(struct ttm_pool *pool, struct device *dev, > +int ttm_pool_init(struct ttm_pool *pool, struct device *dev, [Severity: High] Are the callers of ttm_pool_init() updated to check this new int return value? For example, ttm_device_init() calls ttm_pool_init() but ignores the return value. If ttm_pool_init() fails under memory pressure and cleans up the lists, proceeding with driver initialization can cause a NULL pointer dereference when subsequent allocations attempt to use the destroyed LRU lists. > int nid, unsigned int alloc_flags) > { > - unsigned int i, j; > + unsigned int i, j, k; > + int ret; > + struct ttm_pool_type *initialized[TTM_NUM_CACHING_TYPES * > NR_PAGE_ORDERS]; > + unsigned int n_initialized = 0; [ ... ] > @@ -1251,23 +1274,28 @@ void ttm_pool_init(struct ttm_pool *pool, struct > device *dev, > if (pt != &pool->caching[i].orders[j]) > continue; > > - ttm_pool_type_init(pt, pool, i, j); > + ret = ttm_pool_type_init(pt, pool, i, j); [Severity: Low] Does this assignment cause a build failure? It appears ttm_pool_type_init() is still defined as returning void, so assigning its result to the int ret variable will break the build. > + if (ret) > + goto error; > + > + initialized[n_initialized++] = pt; > } > } > > + return 0; > + > +error: > + for (k = 0; k < n_initialized; ++k) > + ttm_pool_type_fini(initialized[k]); [Severity: High] This is a pre-existing issue, but does ttm_pool_type_fini() fail to destroy the list_lru structures? While ttm_pool_type_fini() drains the LRU via list_lru_walk(), it doesn't call list_lru_destroy(&pt->pages). This causes a persistent memory leak of the node arrays on every device unbind or module unload where the normal pool teardown paths (like ttm_pool_fini and ttm_pool_mgr_fini) are used. > + > + ttm_pool_synchronize_shrinkers(); > + > + for (k = 0; k < n_initialized; ++k) > + list_lru_destroy(&initialized[k]->pages); > + > + return ret; > } > +EXPORT_SYMBOL(ttm_pool_init); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
