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

New issues:
- [Critical] Early return in `amdgpu_vram_mgr_fini()` on eviction failure skips `dmem_cgroup_unregister_region()`, leading to a use-after-free.
- [Medium] The cgroup region name is hardcoded to 'vram', breaking the device-specific namespace prefix and causing collisions on multi-GPU setups.

Pre-existing issues:
- [High] Unconditional cleanup of the buddy allocator causes a UAF/NULL dereference if `gpu_buddy_init()` fails during initialization.
--

commit 869c423a8a9487b5c3e7321e4f9a0e14f69318b4
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..2250bab0970dd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
[ ... ]
> @@ -933,12 +938,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){
> -						.size = adev->gmc.real_vram_size,
> -					      });
> -	if (IS_ERR(man->cg))
> -		return PTR_ERR(man->cg);
> +	cg = dmem_cgroup_register_region(&(struct dmem_cgroup_init){
> +					     .size = adev->gmc.real_vram_size,
> +					     .ops = &amdgpu_vram_mgr_dmem_ops,
> +					     .reclaim_priv = man,
> +					 }, "vram");

[Severity: Medium]
Does this hardcoded "vram" string break the device-specific namespace prefix?

Before this patch, drmm_cgroup_register_region() automatically prefixed the
cgroup region name with the DRM device's unique identifier. On systems with
multiple AMD GPUs, won't using just "vram" here cause all GPUs to register a
region with the exact same name, breaking userspace tools' ability to
distinguish between the GPUs' memory cgroups?

[ ... ]
> @@ -966,6 +975,16 @@ void amdgpu_vram_mgr_fini(struct amdgpu_device *adev)
>  	ret = ttm_resource_manager_evict_all(&adev->mman.bdev, man);
>  	if (ret)
>  		return;
>  
> +	/*
> +	 * Drain any in-flight dmem cgroup reclaim callbacks and remove the
> +	 * region from the global list.  This must happen after evict_all()
> +	 * so that ttm_resource_free() can still uncharge via man->cg while
> +	 * BOs are being evicted.
> +	 */
> +	dmem_cgroup_unregister_region(mgr->cg_region);

[Severity: Critical]
If ttm_resource_manager_evict_all() fails (for example, due to pinned BOs)
and returns an error, the early return above skips the newly added
dmem_cgroup_unregister_region() call.

Could this permanently leak the region and leave a dangling pointer as its
reclaim callback argument? When the DRM device is eventually released and
the amdgpu_device struct is freed, any subsequent memory pressure triggering
dmem cgroup reclaim would dereference the freed man pointer, causing a
kernel use-after-free.

> +	mgr->cg_region = NULL;
> +	man->cg = NULL;
> +
>  	mutex_lock(&mgr->lock);
>  	list_for_each_entry_safe(rsv, temp, &mgr->reservations_pending, blocks)
>  		kfree(rsv);
[ ... ]
>  	list_for_each_entry_safe(rsv, temp, &mgr->reserved_pages, blocks) {
>  		gpu_buddy_free_list(&mgr->mm, &rsv->allocated, 0);
>  		kfree(rsv);
>  	}
>  	if (!adev->gmc.is_app_apu)
>  		gpu_buddy_fini(&mgr->mm);

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditional
cleanup cause a use-after-free or NULL pointer dereference if
gpu_buddy_init() failed during initialization?

If gpu_buddy_init() fails, it returns an error and frees its internal arrays
without setting them to NULL. The driver error path then calls
amdgpu_device_fini_sw(), which triggers amdgpu_vram_mgr_fini().

Since mm->n_roots was populated before the allocation failure, won't
gpu_buddy_fini() iterate over mm->n_roots and dereference the uninitialized
or NULL mm->roots pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611173301.17473-1-thomas.hellstrom@linux.intel.com?part=6