Re: [PATCH v8 4/6] drm/ttm: Split cgroup charge and resource allocation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Eviction walk continues to restrict evictions to a specific cgroup even after its memory limit is satisfied, causing spurious allocation failures under global memory pressure.

Pre-existing issues:
- [Critical] dmem_cgroup_get_common_ancestor returns a pool without a CSS reference, but the caller puts it using dmem_cgroup_pool_state_put which drops a CSS reference, leading to a CSS refcount underflow.
--

--- Patch [4]: [PATCH v8 4/6] drm/ttm: Split cgroup charge and resource allocation ---
commit aa5cab6f4c763e172491a34bffbf1a67e37f5262
Author: Natalie Vock <[email protected]>

drm/ttm: Split cgroup charge and resource allocation

This patch decouples resource allocation from cgroup charging to address 
raciness when checking min/low cgroup limits during eviction. It charges 
the allocation to the cgroup once and keeps the charge for the duration of
the resource allocation attempt.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 1cdb2172d5924..3becddd2c43b8 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -518,18 +522,39 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
>  	bool may_evict;
>  	int ret;
>  
> -	may_evict = force_space && place->mem_type != TTM_PL_SYSTEM;
> -
> -	ret = ttm_resource_alloc(bo, place, res,
> -				 force_space ? &alloc_state->limit_pool : NULL);
> +	may_evict = !alloc_state->in_evict && force_space &&
> +		    place->mem_type != TTM_PL_SYSTEM;
> +	if (!alloc_state->charge_pool) {
> +		ret = ttm_resource_try_charge(bo, place, &alloc_state->charge_pool,
> +					      force_space ? &alloc_state->limit_pool
> +							  : NULL);

[Severity: High]
If ttm_resource_try_charge() succeeds during an eviction callback (where
force_space is false), alloc_state->limit_pool is not cleared.

If the subsequent physical resource allocation still fails due to global
memory pressure, the eviction walk will continue.

Because alloc_state->limit_pool remains set, will the LRU walk continue to
only evict buffers from that specific cgroup, eventually causing a spurious
-ENOSPC failure if that cgroup has no more evictable buffers?

> +		if (ret) {
> +			/*
> +			 * -EAGAIN means the charge failed, which we treat
> +			 * like an allocation failure. Therefore, return an
> +			 * error code indicating the allocation failed -
> +			 * either -EBUSY if the allocation should be
> +			 * retried with eviction, or -ENOSPC if there should
> +			 * be no second attempt.
> +			 */
> +			if (ret == -EAGAIN)
> +				ret = may_evict ? -EBUSY : -ENOSPC;
> +			return ret;
> +		}
> +	}
[ ... ]
> @@ -584,8 +609,10 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
>  
>  	evict_walk->evicted++;
>  	if (evict_walk->res)
> -		lret = ttm_resource_alloc(evict_walk->evictor, evict_walk->place,
> -					  evict_walk->res, NULL);
> +		lret = ttm_bo_alloc_at_place(evict_walk->evictor,
> +					     evict_walk->place, false,
> +					     evict_walk->res,
> +					     evict_walk->alloc_state);

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential refcount underflow
involving the ancestor pool earlier in ttm_bo_evict_cb().

dmem_cgroup_get_common_ancestor() explicitly drops its CSS reference before
returning the pool:

kernel/cgroup/dmem.c:dmem_cgroup_get_common_ancestor() {
    ...
    if (IS_ERR(pool))
        pool = NULL;
    css_put(ancestor_css);
    return pool;
}

However, in ttm_bo_evict_cb(), the ancestor pool is freed using
dmem_cgroup_pool_state_put():

    if (ancestor)
        dmem_cgroup_pool_state_put(ancestor);

Since dmem_cgroup_pool_state_put() calls css_put(&pool->cs->css), doesn't
this drop a CSS reference that the caller does not own, potentially leading
to a use-after-free of the cgroup subsystem state?

>  	if (lret == 0)
>  		return 1;
>  out:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4