Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing resource cleanup in `ttm_global_init()` error paths leads to use-after-free and memory leaks. - [High] `ttm_pool_mgr_init()` NULL pointer dereference on memory-less NUMA nodes. - [High] `ttm_pool_type_init()` ignores the return value of `list_lru_init()`, leading to NULL pointer dereference on allocation failure. -- --- Patch [1]: [PATCH] ttm: Add error handling for ttm_pool_mgr_init() --- commit 2f605d6f8238add861f5522786f57313330623f4 Author: Óscar Megía López <[email protected]> ttm: Add error handling for ttm_pool_mgr_init() This commit updates ttm_global_init() to check the return value of ttm_pool_mgr_init(). If initialization fails, it now correctly jumps to the error path to prevent the system from continuing with an invalid pool manager state. > diff --git a/drivers/gpu/drm/ttm/ttm_device.c > b/drivers/gpu/drm/ttm/ttm_device.c > index d3bfb9a696a74..c880a04303635 100644 > --- a/drivers/gpu/drm/ttm/ttm_device.c > +++ b/drivers/gpu/drm/ttm/ttm_device.c > @@ -96,7 +96,10 @@ static int ttm_global_init(void) > >> PAGE_SHIFT; > num_dma32 = min(num_dma32, 2UL << (30 - PAGE_SHIFT)); > > - ttm_pool_mgr_init(num_pages); > + ret = ttm_pool_mgr_init(num_pages); > + if (ret) > + goto out; > + [Severity: High] This is a pre-existing issue, but looking into ttm_pool_mgr_init(), it iterates over all possible nodes: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_mgr_init() { ... for_each_node(nid) { if (!page_pool_size) { u64 node_size = ttm_get_node_memory_size(nid); ... } And ttm_get_node_memory_size() directly dereferences NODE_DATA(nid): drivers/gpu/drm/ttm/ttm_pool.c:ttm_get_node_memory_size() { pg_data_t *pgdat = NODE_DATA(nid); for (zone_type = 0; zone_type < MAX_NR_ZONES; zone_type++) managed_pages += zone_managed_pages(&pgdat->node_zones[zone_type]); ... } Can this trigger a NULL pointer dereference on systems with memory-less NUMA nodes since it uses for_each_node() instead of for_each_node_state(nid, N_MEMORY)? [Severity: High] This is also a pre-existing issue, but further down in the pool manager initialization, ttm_pool_type_init() calls list_lru_init() without checking its return value: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_type_init() { ... list_lru_init(&pt->pages); ... } Since list_lru_init() can fail and return an error code like -ENOMEM, does ignoring this return value leave the list_lru structure in an invalid state? Could this lead to a crash later in ttm_pool_type_give() when list_lru_add() attempts to use the unallocated internal node arrays? [Severity: High] This isn't a bug introduced by this patch, but if alloc_page() fails later in ttm_global_init(), we jump to this same out label. Looking at the out block: out: if (ret && ttm_debugfs_root) debugfs_remove(ttm_debugfs_root); if (ret) --ttm_glob_use_count; mutex_unlock(&ttm_global_mutex); return ret; Does this leave the mm_shrinker registered in the global shrinker list since ttm_pool_mgr_fini() is never called in this error path? If the driver module is subsequently unloaded, could the shrinker's callback pointers point to freed module text and cause a use-after-free on the next shrinker invocation? > ttm_tt_mgr_init(num_pages, num_dma32); > > glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32 | -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
