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 <[email protected]> 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/[email protected]?part=10
