[PATCH v6] Memory leak error in qxl unbind

[PATCH v6] 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 udevadm settle to allow the cache to
recover):

i=0;\
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 loops=$i; free;\
        grep nr_free_pages /proc/vmstat;\
        grep -E "VmallocUsed|Slab|Reclaimable|SUnreclaim" /proc/meminfo;\
        sync; echo 3 > /proc/sys/vm/drop_caches;\
        echo 1 > /proc/sys/vm/compact_memory;\
        echo "running udevadm settle;";\
        udevadm settle;\
        free;\
        grep nr_free_pages /proc/vmstat;\
        grep -E "VmallocUsed|Slab|Reclaimable|SUnreclaim" /proc/meminfo;\
        uptime;\
    fi;\
    echo 0000:00:01.0 > /sys/bus/pci/drivers/qxl/bind;\
done

ttm_pool_mgr_fini() does not destroy the list lru leaving memory leak.

Fix: Add list_lru_destroy() after ttm_pool_type_fini().

This patch depends on patch ("[PATCH v4] 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: https://claude.ai:Sonnet 5
Assisted-by: https://gemini.google.com:3.6 Flash
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]
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.

Changes in v5:
 - In ttm_pool_init() return int and check return value from
   ttm_pool_type_init and propagate error. Free pt and pg->pages
   if returns error.
 - In ttm_pool_mgr_init() free previous ttm_pool_type_init() if returns
   error.
 - In ttm_global_init() check return value from ttm_pool_mgr_init() and
   propagate.

Changes in v6:
 - Add list_lru_destroy in ttm_pool_mgr_fini to avoid memory leak.
---
 drivers/gpu/drm/ttm/ttm_pool.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 278bbe7a11ad..09fdfe45e39f 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -1453,6 +1453,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);
+	}
+
 	shrinker_free(mm_shrinker);
 	WARN_ON(!list_empty(&shrinker_list));
 }
-- 
2.55.0

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

[sashiko-bot]

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