Re: [PATCH v5 1/6] Memory leak error in qxl unbind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Missing `up_read(&pool_shrink_rwsem)` on the early return path in `ttm_pool_shrink()` causes a lock leak and subsequent deadlocks.
- [High] Callers of `ttm_pool_init()` are not updated to check its new `int` return value, leading to NULL pointer dereferences on initialization failure.
- [Low] Assigning the `void` return value of `ttm_pool_type_init()` to an `int` variable causes a compilation failure.
- [Low] Reinventing `list_empty()` with direct manual pointer comparisons.

Pre-existing issues:
- [High] Missing `list_lru_destroy()` in normal pool teardown paths (`ttm_pool_fini()` and `ttm_pool_mgr_fini()`) causes persistent memory leaks.
--

--- Patch [1]: [PATCH v5 1/6] Memory leak error in qxl unbind ---
commit 575a0762d1411d04c459167fe4dee31c7eee4aff
Author: Óscar Megía López <megia.oscar@gmail.com>

Memory leak error in qxl unbind

This commit attempts to fix a refcount corruption and memory leak in qxl
unbind by changing ttm_pool_init to return an error, checking for an empty
shrinker_list, and cleaning up list_lru structures on init failure.

Link: https://lore.kernel.org/lkml/20260727110212.64913-1-megia.oscar@gmail.com/
Link: https://lore.kernel.org/dri-devel/20260731053047.24503-1-megia.oscar@gmail.com/

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 1bf37023fed66..73284497cc785 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -457,13 +457,21 @@ static unsigned int ttm_pool_shrink(int nid, unsigned long num_to_free)
>  	LIST_HEAD(dispose);
>  	struct ttm_pool_type *pt;
>  	unsigned int num_pages;
> +	int empty = 0;
>  
>  	down_read(&pool_shrink_rwsem);
>  	spin_lock(&shrinker_lock);
> -	pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
> -	list_move_tail(&pt->shrinker_list, &shrinker_list);
> +	if ((shrinker_list.prev == &shrinker_list) && (shrinker_list.next == &shrinker_list)) {

[Severity: Low]
Could we use list_empty(&shrinker_list) here instead of manually checking
the prev and next pointers?

> +		empty = 1;
> +	} else {
> +		pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
> +		list_move_tail(&pt->shrinker_list, &shrinker_list);
> +	}
>  	spin_unlock(&shrinker_lock);
>  
> +	if (empty)
> +		return 0;

[Severity: High]
Does this early return leak the pool_shrink_rwsem read lock?

The down_read(&pool_shrink_rwsem) is acquired at the beginning of the
function, but if the shrinker_list is empty, this path returns early
without calling up_read().

This can cause any subsequent attempt to acquire a write lock (such as in
ttm_pool_synchronize_shrinkers during module unload) to block indefinitely
and deadlock the system.

[ ... ]

> @@ -1231,10 +1251,13 @@ long ttm_pool_backup(struct ttm_pool *pool, struct ttm_tt *tt,
>   *
>   * Initialize the pool and its pool types.
>   */
> -void ttm_pool_init(struct ttm_pool *pool, struct device *dev,
> +int ttm_pool_init(struct ttm_pool *pool, struct device *dev,

[Severity: High]
Are the callers of ttm_pool_init() updated to check this new int return
value?

For example, ttm_device_init() calls ttm_pool_init() but ignores the return
value. If ttm_pool_init() fails under memory pressure and cleans up the
lists, proceeding with driver initialization can cause a NULL pointer
dereference when subsequent allocations attempt to use the destroyed LRU
lists.

>  		   int nid, unsigned int alloc_flags)
>  {
> -	unsigned int i, j;
> +	unsigned int i, j, k;
> +	int ret;
> +	struct ttm_pool_type *initialized[TTM_NUM_CACHING_TYPES * NR_PAGE_ORDERS];
> +	unsigned int n_initialized = 0;

[ ... ]

> @@ -1251,23 +1274,28 @@ void ttm_pool_init(struct ttm_pool *pool, struct device *dev,
>  			if (pt != &pool->caching[i].orders[j])
>  				continue;
>  
> -			ttm_pool_type_init(pt, pool, i, j);
> +			ret = ttm_pool_type_init(pt, pool, i, j);

[Severity: Low]
Does this assignment cause a build failure?

It appears ttm_pool_type_init() is still defined as returning void, so
assigning its result to the int ret variable will break the build.

> +			if (ret)
> +				goto error;
> +
> +			initialized[n_initialized++] = pt;
>  		}
>  	}
>  
> +	return 0;
> +
> +error:
> +	for (k = 0; k < n_initialized; ++k)
> +		ttm_pool_type_fini(initialized[k]);

[Severity: High]
This is a pre-existing issue, but does ttm_pool_type_fini() fail to destroy
the list_lru structures?

While ttm_pool_type_fini() drains the LRU via list_lru_walk(), it doesn't
call list_lru_destroy(&pt->pages). This causes a persistent memory leak of
the node arrays on every device unbind or module unload where the normal
pool teardown paths (like ttm_pool_fini and ttm_pool_mgr_fini) are used.

> +
> +	ttm_pool_synchronize_shrinkers();
> +
> +	for (k = 0; k < n_initialized; ++k)
> +		list_lru_destroy(&initialized[k]->pages);
> +
> +	return ret;
>  }
> +EXPORT_SYMBOL(ttm_pool_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811194224.121597-1-megia.oscar@gmail.com?part=1