Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Incomplete Fix: Teardown path still crashes on VRAM init failure (Null Pointer Dereference) - [High] Use-After-Free due to skipped `ttm_device_fini` on `amdgpu_ttm_pools_init` failure -- --- Patch [1]: [PATCH v8 1/6] drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init() --- commit b8b31530c66aa0b0a491f59f8e3765cadbed921e Author: Thomas Hellström <[email protected]> drm/amdgpu: Fix init ordering in amdgpu_vram_mgr_init() The commit moves drmm_cgroup_register_region() after the initialization of the resource manager and buddy allocator in amdgpu_vram_mgr_init(). This prevents NULL pointer dereferences in amdgpu_vram_mgr_fini() when teardown is triggered following an initialization failure. 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 > index 2a241a5b12c42..ac3f71d77140a 100644 > --- 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) > if (err) > return err; > > + 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); [Severity: High] This isn't a bug introduced by this patch, but it looks like the teardown path will still crash if drmm_cgroup_register_region() or gpu_buddy_init() fails. Because adev->mman.initialized is set to true before amdgpu_vram_mgr_init() is called, an error here propagates up and triggers amdgpu_ttm_fini() during device teardown. amdgpu_ttm_fini() unconditionally calls teardown for managers that were never initialized: drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:amdgpu_ttm_fini() { ... amdgpu_gtt_mgr_fini(adev); amdgpu_preempt_mgr_fini(adev); ... } Because these managers were never initialized, man->lru is zeroed out. When ttm_resource_manager_evict_all() calls ttm_bo_evict_first(), it leads to ttm_resource_manager_first() executing: drivers/gpu/drm/ttm/ttm_resource.c:ttm_resource_manager_first() { ... list_move(&cursor->hitch.link, &man->lru[cursor->priority]); ... } Since man->lru[0] is zero-initialized, man->lru[0].next is NULL, which causes list_move() to dereference NULL and crash the kernel. Furthermore, if gpu_buddy_init() itself fails, it leaves mm->n_roots set to a non-zero value but frees or zeroes mm->roots: drivers/gpu/buddy.c:gpu_buddy_init() { ... mm->n_roots = hweight64(size); mm->roots = kmalloc_array(mm->n_roots, ...); if (!mm->roots) goto out_free_tree; ... } When amdgpu_vram_mgr_fini() then calls gpu_buddy_fini(), will it dereference the dangling or NULL mm->roots array? Could amdgpu_ttm_fini() be updated with additional guards to prevent cleaning up uninitialized managers? [Severity: High] This is a pre-existing issue, but I noticed another problem in the amdgpu_ttm_init() failure path that might be worth addressing. If amdgpu_ttm_pools_init() fails early in amdgpu_ttm_init(), it returns immediately: drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:amdgpu_ttm_init() { ... r = amdgpu_ttm_pools_init(adev); if (r) { ... return r; } adev->mman.initialized = true; ... } This means adev->mman.initialized remains false. However, ttm_device_init() was already called and successfully linked the device into the global list: drivers/gpu/drm/ttm/ttm_device.c:ttm_device_init() { ... list_add_tail(&bdev->device_list, &glob->device_list); ... } When amdgpu_ttm_fini() runs during teardown, it skips ttm_device_fini() because initialized is false: drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:amdgpu_ttm_fini() { ... if (!adev->mman.initialized) return; ... ttm_device_fini(&adev->mman.bdev); ... } This leaves a dangling list entry in ttm_glob.device_list. When ttm_global_swapout() is later triggered by memory pressure, will it traverse the list and dereference the freed amdgpu_device memory, causing a use-after-free? > + > 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
