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

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

On 6/28/26 17:40, Óscar Megía López wrote:
> I discovered OOM after run script below around two hours in my computer:
> 
> while :; do
>     echo [pci qxl id] > /sys/bus/pci/drivers/qxl/unbind
>     echo [pci qxl id] > /sys/bus/pci/drivers/qxl/bind
> done
> 
> If you run 10000 times above script kmemleak does not report any
> memory leak, but if you run above script about two hours several
> OOM ocurs and at the end kernel panic.
> 
> The OOM isn't just a simple leak; it's a refcount corruption that renders
> the list_lru fix dead code after the first mid-init failure.
> 
> Here's the chain:
> 
> 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.
> 
> You must apply the patch from the link "[PATCH v2] drm/qxl: fix
> use-after-free to qxl_irq_handler in PCI mode" before testing.
> If you don't apply this patch, you will get a UAF error when running
> the script above.
> 
> Assisted-by: OpenCode:1.17.8-Big Pickle
> Link: https://lore.kernel.org/virtualization/20260627105445.89827-2-megia.oscar@gmail.com/T/#u
> Signed-off-by: Óscar Megía López <megia.oscar@gmail.com>
> ---
>  drivers/gpu/drm/ttm/ttm_device.c |  5 ++++-
>  drivers/gpu/drm/ttm/ttm_pool.c   | 37 ++++++++++++++++++++++++++------
>  include/drm/ttm/ttm_pool.h       |  2 ++
>  3 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index d3bfb9a696a7..c880a0430363 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -96,7 +96,10 @@ static int ttm_global_init(void)
>                 >> PAGE_SHIFT;
>         num_dma32 = min(num_dma32, 2UL << (30 - PAGE_SHIFT));
> 
> -       ttm_pool_mgr_init(num_pages);
> +       ret = ttm_pool_mgr_init(num_pages);
> +       if (ret)
> +               goto out;
> +

Please put that into a separate patch.

>         ttm_tt_mgr_init(num_pages, num_dma32);
> 
>         glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32 |
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 278bbe7a11ad..e0c82804a07d 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -1198,6 +1198,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);
> 
> @@ -1386,6 +1397,7 @@ 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;
> 
>         int nid;
>         for_each_node(nid) {
> @@ -1423,8 +1435,10 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>  #endif
> 
>         mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");
> -       if (!mm_shrinker)
> -               return -ENOMEM;
> +       if (!mm_shrinker) {
> +               ret = -ENOMEM;
> +               goto err_shrinker;
> +       }
> 
>         mm_shrinker->count_objects = ttm_pool_shrinker_count;
>         mm_shrinker->scan_objects = ttm_pool_shrinker_scan;
> @@ -1434,6 +1448,10 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>         shrinker_register(mm_shrinker);
> 
>         return 0;
> +
> +err_shrinker:
> +       ttm_pool_type_fini_and_list_lru_destroy();
> +       return ret;

Instead of the error handling move shrinker_alloc() before initializing the pool types.

>  }
> 
>  /**
> @@ -1442,17 +1460,24 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>   * Cleanup the global pools and unregister the MM shrinker.
>   */
>  void ttm_pool_mgr_fini(void)
> +{
> +       shrinker_free(mm_shrinker);
> +       ttm_pool_type_fini_and_list_lru_destroy();
> +       WARN_ON(!list_empty(&shrinker_list));
> +}
> +
> +void ttm_pool_type_fini_and_list_lru_destroy(void)
>  {
>         unsigned int i;
> 
>         for (i = 0; i < NR_PAGE_ORDERS; ++i) {
>                 ttm_pool_type_fini(&global_write_combined[i]);
> +               list_lru_destroy(&global_write_combined[i].pages);
>                 ttm_pool_type_fini(&global_uncached[i]);
> -
> +               list_lru_destroy(&global_uncached[i].pages);
>                 ttm_pool_type_fini(&global_dma32_write_combined[i]);
> +               list_lru_destroy(&global_dma32_write_combined[i].pages);
>                 ttm_pool_type_fini(&global_dma32_uncached[i]);
> +               list_lru_destroy(&global_dma32_uncached[i].pages);
>         }
> -
> -       shrinker_free(mm_shrinker);
> -       WARN_ON(!list_empty(&shrinker_list));
>  }
> diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h
> index 26ee592e1994..bda8e816a206 100644
> --- a/include/drm/ttm/ttm_pool.h
> +++ b/include/drm/ttm/ttm_pool.h
> @@ -97,4 +97,6 @@ int ttm_pool_restore_and_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
>  int ttm_pool_mgr_init(unsigned long num_pages);
>  void ttm_pool_mgr_fini(void);
> 
> +void ttm_pool_type_fini_and_list_lru_destroy(void);

That function doesn't need to be exported and so doesn't belong into the header.

Regards,
Christian.

> +
>  #endif
> --
> 2.54.0
>