Re: [PATCH v7 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 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 <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.

[ ... ]

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