Re: [PATCH v8 6/6] drm/ttm: Use common ancestor of evictor and evictee as limit pool

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Refcount underflow and use-after-free of cgroup subsystem state (css).
- [High] GFP_KERNEL allocation while holding a dma_resv lock causes lockdep warnings and potential deadlocks.
--

--- Patch [6]: [PATCH v8 6/6] drm/ttm: Use common ancestor of evictor and evictee as limit pool ---
commit 80190cfff36182bea4c8b3f724df7df584bfc3b3
Author: Natalie Vock <[email protected]>

drm/ttm: Use common ancestor of evictor and evictee as limit pool
    
When checking if buffers should be skipped for eviction due to dmem.low
protection, this patch calculates the protection from the shared ancestor
of the evictor and evictee instead of the root cgroup. This prevents
breaking prioritization of sibling cgroups.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 6bbea148838e4..79c96aba4bf49 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -628,11 +628,48 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
>  {
>  	struct ttm_bo_evict_walk *evict_walk =
>  		container_of(walk, typeof(*evict_walk), walk);
> +	struct dmem_cgroup_pool_state *limit_pool, *ancestor = NULL;
> +	bool evict_valuable;
>  	s64 lret;
>  
[ ... ]
> +	if (!limit_pool) {
> +		ancestor = dmem_cgroup_get_common_ancestor(bo->resource->css,
> +							   evict_walk->alloc_state->charge_pool);

[Severity: High]
Does this allocation path trigger a deadlock?

ttm_bo_evict_cb() is executed via ttm_lru_walk_for_evict() which holds the
dma_resv lock of the buffer object (acquired via
ttm_bo_lru_for_each_reserved_guarded).

When calling dmem_cgroup_get_common_ancestor(), it can end up in
get_cg_pool_unlocked() which does a GFP_KERNEL allocation if the ancestor
pool does not exist:

kernel/cgroup/dmem.c:get_cg_pool_unlocked() {
	...
	allocpool = kzalloc_obj(*allocpool);
	...
}

Using GFP_KERNEL while holding a dma_resv lock can trigger direct memory
reclaim, which in turn can invoke GPU shrinkers.

Those shrinkers will attempt to acquire dma_resv locks, leading to a
potential deadlock.

> +		limit_pool = ancestor;
> +	}
> +
> +	evict_valuable = dmem_cgroup_state_evict_valuable(limit_pool, bo->resource->css,
> +							  evict_walk->try_low,
> +							  &evict_walk->hit_low);
> +	if (ancestor)
> +		dmem_cgroup_pool_state_put(ancestor);

[Severity: Critical]
Does this cause a refcount underflow and use-after-free of the cgroup
subsystem state (css)?

dmem_cgroup_get_common_ancestor() acquires and returns a pool reference but
explicitly drops its temporary css reference before returning:

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

However, dmem_cgroup_pool_state_put() is designed to balance
dmem_cgroup_try_charge() and drops both a pool reference and a css reference.

Since dmem_cgroup_get_common_ancestor() did not acquire an extra css reference
for the caller, the css_put() in dmem_cgroup_pool_state_put() causes an
underflow.

This can lead to premature freeing of the css, resulting in a use-after-free
and memory corruption.

> +
> +	if (!evict_valuable)
>  		return 0;

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