[PATCH v4] Memory leak error in qxl unbind

[PATCH v4] Memory leak error in qxl unbind

[Óscar Megía López]

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.

Here's the chain:

Bug 1: ttm_pool_type_init() does not check return from list_lru_init().

Fix: Check the return value from list_lru_init() and propagate the error.

Bug 2: ttm_pool_fini() does not destroy list lru with list_lru_destroy().

Fix: Add list_lru_destroy() after ttm_pool_type_fini().

Bug 3: ttm_pool_mgr_init() does not check ttm_pool_type_init() return
and does not free pool if returns error.

Fix: Move up shrinker_alloc(), check ttm_pool_type_init() return and free
pool types and shrinker if non-zero and return error.

Bug 4: ttm_pool_mgr_fini() does not destroy the list_lru.

Fix: Add list_lru_destroy() after ttm_pool_type_fini().

Fixed check for an empty shrinker_list.

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 | 135 ++++++++++++++++++++++++++++-----
 1 file changed, 115 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 278bbe7a11ad..183836f2df34 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -354,17 +354,23 @@ static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, int nid)
 }
 
 /* Initialize and add a pool type to the global shrinker list */
-static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
+static int ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
 			       enum ttm_caching caching, unsigned int order)
 {
+	int ret = 0;
+
 	pt->pool = pool;
 	pt->caching = caching;
 	pt->order = order;
-	list_lru_init(&pt->pages);
+	ret = list_lru_init(&pt->pages);
+	if (ret)
+		return ret;
 
 	spin_lock(&shrinker_lock);
 	list_add_tail(&pt->shrinker_list, &shrinker_list);
 	spin_unlock(&shrinker_lock);
+
+	return 0;
 }
 
 static enum lru_status pool_move_to_dispose_list(struct list_head *item,
@@ -438,6 +444,9 @@ static unsigned int ttm_pool_shrink(int nid, unsigned long num_to_free)
 	struct ttm_pool_type *pt;
 	unsigned int num_pages;
 
+	if ((shrinker_list.prev == &shrinker_list) && (shrinker_list.next == &shrinker_list))
+		return 0;
+
 	down_read(&pool_shrink_rwsem);
 	spin_lock(&shrinker_lock);
 	pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
@@ -1198,6 +1207,17 @@ void ttm_pool_fini(struct ttm_pool *pool)
 	 * that no shrinker is concurrently freeing pages from the pool.
 	 */
 	ttm_pool_synchronize_shrinkers();
+
+	for (i = 0; i < TTM_NUM_CACHING_TYPES; ++i) {
+		for (j = 0; j < NR_PAGE_ORDERS; ++j) {
+			struct ttm_pool_type *pt;
+
+			pt = ttm_pool_select_type(pool, i, j);
+			if (pt != &pool->caching[i].orders[j])
+				continue;
+			list_lru_destroy(&pt->pages);
+		}
+	}
 }
 EXPORT_SYMBOL(ttm_pool_fini);
 
@@ -1376,6 +1396,54 @@ static inline u64 ttm_get_node_memory_size(int nid)
 	return managed_pages * PAGE_SIZE;
 }
 
+static void ttm_pool_type_fini_and_list_lru_destroy(unsigned int nr)
+{
+	unsigned int i;
+
+	if (nr == 0)
+		return;
+
+	for (i = 0; i < nr; ++i) {
+		ttm_pool_type_fini(&global_write_combined[i]);
+		ttm_pool_type_fini(&global_uncached[i]);
+		ttm_pool_type_fini(&global_dma32_write_combined[i]);
+		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; ++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);
+	}
+
+}
+
+static void ttm_pool_type_fini_and_list_lru_destroy_partial(
+				struct ttm_pool_type *types[], unsigned int n)
+{
+	unsigned int k;
+
+	if (n == 0)
+		return;
+
+	for (k = 0; k < n; ++k)
+		ttm_pool_type_fini(types[k]);
+
+	/* 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 (k = 0; k < n; ++k)
+		list_lru_destroy(&types[k]->pages);
+}
+
 /**
  * ttm_pool_mgr_init - Initialize globals
  *
@@ -1386,6 +1454,8 @@ static inline u64 ttm_get_node_memory_size(int nid)
 int ttm_pool_mgr_init(unsigned long num_pages)
 {
 	unsigned int i;
+	int ret = 0;
+	struct ttm_pool_type *types_free[3];
 
 	int nid;
 	for_each_node(nid) {
@@ -1400,15 +1470,53 @@ int ttm_pool_mgr_init(unsigned long num_pages)
 	spin_lock_init(&shrinker_lock);
 	INIT_LIST_HEAD(&shrinker_list);
 
+	mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
+	if (!mm_shrinker)
+		return -ENOMEM;
+
 	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
-		ttm_pool_type_init(&global_write_combined[i], NULL,
+		ret = ttm_pool_type_init(&global_write_combined[i], NULL,
 				   ttm_write_combined, i);
-		ttm_pool_type_init(&global_uncached[i], NULL, ttm_uncached, i);
+		if (ret) {
+			ttm_pool_type_fini_and_list_lru_destroy(i);
+			shrinker_free(mm_shrinker);
+			return ret;
+		}
+
+		ret = ttm_pool_type_init(&global_uncached[i], NULL, ttm_uncached, i);
+		if (ret) {
+			types_free[0] = &global_write_combined[i];
+			ttm_pool_type_fini_and_list_lru_destroy_partial(types_free, 1);
 
-		ttm_pool_type_init(&global_dma32_write_combined[i], NULL,
+			ttm_pool_type_fini_and_list_lru_destroy(i);
+			shrinker_free(mm_shrinker);
+			return ret;
+		}
+
+		ret = ttm_pool_type_init(&global_dma32_write_combined[i], NULL,
 				   ttm_write_combined, i);
-		ttm_pool_type_init(&global_dma32_uncached[i], NULL,
+		if (ret) {
+			types_free[0] = &global_write_combined[i];
+			types_free[1] = &global_uncached[i];
+			ttm_pool_type_fini_and_list_lru_destroy_partial(types_free, 2);
+
+			ttm_pool_type_fini_and_list_lru_destroy(i);
+			shrinker_free(mm_shrinker);
+			return ret;
+		}
+
+		ret = ttm_pool_type_init(&global_dma32_uncached[i], NULL,
 				   ttm_uncached, i);
+		if (ret) {
+			types_free[0] = &global_write_combined[i];
+			types_free[1] = &global_uncached[i];
+			types_free[2] = &global_dma32_write_combined[i];
+			ttm_pool_type_fini_and_list_lru_destroy_partial(types_free, 3);
+
+			ttm_pool_type_fini_and_list_lru_destroy(i);
+			shrinker_free(mm_shrinker);
+			return ret;
+		}
 	}
 
 #ifdef CONFIG_DEBUG_FS
@@ -1422,10 +1530,6 @@ int ttm_pool_mgr_init(unsigned long num_pages)
 #endif
 #endif
 
-	mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
-	if (!mm_shrinker)
-		return -ENOMEM;
-
 	mm_shrinker->count_objects = ttm_pool_shrinker_count;
 	mm_shrinker->scan_objects = ttm_pool_shrinker_scan;
 	mm_shrinker->batch = TTM_SHRINKER_BATCH;
@@ -1443,16 +1547,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
  */
 void ttm_pool_mgr_fini(void)
 {
-	unsigned int i;
-
-	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
-		ttm_pool_type_fini(&global_write_combined[i]);
-		ttm_pool_type_fini(&global_uncached[i]);
-
-		ttm_pool_type_fini(&global_dma32_write_combined[i]);
-		ttm_pool_type_fini(&global_dma32_uncached[i]);
-	}
-
 	shrinker_free(mm_shrinker);
+	ttm_pool_type_fini_and_list_lru_destroy(NR_PAGE_ORDERS);
 	WARN_ON(!list_empty(&shrinker_list));
 }
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The patch adds error cleanup to `ttm_pool_mgr_init()` (freeing the shrinker and destroying pools), but the caller (`ttm_global_init()`) ignores its return value. This leads to double-frees and NULL pointer dereferences during teardown.
- [High] `ttm_pool_init()` ignores the newly added return value of `ttm_pool_type_init()`, resulting in guaranteed NULL pointer dereferences during teardown if initialization fails.
- [High] Lockless TOCTOU data race on `shrinker_list` emptiness check in `ttm_pool_shrink()` leads to invalid pointer dereference and memory corruption.
--

--- Patch [1]: [PATCH v4] Memory leak error in qxl unbind ---
commit ed37ece8c0ad50e1a16deff6653abfe2c17eb1a0
Author: Óscar Megía López <megia.oscar@gmail.com>
Subject: Memory leak error in qxl unbind

This commit addresses an OOM leak and refcount corruption in qxl unbind.
It adds proper cleanup and error propagation for list_lru_init() failures
during TTM pool initialization to ensure pool types are not left orphaned.

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..6821fadf1a8d0 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -374,17 +374,23 @@ static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, int nid)
>  }
>  
>  /* Initialize and add a pool type to the global shrinker list */
> -static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
> +static int ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool,
>  			       enum ttm_caching caching, unsigned int order)

[Severity: High]
Since this function now returns an error code instead of void, do all callers
check the new return value?

Looking at ttm_pool_init() in drivers/gpu/drm/ttm/ttm_pool.c:

    ttm_pool_type_init(pt, pool, i, j);

If ttm_pool_type_init() returns an error, pt->shrinker_list remains
zero-initialized. Later, when ttm_pool_fini() calls ttm_pool_type_fini(),
it unconditionally runs:

    list_del(&pt->shrinker_list);

Will this cause a NULL pointer dereference on the zero-initialized list head
during teardown?

[ ... ]
> @@ -458,6 +464,9 @@ static unsigned int ttm_pool_shrink(int nid, unsigned long num_to_free)
>  	struct ttm_pool_type *pt;
>  	unsigned int num_pages;
>  
> +	if ((shrinker_list.prev == &shrinker_list) && (shrinker_list.next == &shrinker_list))
> +		return 0;
> +
>  	down_read(&pool_shrink_rwsem);
>  	spin_lock(&shrinker_lock);
>  	pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);

[Severity: High]
Is it safe to check if shrinker_list is empty without holding shrinker_lock?

If the list becomes empty between this check and acquiring shrinker_lock
(for example, if a concurrent thread calls ttm_pool_type_fini() during
device teardown), list_first_entry() will execute on an empty list.

This would return a pointer to the list head itself cast to a struct
ttm_pool_type pointer, which could lead to memory corruption when
list_move_tail() and list_lru_walk_node() are invoked on this invalid
pointer.

Could this check be safely moved inside the critical section?

[ ... ]
> @@ -1506,15 +1576,53 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>  	spin_lock_init(&shrinker_lock);
>  	INIT_LIST_HEAD(&shrinker_list);
>  
> +	mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
> +	if (!mm_shrinker)
> +		return -ENOMEM;
> +
>  	for (i = 0; i < NR_PAGE_ORDERS; ++i) {
> -		ttm_pool_type_init(&global_write_combined[i], NULL,
> +		ret = ttm_pool_type_init(&global_write_combined[i], NULL,
>  				   ttm_write_combined, i);
> -		ttm_pool_type_init(&global_uncached[i], NULL, ttm_uncached, i);
> +		if (ret) {
> +			ttm_pool_type_fini_and_list_lru_destroy(i);
> +			shrinker_free(mm_shrinker);
> +			return ret;
> +		}

[Severity: Critical]
If ttm_pool_mgr_init() fails here, does the caller properly handle the
returned error?

If the caller (ttm_global_init()) ignores this error and continues execution,
mm_shrinker is left pointing to freed memory while execution proceeds.

Then during module unload, ttm_pool_mgr_fini() unconditionally calls:

    shrinker_free(mm_shrinker);
    ttm_pool_type_fini_and_list_lru_destroy(NR_PAGE_ORDERS);

Could this lead to a double-free of mm_shrinker and potential NULL pointer
dereferences on the partially destroyed list structures?

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

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

[Greg KH]

On Sun, Aug 09, 2026 at 06:53:14PM +0200, Ó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.
> 
> Here's the chain:
> 
> Bug 1: ttm_pool_type_init() does not check return from list_lru_init().
> 
> Fix: Check the return value from list_lru_init() and propagate the error.
> 
> Bug 2: ttm_pool_fini() does not destroy list lru with list_lru_destroy().
> 
> Fix: Add list_lru_destroy() after ttm_pool_type_fini().
> 
> Bug 3: ttm_pool_mgr_init() does not check ttm_pool_type_init() return
> and does not free pool if returns error.
> 
> Fix: Move up shrinker_alloc(), check ttm_pool_type_init() return and free
> pool types and shrinker if non-zero and return error.
> 
> Bug 4: ttm_pool_mgr_fini() does not destroy the list_lru.
> 
> Fix: Add list_lru_destroy() after ttm_pool_type_fini().
> 
> Fixed check for an empty shrinker_list.

Why is this not 4 different patches as part of a series?

> This patch depends on patch ("[PATCH v3] drm/qxl: fix use-after-free in
> qxl_irq_handler on PCI"), link [1] below.

That's not going to age well in a changelog text :(

thanks,

greg k-h