Re: [PATCH v6] Memory leak error in qxl unbind

[sashiko-bot@kernel.org]

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 <megia.oscar@gmail.com>

    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/20260927101026.45411-1-megia.oscar@gmail.com/ [1]
Link: https://lore.kernel.org/dri-devel/20260731053047.24503-1-megia.oscar@gmail.com/ [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/20260927103231.47692-1-megia.oscar@gmail.com?part=1