Re: [PATCH v7 5/6] drm/ttm: Be more aggressive when allocating below protection limit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing IS_ERR check on get_cg_pool_unlocked() return value in dmem_cgroup_get_common_ancestor() causes a css reference leak and returns an ERR_PTR.
- [High] alloc_state->may_try_low remains uninitialized (false) when ttm_resource_try_charge fails with -EAGAIN, preventing necessary eviction of low-protected buffers.
--

--- Patch [5]: [PATCH v7 5/6] drm/ttm: Be more aggressive when allocating below protection limit ---
commit e2b13b1ad2c262766026403c3b8254d739bd29e9
Author: Natalie Vock <natalie.vock@gmx.de>
Subject: drm/ttm: Be more aggressive when allocating below protection limit

This patch modifies the TTM allocation process to consider cgroup protection
limits. When a cgroup's memory usage is below its low or min limit, TTM will
now attempt to evict unprotected buffers to make space rather than forcing
allocations into the GTT domain.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 95ce3fc0f8764..ec0821e306da1 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -544,6 +548,42 @@ static int ttm_bo_alloc_at_place(struct ttm_buffer_object *bo,
>  		}
>  	}
>  
> +	/*
> +	 * cgroup protection plays a special role in eviction.
[ ... ]
> +	 * eviction path that only evicts buffers covered by neither 'low' or
> +	 * 'min' protections.
> +	 */
> +	if (!alloc_state->in_evict) {
> +		may_evict |= dmem_cgroup_below_min(NULL, alloc_state->charge_pool);
> +		alloc_state->may_try_low = may_evict;

[Severity: High]
If ttm_resource_try_charge() fails with -EAGAIN earlier in this function, it
returns early and bypasses setting alloc_state->may_try_low here. 

Since alloc_state is zero-initialized, may_try_low would remain false.
Would this unintentionally prevent the fallback eviction pass in
ttm_bo_evict_alloc() from executing, thereby preventing a cgroup at its max
limit from evicting low-protected BOs from its own subtree?

> +
> +		may_evict |= dmem_cgroup_below_low(NULL, alloc_state->charge_pool);
> +	}
> +
>  	ret = ttm_resource_alloc(bo, place, res, alloc_state->charge_pool);
>  	if (ret) {
[ ... ]

While reviewing the interactions with the dmem cgroup API, I noticed an
issue in kernel/cgroup/dmem.c:

kernel/cgroup/dmem.c:dmem_cgroup_get_common_ancestor() {
        ...
		pool = get_cg_pool_unlocked(css_to_dmemcs(ancestor_css),
					    a->region);
	if (!pool)
		css_put(ancestor_css);
        ...
}

[Severity: High]
Could get_cg_pool_unlocked() return an error pointer like ERR_PTR(-ENOMEM)?

If it returns an error pointer, the !pool check evaluates to false, causing
the ancestor_css reference to be leaked.

Additionally, since this function is expected to return NULL on failure,
returning an ERR_PTR could cause callers checking for NULL to dereference
the error pointer. Should this check use IS_ERR(pool) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dmemcg-aggressive-protect-v7-0-97c4b1014315@gmx.de?part=5