Re: [PATCH 08/10] amdgpu: add support for memory cgroups

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Evicted GPU buffers bypass memory cgroup limits because the memcg accounting flag is cleared during VRAM eviction and pages are never recharged upon re-validation.
- [Low] Missing dummy implementations for GPU page memory control functions when CONFIG_MEMCG is disabled causes a linker error.
--

commit 14a7ef9775310750fa8f3933b124ce94b537e5c7
Author: Dave Airlie <airlied@redhat.com>

amdgpu: add support for memory cgroups
    
This adds support for adding a obj cgroup to a buffer object,
and passing in the placement flags to make sure it's accounted
properly.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 8062b3d611574..1c8123c564ac1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -155,11 +155,14 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
>  			amdgpu_bo_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_GTT |
>  							AMDGPU_GEM_DOMAIN_CPU);
>  		}
> +		for (int i = 0; i < abo->placement.num_placement; i++)
> +			abo->placements[i].flags &= ~TTM_PL_FLAG_MEMCG;

[Severity: High]
Does clearing TTM_PL_FLAG_MEMCG during eviction allow memory cgroup limits to
be bypassed?

When a user forces eviction (for example, by allocating more VRAM), the flag
is cleared in amdgpu_evict_flags() and TTM moves the buffer to system memory.
The pages are then allocated via ttm_pool_alloc() with memcg_account set to
false, leaving them uncharged.

If the user later re-validates the buffer into GTT, ttm_bo_validate() calls
ttm_resource_compatible(). This function only checks the memory type and
contiguous flags, ignoring TTM_PL_FLAG_MEMCG.

This seems to allow the buffer to remain backed by uncharged system pages,
potentially letting a containerized workload exhaust host system memory.

>  		break;
>  	case TTM_PL_TT:
>  	case AMDGPU_PL_PREEMPT:
>  	default:
>  		amdgpu_bo_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_CPU);
> +		abo->placements[0].flags &= ~TTM_PL_FLAG_MEMCG;

[Severity: High]
Does this also allow bypassing memory cgroup limits for TTM_PL_TT evictions
to the CPU domain for the same reason?

[ ... ]

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4c682b91cbbee..625de4a287eb3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3032,6 +3032,7 @@ __always_inline struct obj_cgroup *current_obj_cgroup(void)
>  
>  	return rcu_dereference_check(root_mem_cgroup->nodeinfo[nid]->objcg, 1);
>  }
> +EXPORT_SYMBOL_GPL(current_obj_cgroup);
>  
>  struct obj_cgroup *get_obj_cgroup_from_folio(struct folio *folio)
>  {

[Severity: Low]
Will this series cause a linker error when CONFIG_DRM_TTM=y and CONFIG_MEMCG=n?

It looks like mem_cgroup_charge_gpu_page(), mem_cgroup_uncharge_gpu_page(),
and mem_cgroup_move_gpu_page_reclaim() are declared unconditionally in
include/linux/memcontrol.h.

However, their definitions in mm/memcontrol.c are skipped when CONFIG_MEMCG is
disabled.

Since ttm_pool_alloc_page() in drivers/gpu/drm/ttm/ttm_pool.c calls these
without ifdef guards, this seems to lead to undefined references during
linking.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706024122.853329-1-airlied@gmail.com?part=8