Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] ttm_resource_manager_set_dmem_region(man, NULL) fails to prevent a Use-After-Free in the reclaim callback when the dmem region outlives the resource manager. - [High] AMDGPU's eviction_valuable callback dereferences the `place` pointer without checking for NULL, which crashes during cgroup reclaim. - [High] Incorrect check in `ttm_resource_free()` causes cgroup charge leaks when a region is detached, or potential crashes when a region is attached post-allocation. --
--- Patch [4]: [PATCH v8 4/6] drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller --- commit 2a3c858c74bfd2816b989356ca73920f98d725df Author: Thomas Hellström <[email protected]> Subject: drm/ttm: Hook up a cgroup-aware reclaim callback for the dmem controller This commit introduces a cgroup-aware reclaim callback for the TTM dmem controller to evict buffer objects based on cgroup pools. It adds a new sleeping lock mode for the LRU walker and provides functions to associate dmem regions with resource managers. > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c [ ... ] > @@ -515,12 +515,20 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, > struct ttm_buffer_object * [ ... ] > + /* > + * evict_walk->place is NULL in cgroup drain mode. Drivers' > + * eviction_valuable() callbacks must handle a NULL place, treating it > + * as "any placement": the TTM base implementation already does so via > + * ttm_resource_intersects(). > + */ > if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, > evict_walk->place)) > return 0; [Severity: High] Will this crash in AMDGPU when it evaluates KFD fences? While the TTM base implementation handles a NULL place, the AMDGPU implementation does not seem to do so. In amdgpu_ttm_bo_eviction_valuable(), the place pointer is unconditionally dereferenced: amdgpu_ttm_bo_eviction_valuable() if (amdkfd_fence_check_mm(f, current->mm) && !(place->flags & TTM_PL_FLAG_CONTIGUOUS)) return false; Does the driver callback need to be updated to handle the NULL place during cgroup drain, or should TTM provide a dummy place? [ ... ] > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c > b/drivers/gpu/drm/ttm/ttm_resource.c > --- a/drivers/gpu/drm/ttm/ttm_resource.c > +++ b/drivers/gpu/drm/ttm/ttm_resource.c [ ... ] > @@ -953,3 +953,55 @@ void ttm_resource_manager_create_debugfs(struct > ttm_resource_manager *man, [ ... ] > +void ttm_resource_manager_set_dmem_region(struct ttm_resource_manager *man, > + struct dmem_cgroup_region *region) > +{ > + if (!IS_ERR(region)) > + man->cg = region; [Severity: High] Does setting the region to NULL properly detach the reclaim callback? When ttm_resource_manager_set_dmem_region() is called with a NULL region to detach it before teardown, it clears man->cg, but it doesn't appear to unregister the callback from the cgroup region. If the manager is destroyed before the region, could background reclaim or sysfs writes to dmem.max invoke the callback with a dangling priv pointer? Additionally, can modifying man->cg here cause cgroup charge leaks when resources are freed? Looking at ttm_resource_free(), the uncharge path checks man->cg instead of the resource's pool: ttm_resource_free() pool = (*res)->css; man = ttm_manager_type(bo->bdev, (*res)->mem_type); man->func->free(man, *res); *res = NULL; if (man->cg) dmem_cgroup_uncharge(pool, bo->base.size); If a region is detached (man->cg becomes NULL) after a resource was allocated, it skips uncharging a valid pool pointer, permanently leaking the charge. Conversely, if man->cg is attached after allocation (so pool is NULL), it might call dmem_cgroup_uncharge() with a NULL pool. Should ttm_resource_free() check if the resource itself has a valid css pool rather than checking man->cg? > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
