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 <[email protected]> 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/[email protected]?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/[email protected]?part=1
