Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Unchecked `list_lru_init()` return value in `ttm_pool_type_init()` causes a deterministic NULL pointer dereference in the newly added error path. - [High] Concurrent debugfs access during module unload or initialization failure causes a NULL pointer dereference due to a teardown race introduced by the patch. Pre-existing issues: - [High] `ttm_global_init()` leaks the registered shrinker and list_lrus if `alloc_page()` fails. -- commit e23de1fd240385312484194a68f533845be6969a Author: Óscar Megía López <[email protected]> Memory leak error in qxl unbind This commit fixes a memory leak and refcount corruption during OOM conditions by adding error handling for ttm_pool_mgr_init() in ttm_global_init() and properly cleaning up pool types if shrinker_alloc() fails. Link: https://lore.kernel.org/virtualization/[email protected]/T/#u 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; + ttm_tt_mgr_init(num_pages, num_dma32); glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32 | [Severity: High] This is a pre-existing issue, but does the error path for alloc_page() leak the registered shrinker? If alloc_page() fails in ttm_global_init(), the code jumps to out: and returns -ENOMEM without calling ttm_pool_mgr_fini(). This permanently leaks the previously registered mm_shrinker. Because the global mm_shrinker pointer is overwritten on a subsequent retry, the leaked shrinker is never freed. When the ttm module is unloaded, the leaked shrinker remains registered with the MM subsystem. Any subsequent memory pressure will invoke the shrinker callback pointing to unloaded module text, crashing the system. [ ... ] diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c index 278bbe7a11add..e0c82804a07d5 100644 --- a/drivers/gpu/drm/ttm/ttm_pool.c +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] @@ -1434,6 +1448,10 @@ int ttm_pool_mgr_init(unsigned long num_pages) shrinker_register(mm_shrinker); return 0; + +err_shrinker: + ttm_pool_type_fini_and_list_lru_destroy(); [Severity: High] Can ttm_pool_type_fini_and_list_lru_destroy() cause a NULL pointer dereference here if list_lru_init() previously failed? ttm_pool_type_init() ignores the return value of list_lru_init(). If it fails due to memory pressure, pt->pages.node remains NULL. This new err_shrinker path invokes ttm_pool_type_fini(), which unconditionally calls list_lru_walk(). list_lru_walk() dereferences lru->node, which would crash the system on the NULL pointer. + return ret; } /** @@ -1442,17 +1460,24 @@ int ttm_pool_mgr_init(unsigned long num_pages) * Cleanup the global pools and unregister the MM shrinker. */ void ttm_pool_mgr_fini(void) +{ + shrinker_free(mm_shrinker); + ttm_pool_type_fini_and_list_lru_destroy(); [Severity: High] Does this introduce a race condition with debugfs access? By destroying the list_lru structures here, their nodes are set to NULL. However, ttm_pool_mgr_fini() executes before debugfs_remove() in ttm_global_release() (and before the out: label cleanup in ttm_global_init()). If a user concurrently reads the /sys/kernel/debug/ttm/page_pool file, ttm_pool_debugfs_globals_show() invokes list_lru_count(). Since list_lru_destroy() has already set lru->node to NULL, the count function will dereference the NULL pointer. + WARN_ON(!list_empty(&shrinker_list)); +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
