Re: [PATCH] ttm: Add error handling for ttm_pool_mgr_init()

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

Pre-existing issues:
- [High] Missing resource cleanup in `ttm_global_init()` error paths leads to use-after-free and memory leaks.
- [High] `ttm_pool_mgr_init()` NULL pointer dereference on memory-less NUMA nodes.
- [High] `ttm_pool_type_init()` ignores the return value of `list_lru_init()`, leading to NULL pointer dereference on allocation failure.
--

--- Patch [1]: [PATCH] ttm: Add error handling for ttm_pool_mgr_init() ---
commit 2f605d6f8238add861f5522786f57313330623f4
Author: Óscar Megía López <[email protected]>

ttm: Add error handling for ttm_pool_mgr_init()

This commit updates ttm_global_init() to check the return value of
ttm_pool_mgr_init(). If initialization fails, it now correctly jumps to the
error path to prevent the system from continuing with an invalid pool
manager state.

> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index d3bfb9a696a74..c880a04303635 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;
> +

[Severity: High]
This is a pre-existing issue, but looking into ttm_pool_mgr_init(), it
iterates over all possible nodes:

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_mgr_init() {
    ...
    for_each_node(nid) {
        if (!page_pool_size) {
            u64 node_size = ttm_get_node_memory_size(nid);
    ...
}

And ttm_get_node_memory_size() directly dereferences NODE_DATA(nid):

drivers/gpu/drm/ttm/ttm_pool.c:ttm_get_node_memory_size() {
    pg_data_t *pgdat = NODE_DATA(nid);

    for (zone_type = 0; zone_type < MAX_NR_ZONES; zone_type++)
        managed_pages +=
            zone_managed_pages(&pgdat->node_zones[zone_type]);
    ...
}

Can this trigger a NULL pointer dereference on systems with memory-less NUMA
nodes since it uses for_each_node() instead of
for_each_node_state(nid, N_MEMORY)?

[Severity: High]
This is also a pre-existing issue, but further down in the pool manager
initialization, ttm_pool_type_init() calls list_lru_init() without checking
its return value:

drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_type_init() {
    ...
    list_lru_init(&pt->pages);
    ...
}

Since list_lru_init() can fail and return an error code like -ENOMEM, does
ignoring this return value leave the list_lru structure in an invalid state?

Could this lead to a crash later in ttm_pool_type_give() when list_lru_add()
attempts to use the unallocated internal node arrays?

[Severity: High]
This isn't a bug introduced by this patch, but if alloc_page() fails later
in ttm_global_init(), we jump to this same out label.

Looking at the out block:

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

Does this leave the mm_shrinker registered in the global shrinker list since
ttm_pool_mgr_fini() is never called in this error path? 

If the driver module is subsequently unloaded, could the shrinker's callback
pointers point to freed module text and cause a use-after-free on the next
shrinker invocation?

>  	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=1