Re: [PATCH 10/10] xe: create a flag to enable memcg accounting for XE as well.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing memcg accounting for userspace dumb buffers created via xe_bo_dumb_create.
- [High] Page tables created asynchronously via workqueues are charged to the kworker's root memcg instead of the user's memcg.
- [High] BOs created with deferred backing bypass memcg accounting upon CPU page fault.
--

commit ebf9090c8281d875d2f980bb34d324c88a8b277f
Author: Maarten Lankhorst <dev@lankhorst.se>

xe: create a flag to enable memcg accounting for XE as well.

This adds support for memcg accounting to ttm object used by xe driver.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -56,6 +56,7 @@ static const struct ttm_place sys_placement_flags = {
>  	.flags = 0,
>  };
>  
> +/* TTM_PL_FLAG_MEMCG is not set, those placements are used for eviction */
>  static struct ttm_placement sys_placement = {

[Severity: High]
If userspace creates a buffer object with the deferred backing flag, mmaps
it, and accesses it via the CPU before the GPU validates it,
xe_bo_init_locked() assigns sys_placement as the initial placement. 
Since sys_placement lacks TTM_PL_FLAG_MEMCG, will a CPU page fault
(which calls xe_bo_fault_migrate() and ttm_bo_populate()) allocate memory
without charging the memcg? Does this allow userspace to bypass memcg
limits for deferred backing buffers?

>  	.num_placement = 1,
>  	.placement = &sys_placement_flags,
>  };

[ ... ]

> @@ -2384,6 +2385,9 @@ struct xe_bo *xe_bo_init_locked(struct xe_device *xe, struct xe_bo *bo,
>  	placement = (type == ttm_bo_type_sg ||
>  		     bo->flags & XE_BO_FLAG_DEFER_BACKING) ? &sys_placement :
>  		&bo->placement;
> +
> +	if (bo->flags & XE_BO_FLAG_ACCOUNTED)
> +		ttm_bo_set_cgroup(&bo->ttm, get_obj_cgroup_from_current());

[Severity: High]
When page tables are created asynchronously, such as from GPU page fault
handlers or rebinding operations executing in workqueues via
xe_pt_create(), get_obj_cgroup_from_current() will resolve to the
kworker's context. Does this cause these allocations to be incorrectly
charged to the root memcg instead of the user's memcg?

>  	err = ttm_bo_init_reserved(&xe->ttm, &bo->ttm, type,
>  				   placement, alignment,

[ ... ]

> @@ -3361,7 +3365,7 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>  	if (XE_IOCTL_DBG(xe, args->size & ~PAGE_MASK))
>  		return -EINVAL;
>  
> -	bo_flags = 0;
> +	bo_flags = XE_BO_FLAG_ACCOUNTED;
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING)
>  		bo_flags |= XE_BO_FLAG_DEFER_BACKING;

[Severity: High]
This correctly adds the accounting flag for standard GEM object creation, but
are dumb buffers created via xe_bo_dumb_create() missing this flag?

Dumb buffers are also allocated on behalf of userspace via
DRM_IOCTL_MODE_CREATE_DUMB:

xe_bo_dumb_create() {
    ...
    bo = xe_bo_create_user(xe, NULL, args->size,
                           DRM_XE_GEM_CPU_CACHING_WC,
                           XE_BO_FLAG_VRAM_IF_DGFX(xe_device_get_root_tile(xe)) |
                           XE_BO_FLAG_FORCE_WC |
                           XE_BO_FLAG_NEEDS_CPU_ACCESS, NULL);
}

Do these also need XE_BO_FLAG_ACCOUNTED to prevent a malicious or buggy
userspace application from bypassing memcg limits to exhaust host memory?

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