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

["Christian König" <christian.koenig@amd.com>]

First of all those patches doesn't have meaningful subject lines so I previously ignored them.

The subject should be something like "drm/ttm: fix memory leaks in ttm_pool".

On 8/11/26 21:42, Óscar Megía López wrote:
> I discovered an OOM after run the script below
> (I updated it and added a sleep to allow enough time for the cache to
> recover):
> 
> while [ 1 -eq 1 ]; do\
>     i=$((i+1)); echo 0000:00:01.0 > /sys/bus/pci/drivers/qxl/unbind;\
>     if (($i%1000==0)); then\
>         echo i=$i; free;\
>         grep nr_free_pages /proc/vmstat;\
>         grep -E "PageTables|VmallocUsed|Slab|Reclaimable" /proc/meminfo;\
>         sync; echo 3 > /proc/sys/vm/drop_caches;\
>         echo 1 > /proc/sys/vm/compact_memory;\
>         sleep 10s;\
>         free;\
>         grep nr_free_pages /proc/vmstat;\
>         grep -E "PageTables|VmallocUsed|Slab|Reclaimable" /proc/meminfo;\
>         uptime;\
>     fi;\
>     echo 0000:00:01.0 > /sys/bus/pci/drivers/qxl/bind;\
> done
> 
> The OOM isn't just a simple leak; it's a refcount corruption which renders
> the list_lru fix dead code after the first mid-init failure.
> 
> Fixed check if shrinker_list is empty holding shrinker_lock.
> Fixed check return value from ttm_pool_type_init and run
> ttm_pool_type_fini and list_lru_destroy for every pt initialized.
> Fixed change return value from ttm_pool_init to int.
> 
> This patch depends on patch ("[PATCH v3] drm/qxl: fix use-after-free in
> qxl_irq_handler on PCI"), link [1] below.
> 
> Assisted-by: OpenCode:1.17.18-Big Pickle/DeepSeek V4 Flash
> Assisted-by: claude.ai:Sonnet 5
> Link: https://lore.kernel.org/lkml/
> 20260727110212.64913-1-megia.oscar@gmail.com/ [1]
> Link: https://lore.kernel.org/dri-devel/
> 20260731053047.24503-1-megia.oscar@gmail.com/ [2]
> Cc: <stable@vger.kernel.org> # 7.1.0
> Fixes: 444e2a19d7fd ("ttm/pool: port to list_lru. (v2)")
> Signed-off-by: Óscar Megía López <megia.oscar@gmail.com>
> ---
> Changes in v2:
>  - Bug 1: ttm_global_init ignores ttm_pool_mgr_init() return.
>    If shrinker_alloc() fails under memory pressure, ttm_pool_mgr_init
>    returns -ENOMEM with pool types already initialized (64 list_lru_init
>    calls done). ttm_global_init ignored this and returned 0, leaving orphaned
>    pool types with a NULL mm_shrinker.
> 
>    Fix: Check ret from ttm_pool_mgr_init; if non-zero, goto out cleans up
>    refcount + debugfs.
> 
>  - Bug 2: ttm_pool_mgr_init leaks pool types on shrinker_alloc failure
>    If shrinker_alloc fails after all 64 pool types were list_lru_init'd,
>    the function returned -ENOMEM without undoing them. With Bug 1 now
>    triggering proper error handling, this undo is necessary.
> 
>    Fix: err_shrinker: label that finalizes + destroys all 64 pool types
>    before returning.
> 
> Changes in v3:
>  - Fix: "Unchecked list_lru_init() return value in ttm_pool_type_init()
>    causes a deterministic NULL pointer dereference in the newly added
>    error path."
>    Now check list_lru_init return value in ttm_pool_type_init() and
>    returns error if any.
> 
>  - Solved pre-existing issues reported by kernel test robot:
>    - [High] `ttm_pool_type_init()` ignores the return value of
>      `list_lru_init()`, leading to a NULL pointer dereference
>      if allocation fails.
> 
>      Fix: get return value from list_lru_init and return error if any.
> 
>    - [High] `ttm_pool_shrink()` assumes `shrinker_list` is never empty,
>      causing memory corruption and crashes during module unload
>      if triggered.
> 
>      Fix: Check if shrinker_list is empty and return 0 if it is empty.
> 
> Changes in v4:
>  - removed check return value in ttm_pool_mgr_init, now in new patch
>  ("[PATCH] ttm: Add error handling for ttm_pool_mgr_init()")
>  link [2] above.
>  - Fixed check empty shrinker_list.
>  - Check return value from ttm_pool_type_init.
>  - Move up shrinker_alloc.
>  - Deleted dput(backup_fault_inject.dname);
>  - Fixed issue [High] The patch introduces a use-after-free race condition
>    between `ttm_pool_type_fini()` and the active memory shrinker
>    `ttm_pool_shrink()` by calling `list_lru_destroy()` prematurely as
>    reported by kernel test robot.
> 
>    Fix: separate ttm_pool_type_fini and list_lru_destroy. Then, add
>    ttm_pool_synchronize_shrinkers between them.
> ---
>  drivers/gpu/drm/ttm/ttm_pool.c | 62 ++++++++++++++++++++++++----------
>  include/drm/ttm/ttm_pool.h     |  2 +-
>  2 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 278bbe7a11ad..88c0d33eed1a 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -437,13 +437,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;

That should probably be a bool.

>  
>  	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)) {

Clear NAK to such list hacks. Usually list_first_entry_or_null() is used for that.

> +		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;
> +
>  	num_pages = list_lru_walk_node(&pt->pages, nid, pool_move_to_dispose_list, &dispose, &num_to_free);
>  	num_pages *= 1 << pt->order;
>  
> @@ -1122,6 +1130,18 @@ long ttm_pool_backup(struct ttm_pool *pool, struct ttm_tt *tt,
>  	return shrunken ? shrunken : ret;
>  }
>  
> +/**
> + * ttm_pool_synchronize_shrinkers - Wait for all running shrinkers to complete.
> + *
> + * This is useful to guarantee that all shrinker invocations have seen an
> + * update, before freeing memory, similar to rcu.
> + */
> +static void ttm_pool_synchronize_shrinkers(void)
> +{
> +	down_write(&pool_shrink_rwsem);
> +	up_write(&pool_shrink_rwsem);
> +}
> +
>  /**
>   * ttm_pool_init - Initialize a pool
>   *
> @@ -1132,10 +1152,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,
>  		   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;
>  
>  	WARN_ON(!dev && ttm_pool_uses_dma_alloc(pool));
>  
> @@ -1152,23 +1175,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);
> +			if (ret)
> +				goto error;
> +
> +			initialized[n_initialized++] = pt;

That is just a horrible mess.

First of all the change to ttm_pool_type_init() must come first in the patch set or otherwise that stuff here won't even compile.

Then don't use a local array, that is *way* to big for the kernel stack.

That patch set here is not even remotely sufficient for inclusion in the upstream kernel.

Regards,
Christian.

>  		}
>  	}
> -}
> -EXPORT_SYMBOL(ttm_pool_init);
>  
> -/**
> - * ttm_pool_synchronize_shrinkers - Wait for all running shrinkers to complete.
> - *
> - * This is useful to guarantee that all shrinker invocations have seen an
> - * update, before freeing memory, similar to rcu.
> - */
> -static void ttm_pool_synchronize_shrinkers(void)
> -{
> -	down_write(&pool_shrink_rwsem);
> -	up_write(&pool_shrink_rwsem);
> +	return 0;
> +
> +error:
> +	for (k = 0; k < n_initialized; ++k)
> +		ttm_pool_type_fini(initialized[k]);
> +
> +	ttm_pool_synchronize_shrinkers();
> +
> +	for (k = 0; k < n_initialized; ++k)
> +		list_lru_destroy(&initialized[k]->pages);
> +
> +	return ret;
>  }
> +EXPORT_SYMBOL(ttm_pool_init);
>  
>  /**
>   * ttm_pool_fini - Cleanup a pool
> diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h
> index 26ee592e1994..66248323c2c1 100644
> --- a/include/drm/ttm/ttm_pool.h
> +++ b/include/drm/ttm/ttm_pool.h
> @@ -81,7 +81,7 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
>  		   struct ttm_operation_ctx *ctx);
>  void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt);
>  
> -void ttm_pool_init(struct ttm_pool *pool, struct device *dev,
> +int ttm_pool_init(struct ttm_pool *pool, struct device *dev,
>  		   int nid, unsigned int alloc_flags);
>  void ttm_pool_fini(struct ttm_pool *pool);
>