Re: [PATCH v7 1/6] drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch resolves a panic when `drmm_cgroup_register_region()` fails, but leaves an identical teardown vulnerability if `gpu_buddy_init()` fails.
--

commit 3650035d068659ae5cf730d3debae46e3ebe9589
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init()

This commit fixes a kernel panic on the module load error path by moving
drmm_cgroup_register_region() after the list and buddy allocator are fully
initialized. This prevents the teardown path from dereferencing uninitialized
pointers if the cgroup registration fails.

Closes: https://sashiko.dev/#/patchset/20260428073116.15687-1-thomas.hellstrom@linux.intel.com?part=4

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c

[ ... ]

> @@ -935,6 +932,10 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev)
>  	err = gpu_buddy_init(&mgr->mm, man->size, PAGE_SIZE);
>  	if (err)
>  		return err;

[Severity: High]
This isn't a bug introduced by this patch, but while fixing the teardown
vulnerability for the cgroup registration, does this leave an identical
teardown vulnerability if gpu_buddy_init() fails?

If gpu_buddy_init() fails, it frees the memory but leaves dangling pointers
and a non-zero mm->n_roots:

drivers/gpu/buddy.c:gpu_buddy_init() {
    ...
out_free_roots:
    while (root_count--)
        gpu_block_free(mm, mm->roots[root_count]);
    kfree(mm->roots);
out_free_tree:
    while (i--)
        kfree(mm->free_trees[i]);
    kfree(mm->free_trees);
    return -ENOMEM;
}

Since adev->mman.initialized is already set to true by the caller
(amdgpu_ttm_init()), returning this error triggers the driver's teardown path
which unconditionally calls gpu_buddy_fini():

drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c:amdgpu_vram_mgr_fini() {
    ...
    if (!adev->gmc.is_app_apu)
        gpu_buddy_fini(&mgr->mm);
    ...
}

This would lead to a use-after-free, NULL pointer dereference, and
double-free when gpu_buddy_fini() iterates up to the non-zero mm->n_roots:

drivers/gpu/buddy.c:gpu_buddy_fini() {
    ...
    for (i = 0; i < mm->n_roots; ++i) {
        order = ilog2(size) - ilog2(mm->chunk_size);
        start = gpu_buddy_block_offset(mm->roots[i]);
    ...
}

Should gpu_buddy_init() clear mm->n_roots and zero the pointers on error,
or should amdgpu_vram_mgr_init() clear its state upon this failure?

> +	man->cg = drmm_cgroup_register_region(adev_to_drm(adev), "vram", adev->gmc.real_vram_size);
> +	if (IS_ERR(man->cg))
> +		return PTR_ERR(man->cg);
> +
>  	ttm_set_driver_manager(&adev->mman.bdev, TTM_PL_VRAM, &mgr->manager);
>  	ttm_resource_manager_set_used(man, true);
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703130541.2686-1-thomas.hellstrom@linux.intel.com?part=1