Re: [PATCH v5 6/6] ttm_pool_mgr_init() does not check return value

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup of `ttm_pool_mgr_init()` allocations when `alloc_page()` fails in `ttm_global_init()`.
--

--- Patch [6]: [PATCH v5 6/6] ttm_pool_mgr_init() does not check return value ---
commit 18452a40ead8211d272e9f32ad64ceec8ef1cf22
Author: Óscar Megía López <[email protected]>

ttm_pool_mgr_init() does not check return value

This commit fixes a missing return value check for ttm_pool_mgr_init() in
ttm_global_init(). It propagates the error by jumping to the out label if
initialization fails.

Cc: <[email protected]> # 7.1.0
Fixes: 444e2a19d7fd ("ttm/pool: port to list_lru. (v2)")

> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index d3bfb9a696a74..896b766712d04 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -96,7 +96,9 @@ 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;

[Severity: High]
This is a pre-existing issue, but does the error handling at the out label
properly clean up after a successful ttm_pool_mgr_init() call?

If the dummy_read_page allocation fails later in this function, we
jump to the out label returning -ENOMEM:

        glob->dummy_read_page = alloc_page(__GFP_ZERO);
        if (unlikely(glob->dummy_read_page == NULL)) {
                ret = -ENOMEM;
                goto out;
        }

However, the cleanup at the out label doesn't appear to call
ttm_pool_mgr_fini() or unregister the shrinker:

out:
        if (ret && ttm_debugfs_root)
                debugfs_remove(ttm_debugfs_root);
        if (ret)
                --ttm_glob_use_count;
        mutex_unlock(&ttm_global_mutex);
        return ret;

Since ttm_pool_mgr_init() registers a shrinker (mm_shrinker) and
initializes global spinlocks and lists:

        spin_lock_init(&shrinker_lock);
        INIT_LIST_HEAD(&shrinker_list);
        ...
        mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool");

Could this leave a registered shrinker behind on failure?

If ttm_global_init() is called again on a subsequent driver load, it would
unconditionally re-initialize shrinker_lock and shrinker_list. If the
originally leaked shrinker is running concurrently during memory reclaim,
could this cause lock or list corruption?

>  	ttm_tt_mgr_init(num_pages, num_dma32);
>  
>  	glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32 |

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6