Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mm: debugfs NULL pointer dereference after premature list_lru_destroy() --
commit 32ad6c26fe41ce4b4170b40c6c684d9806d52913 Author: Óscar Megía López <[email protected]> Memory leak error in qxl unbind This commit addresses a memory leak during qxl unbind by calling list_lru_destroy() for each global pool type's page list in ttm_pool_mgr_fini(). It also introduces ttm_pool_synchronize_shrinkers() to prevent concurrent shrinker access to pages being freed. Link: https://lore.kernel.org/virtualization/[email protected]/ [1] Link: https://lore.kernel.org/dri-devel/[email protected]/ [2] > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 1bf37023fed66..520057cb13332 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -1559,6 +1559,18 @@ void ttm_pool_mgr_fini(void) > ttm_pool_type_fini(&global_dma32_uncached[i]); > } > > + /* We removed the pool types from the LRU, but we need to also make sure > + * that no shrinker is concurrently freeing pages from the pool. > + */ > + ttm_pool_synchronize_shrinkers(); > + > + for (i = 0; i < NR_PAGE_ORDERS; ++i) { > + list_lru_destroy(&global_write_combined[i].pages); > + list_lru_destroy(&global_uncached[i].pages); > + list_lru_destroy(&global_dma32_write_combined[i].pages); > + list_lru_destroy(&global_dma32_uncached[i].pages); > + } > + [Severity: High] Does this list_lru_destroy() occur while the debugfs interface is still fully active? During device unbind, ttm_global_release() calls ttm_pool_mgr_fini() before debugfs_remove(ttm_debugfs_root). This newly added list_lru_destroy() frees the internal lru->node array and sets it to NULL. If a user concurrently reads the debugfs page_pool file, ttm_pool_debugfs_globals_show() executes, iterating over the global pools and calling list_lru_count(). Will this trigger a NULL pointer dereference when accessing lru->node[nid]? > shrinker_free(mm_shrinker); [Severity: High] This isn't a bug introduced by this patch, but does leaving shrinker_free() until the very end expose an empty shrinker_list to a concurrent shrinker invocation? The pools are removed from shrinker_list earlier in ttm_pool_type_fini(), but the shrinker remains active because shrinker_free(mm_shrinker) hasn't been called yet. The ttm_pool_synchronize_shrinkers() call only waits for in-flight shrinkers, it does not prevent new invocations. If the system experiences memory pressure in this window, the VM can call ttm_pool_shrink(), which completely lacks a list_empty() check on the shrinker_list: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_shrink() { ... pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list); list_move_tail(&pt->shrinker_list, &shrinker_list); ... } Will it blindly extract a pointer from the empty shrinker_list and dereference it, causing a list corruption and bogus pointer dereference? > WARN_ON(!list_empty(&shrinker_list)); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
