Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Double put of `over_high_pool` reference on the `ttm_resource_alloc` error path leading to Use-After-Free. - [High] Swallowed `-EDEADLK` error in `ttm_bo_proactive_evict_high()` breaks the WW mutex backoff protocol. - [Medium] Proactive reclaim bypasses parent `dmem.high` limits because the ancestor chain is not evaluated. --
--- Patch [1]: [PATCH v7] cgroup/dmem: implement dmem.high soft limit with proactive reclaim --- commit cf68cf477adb57880cd824c91aad57538df44a76 Author: Qiliang Yuan <[email protected]> cgroup/dmem: implement dmem.high soft limit with proactive reclaim This commit implements dmem.high as a proper soft limit following the memory.high semantics. The limit is checked in the charge path on every successful allocation, and proactive reclaim is triggered when usage crosses the threshold. Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/all/[email protected] > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index bcd76f6bb7f02..bdfcfe5c7d1e2 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c [ ... ] > @@ -553,6 +556,38 @@ static const struct ttm_lru_walk_ops ttm_evict_walk_ops > = { > .process_bo = ttm_bo_evict_cb, > }; > > +/* > + * Proactive reclaim: evict one BO from a cgroup that exceeds its dmem.high > + * soft limit. Called after a successful charge that pushed usage over the > + * high threshold. Best-effort; allocation already succeeded (soft limit). > + */ > +static void ttm_bo_proactive_evict_high(struct ttm_device *bdev, > + struct ttm_resource_manager *man, > + const struct ttm_place *place, > + struct ttm_buffer_object *evictor, > + struct ttm_operation_ctx *ctx, > + struct ww_acquire_ctx *ticket, > + struct dmem_cgroup_pool_state > *over_high_pool) > +{ > + struct ttm_bo_evict_walk evict_walk = { > + .walk = { > + .ops = &ttm_evict_walk_ops, > + .arg = { > + .ctx = ctx, > + .ticket = ticket, > + .trylock_only = !ticket, > + } > + }, > + .place = place, > + .evictor = evictor, > + .res = NULL, > + .limit_pool = over_high_pool, > + .try_high = true, > + }; > + > + ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man, 1); > +} [Severity: High] Does this code silently swallow WW mutex backoff requests? When ttm_lru_walk_for_evict() is called with a ticket, it can return -EDEADLK if a cyclic dependency is detected. Because ttm_bo_proactive_evict_high() has a void return type and ignores the return value of ttm_lru_walk_for_evict(), the top-level caller (ttm_bo_alloc_resource) is not notified of the deadlock and will fail to execute the required backoff sequence. Could this regression lead to a hard deadlock? [ ... ] > @@ -737,7 +767,17 @@ static int ttm_bo_alloc_resource(struct > ttm_buffer_object *bo, > continue; > > may_evict = (force_space && place->mem_type != TTM_PL_SYSTEM); > - ret = ttm_resource_alloc(bo, place, res, force_space ? > &limit_pool : NULL); > + { > + struct dmem_cgroup_pool_state *over_high_pool = NULL; > + > + ret = ttm_resource_alloc(bo, place, res, > + force_space ? &limit_pool : > NULL, > + &over_high_pool); > + if (!ret && over_high_pool) > + ttm_bo_proactive_evict_high(bdev, man, place, > bo, > + ctx, ticket, > over_high_pool); > + dmem_cgroup_pool_state_put(over_high_pool); [Severity: High] Is there a use-after-free risk for over_high_pool on the error path? If ttm_resource_alloc() encounters an error after successfully charging the cgroup, it calls dmem_cgroup_pool_state_put(*ret_over_high_pool), but it leaves the pointer value intact: drivers/gpu/drm/ttm/ttm_resource.c:ttm_resource_alloc() { ... if (ret) { if (pool) dmem_cgroup_uncharge(pool, bo->base.size); if (ret_over_high_pool) dmem_cgroup_pool_state_put(*ret_over_high_pool); return ret; } ... } When it returns the error code, the caller (ttm_bo_alloc_resource) will proceed to unconditionally call dmem_cgroup_pool_state_put(over_high_pool) on the exact same pointer, causing a double-free regression. Should ttm_resource_alloc() clear *ret_over_high_pool after putting the reference on its error path? > + } > if (ret) { > if (ret != -ENOSPC) { [ ... ] > diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c > index 4753a67d0f0f2..b322c8a7e2a67 100644 > --- a/kernel/cgroup/dmem.c > +++ b/kernel/cgroup/dmem.c [ ... ] > @@ -685,6 +748,18 @@ int dmem_cgroup_try_charge(struct dmem_cgroup_region > *region, u64 size, > goto err; > } > > + /* > + * Charge succeeded. Check if usage now exceeds the soft high limit so > + * the caller can trigger proactive reclaim to bring the cgroup back > + * under its dmem.high threshold. > + */ > + if (ret_over_high_pool && > + page_counter_read(&pool->cnt) > READ_ONCE(pool->cnt.high)) { > + *ret_over_high_pool = pool; [Severity: Medium] Will this bypass proactive reclaim for parent cgroups? The check here only evaluates whether the leaf pool (pool->cnt) exceeds its own dmem.high limit. If the leaf is under its limit but a parent cgroup is over its dmem.high soft limit, ret_over_high_pool remains NULL. Should this walk the ancestor chain to enforce the hierarchical dmem.high limits, similar to what is done in dmem_cgroup_state_evict_valuable()? > + css_get(&pool->cs->css); > + dmemcg_pool_get(*ret_over_high_pool); > + } > + > /* On success, reference from get_current_dmemcs is transferred to > *ret_pool */ > *ret_pool = pool; > return 0; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
