Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Memory Leak and Use-After-Free of cgroup region and gpu_buddy on probe failure - [High] Permanent memory charge leak on VRAM eviction failure - [Medium] Commit Message and Implementation Mismatch regarding cgroup region registration --
--- Patch [6]: [PATCH v8 6/6] drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager --- commit 9102e928a842cf958ce390fd437331e21fe61836 Author: Thomas Hellström <[email protected]> drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager Register the VRAM manager with the dmem cgroup reclaim infrastructure so that lowering dmem.max below current VRAM usage triggers TTM eviction rather than failing with -EBUSY. > Use drmm_cgroup_register_region() so that the region is automatically > unregistered at DRM device release, after drm_dev_unplug() has already > made drm_dev_enter() return false. [Severity: Medium] Is this description still accurate? The commit message claims that drmm_cgroup_register_region() is used to automatically tie the cgroup region's lifecycle to the DRM device release, but the code implements manual registration using dmem_cgroup_register_region(). The v8 changelog at the bottom accurately describes why this manual approach was chosen, but the main body of the commit message appears to still have the older description. > 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 [ ... ] > @@ -917,6 +939,7 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev) > { > struct amdgpu_vram_mgr *mgr = &adev->mman.vram_mgr; > struct ttm_resource_manager *man = &mgr->manager; > + struct dmem_cgroup_region *cg; > int err; > > ttm_resource_manager_init(man, &adev->mman.bdev, > @@ -933,12 +956,17 @@ int amdgpu_vram_mgr_init(struct amdgpu_device *adev) > if (err) > return err; > > - man->cg = drmm_cgroup_register_region(adev_to_drm(adev), "vram", > - &(struct dmem_cgroup_init){ > + cg = dmem_cgroup_register_region(&(struct dmem_cgroup_init){ > .size = > adev->gmc.real_vram_size, > - }); > - if (IS_ERR(man->cg)) > - return PTR_ERR(man->cg); > + .ops = > &amdgpu_vram_mgr_dmem_ops, > + .reclaim_priv = man, > + }, > + "drm/%s/vram", > adev_to_drm(adev)->unique); > + if (IS_ERR(cg)) > + return PTR_ERR(cg); [Severity: High] Does this early return leak the gpu_buddy allocator? It looks like gpu_buddy_init() succeeds just before this, but gpu_buddy_fini() is not called if dmem_cgroup_register_region() fails. Also, if amdgpu_vram_mgr_init() succeeds but a subsequent step in the GMC block's sw_init fails: amdgpu_device_ip_init() { r = adev->ip_blocks[i].version->funcs->sw_init(&adev->ip_blocks[i]); if (r) { goto init_failed; } adev->ip_blocks[i].status.sw = true; } The status.sw flag is not set, causing amdgpu_device_ip_fini() to skip sw_fini and bypass all cleanup. This would permanently leak the gpu_buddy structures and the registered cgroup region. Because the cgroup region remains active and its reclaim_priv points to the manager inside the freed amdgpu_device structure, could a subsequent cgroup memory pressure event trigger a use-after-free on the dangling pointer? > + > + mgr->cg_region = cg; > + ttm_resource_manager_set_dmem_region(man, cg); > > ttm_set_driver_manager(&adev->mman.bdev, TTM_PL_VRAM, &mgr->manager); > ttm_resource_manager_set_used(man, true); [ ... ] > @@ -963,6 +991,19 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev) > ttm_resource_manager_set_used(man, false); > > ret = ttm_resource_manager_evict_all(&adev->mman.bdev, man); > + > + /* > + * Unregister the dmem cgroup region regardless of the evict_all() > + * result and before any further teardown. This drains in-flight > + * reclaim callbacks and blocks new ones, so no reclaim can reference > + * the manager once we start freeing it. It must run after evict_all() > + * so that ttm_resource_free() can still uncharge via man->cg during > + * eviction. Clear man->cg afterwards. > + */ > + dmem_cgroup_unregister_region(mgr->cg_region); > + mgr->cg_region = NULL; > + ttm_resource_manager_set_dmem_region(man, NULL); > + > if (ret) > return; [Severity: High] Does this cause a permanent memory charge leak if eviction fails? The unregistration happens and man->cg is cleared before returning early on eviction failure. If eviction fails, buffer objects are left alive in VRAM. When these buffer objects are eventually freed, ttm_resource_free() relies on man->cg being non-NULL to know whether it should call dmem_cgroup_uncharge(): ttm_resource_free() { if (man->cg) dmem_cgroup_uncharge(pool, bo->base.size); } Since man->cg was already cleared, the uncharge is skipped, which appears to leak the cgroup charge and its css reference permanently. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
