Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] If `amdgpu_vram_mgr_init()` fails early, structures like `ttm_resource_manager`, mutexes, and list heads remain zero-initialized. `amdgpu_vram_mgr_fini()` runs unconditionally and accesses these, causing NULL pointer dereferences and kernel panics. -- --- Patch [1]: [PATCH] drm/amdgpu: Fix use-after-free in amdgpu_vram_mgr_fini() on init failure --- commit 48887fb6b109845d375cd21461abab0a51f3196c Author: Arunpravin Paneer Selvam <[email protected]> drm/amdgpu: Fix use-after-free in amdgpu_vram_mgr_fini() on init failure The patch tries to fix a double-free or use-after-free when gpu_buddy_init() fails by guarding gpu_buddy_fini() with ttm_resource_manager_used(). Closes: https://sashiko.dev/#/patchset/[email protected]?part=1 > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > index 2a241a5b12c42..38529a904deb6 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > @@ -954,6 +954,9 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev) > struct ttm_resource_manager *man = &mgr->manager; > int ret; > struct amdgpu_vram_reservation *rsv, *temp; > + bool was_used; > + > + was_used = ttm_resource_manager_used(man); [Severity: High] This isn't a bug introduced by this patch, but does this function still crash if amdgpu_vram_mgr_init() fails early? If amdgpu_vram_mgr_init() fails (for example, if drmm_cgroup_register_region() returns an error), it bails out before ttm_resource_manager_init() can initialize the man->lru list heads or mutex_init() runs. When the error path unwinds through amdgpu_vram_mgr_fini(), the code unconditionally calls: ttm_resource_manager_set_used(man, false); ret = ttm_resource_manager_evict_all(&adev->mman.bdev, man); This traverses the uninitialized man->lru lists in ttm_bo_evict_first(), where list_empty() evaluates to false on zeroed memory (since NULL != head), causing list_first_entry() to return an invalid offset and eventually dereferencing a NULL pointer. If that doesn't crash, the function later unconditionally locks the zero-initialized mutex and iterates over the zero-initialized list head: mutex_lock(&mgr->lock); list_for_each_entry_safe(rsv, temp, &mgr->reservations_pending, blocks) Could we return early from amdgpu_vram_mgr_fini() if the manager wasn't initialized, perhaps by using the was_used flag to skip these teardown steps as well? > > ttm_resource_manager_set_used(man, false); > [ ... ] > @@ -969,7 +972,7 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev) > gpu_buddy_free_list(&mgr->mm, &rsv->allocated, 0); > kfree(rsv); > } > - if (!adev->gmc.is_app_apu) > + if (!adev->gmc.is_app_apu && was_used) > gpu_buddy_fini(&mgr->mm); > mutex_unlock(&mgr->lock); > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
