Re: [PATCH v8 6/6] drm/amdgpu: Wire up dmem cgroup reclaim for VRAM manager

[sashiko-bot@kernel.org]

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 <thomas.hellstrom@linux.intel.com>

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/20260723100350.16895-1-thomas.hellstrom@linux.intel.com?part=6