Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Use-After-Free due to concurrent cgroup reclaim during probe error teardown. - [Medium] Memory leak of VRAM manager buddy allocator on cgroup region registration failure. --
commit 7beb8b077a31ff08f80ed7810a3913fd3a463579 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. [ ... ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > index 08f05c3aed1d5..9b9d738ba7941 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c [ ... ] > @@ -906,6 +906,28 @@ static const struct ttm_resource_manager_func > amdgpu_vram_mgr_func = { > .debug = amdgpu_vram_mgr_debug > }; > > +static const struct dmem_cgroup_ops amdgpu_vram_mgr_dmem_ops; > + > +static int amdgpu_vram_mgr_dmem_reclaim(struct dmem_cgroup_pool_state *pool, > + u64 target_bytes, void *priv) > +{ > + struct ttm_resource_manager *man = priv; > + struct amdgpu_device *adev = amdgpu_ttm_adev(man->bdev); > + int ret, idx; > + > + if (!drm_dev_enter(adev_to_drm(adev), &idx)) > + return -ENODEV; [Severity: High] Does this drm_dev_enter() check adequately protect against teardown during a probe error? If a failure occurs during amdgpu_pci_probe() (for instance in amdgpu_driver_load_kms()), drm_dev_unplug() is never called, so dev->unplugged remains false. The teardown path then proceeds to drm_dev_release() which does: drm_dev_release(dev) { if (dev->driver->release) dev->driver->release(dev); drm_managed_release(dev); ... } The driver release callback will destroy the VRAM manager and TTM structures. Only after this does drm_managed_release() unregister the cgroup region. If a cgroup memory reclaim occurs during this window, drm_dev_enter() will allow execution to proceed, and ttm_resource_manager_dmem_reclaim() will dereference the destroyed VRAM manager. Is there a way to prevent reclaim from accessing the manager during probe failure teardown? > + > + ret = ttm_resource_manager_dmem_reclaim(pool, target_bytes, priv); > + > + drm_dev_exit(idx); > + return ret; > +} [ ... ] > @@ -933,12 +956,16 @@ 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 = drmm_cgroup_register_region(adev_to_drm(adev), "vram", > + &(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, > + }); > + if (IS_ERR(cg)) > + return PTR_ERR(cg); [Severity: Medium] Does returning here leak the buddy allocator initialized earlier in this function via gpu_buddy_init()? If drmm_cgroup_register_region() fails, the error propagates out of amdgpu_ttm_init(), causing the GMC IP block's sw_init to fail. During driver teardown, amdgpu_device_ip_fini() skips calling sw_fini for IP blocks where status.sw is false. Since amdgpu_vram_mgr_fini() is skipped, it seems the buddy allocator memory allocated by gpu_buddy_init() will never be freed. Should this error path call gpu_buddy_fini() before returning? > + > + 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); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
