Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The return value of `ttm_pool_type_init()` is ignored in `ttm_pool_mgr_init()`, leading to kernel panics if `list_lru_init()` fails. - [High] External callers of `ttm_pool_init()` and `ttm_pool_mgr_init()` ignore their error return values, leading to silent failures and crashes. Pre-existing issues: - [High] Missing `list_lru_destroy()` calls in normal teardown paths leak memory on every pool destruction and module unload. -- --- Patch [2]: [PATCH v5 2/6] list_lru_init() does not check return value --- commit 05689bd64e30d8c766538356d67e8a63f77ecc7c Author: Óscar Megía López <[email protected]> list_lru_init() does not check return value This patch adds an error check for list_lru_init() in ttm_pool_type_init(), propagating the error code back to the caller if the initialization fails. Cc: <[email protected]> # 7.1.0 > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 73284497cc785..e116b83db1356 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -374,17 +374,23 @@ static struct page *ttm_pool_type_take(struct > ttm_pool_type *pt, int nid) > } > > /* Initialize and add a pool type to the global shrinker list */ > -static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool > *pool, > +static int ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool > *pool, > enum ttm_caching caching, unsigned int order) [Severity: High] Since ttm_pool_type_init() now returns an error code, do the callers need to be updated to check it? Looking at ttm_pool_mgr_init(), it calls ttm_pool_type_init() in a loop without checking the return value. If list_lru_init() fails, it skips adding the node to shrinker_list. During module unload, ttm_pool_mgr_fini() will call ttm_pool_type_fini(), which executes list_del() on the uninitialized items. Will this result in a NULL pointer dereference? Also, ttm_pool_type_take() calls list_lru_walk_node() unconditionally on pt->pages. Can this crash if it was never initialized? [Severity: High] Similarly, do external callers of ttm_pool_init() and ttm_pool_mgr_init() need to check for errors now that list_lru_init() failures are propagated? Callers like ttm_device_init() and amdgpu_ttm_pools_init() appear to ignore the return values. If initialization fails, the device driver might later attempt an allocation or unbind the device, accessing uninitialized list_lru structures. > { > + int ret = 0; > + > pt->pool = pool; > pt->caching = caching; > pt->order = order; > - list_lru_init(&pt->pages); > + ret = list_lru_init(&pt->pages); [Severity: High] This is a pre-existing issue, but does normal teardown leak the list_lru allocations? list_lru_init() dynamically allocates node arrays. While ttm_pool_type_init() sets it up, ttm_pool_type_fini() does not appear to call list_lru_destroy(). Neither ttm_pool_fini() nor ttm_pool_mgr_fini() call list_lru_destroy() on normal device unbinds or module unloads. If a device is repeatedly bound and unbound via sysfs, will this systematically leak memory and eventually exhaust system memory? > + if (ret) > + return ret; > > spin_lock(&shrinker_lock); > list_add_tail(&pt->shrinker_list, &shrinker_list); > spin_unlock(&shrinker_lock); > + > + return 0; > } > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
