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/
> [email protected]/ [1]
> Link: https://lore.kernel.org/dri-devel/
> [email protected]/ [2]
> Cc: <[email protected]> # 7.1.0
> Fixes: 444e2a19d7fd ("ttm/pool: port to list_lru. (v2)")
> Signed-off-by: Óscar Megía López <[email protected]>
> ---
> 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);
>
