Re: [PATCH v4 3/4] drm/xe: Split the bo shrinker into purgeable and non-purgeable shrinkers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: Use-After-Free of shrinker during DRM device teardown
- [High] drm/xe: O(N^2) CPU lockup in purgeable shrinker due to uncounted LRU skips
--

commit 1e9348c2fe4768de2cf24e4dd9b6188400c84043
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/xe: Split the bo shrinker into purgeable and non-purgeable shrinkers

This commit splits the single xe_shrinker into two independent shrinker
instances: one for purgeable objects and another for non-purgeable
(shrinkable) objects. This allows the shrinker core to apply different
reclaim pressures based on the varying costs of reclaiming these object
types.

> diff --git a/drivers/gpu/drm/xe/xe_shrinker.c b/drivers/gpu/drm/xe/xe_shrinker.c
> index 378022c37a57c..8e3fe675e0f34 100644
> --- a/drivers/gpu/drm/xe/xe_shrinker.c
> +++ b/drivers/gpu/drm/xe/xe_shrinker.c

[ ... ]

> @@ -216,63 +238,52 @@ static bool xe_shrinker_runtime_pm_get(struct xe_shrinker *shrinker, bool force,

[ ... ]

> -static unsigned long xe_shrinker_scan(struct shrinker *shrink, struct shrink_control *sc)
> +static unsigned long xe_shrinker_scan_purgeable(struct shrinker *shrink,
> +						struct shrink_control *sc)
>  {
>  	struct xe_shrinker *shrinker = to_xe_shrinker(shrink);
>  	struct ttm_operation_ctx ctx = {
>  		.interruptible = false,
>  		.no_wait_gpu = ttm_bo_shrink_avoid_wait(),
>  	};
> -	unsigned long nr_to_scan, nr_scanned = 0, freed = 0;
> +	unsigned long nr_to_scan = sc->nr_to_scan, nr_scanned = 0, freed = 0;
>  	struct xe_bo_shrink_flags shrink_flags = {
>  		.purge = true,
> -		/* Don't request writeback without __GFP_IO. */
> -		.writeback = !ctx.no_wait_gpu && (sc->gfp_mask & __GFP_IO),
>  	};
> -	bool runtime_pm;
> -	bool purgeable;
> -	bool can_backup = !!(sc->gfp_mask & __GFP_FS);
> -
> -	nr_to_scan = sc->nr_to_scan;
> -
> -	read_lock(&shrinker->lock);
> -	purgeable = !!shrinker->purgeable_pages;
> -	read_unlock(&shrinker->lock);
>  
> -	/* Might need runtime PM. Try to wake early if it looks like it. */
> -	runtime_pm = xe_shrinker_runtime_pm_get(shrinker, false, nr_to_scan, can_backup);
> -
> -	if (purgeable && nr_scanned < nr_to_scan)
> -		xe_shrinker_walk(shrinker, &ctx, shrink_flags,
> -				 nr_to_scan, &nr_scanned, &freed);
> +	xe_shrinker_walk(shrinker, &ctx, shrink_flags, nr_to_scan, &nr_scanned, &freed);
>  
>  	sc->nr_scanned = nr_scanned;

[Severity: High]
Can this cause an O(N^2) CPU lockup in the purgeable shrinker?

When the memory reclaim core (do_shrink_slab) calls this purgeable scan
routine under memory pressure, xe_shrinker_walk() iterates the LRU list.

However, non-purgeable BOs return -EBUSY and are skipped without advancing
the scanned counter:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() {
    ...
    if (!(tt->page_flags & TTM_TT_FLAG_EXTERNAL_MAPPABLE) ||
        (flags.purge && !xe_tt->purgeable))
        return -EBUSY;
    ...
}

drivers/gpu/drm/xe/xe_shrinker.c:__xe_shrinker_walk() {
    ...
            lret = xe_bo_shrink(ctx, ttm_bo, flags, scanned);
            /* Like the TTM LRU walk, skip bos we can't shrink now. */
            if (lret == -EBUSY)
                continue;
    ...
}

Since the LRU cursor is initialized locally on each call and non-purgeable
BOs do not increment nr_scanned, every batch rescans all leading
non-purgeable BOs without significantly advancing sc->nr_scanned.

Will this force the shrinker core to repeatedly call this function,
processing the same leading non-purgeable BOs over and over again?

> -	if (nr_scanned >= nr_to_scan || !can_backup)
> -		goto out;
>  
> -	/* If we didn't wake before, try to do it now if needed. */
> -	if (!runtime_pm)
> -		runtime_pm = xe_shrinker_runtime_pm_get(shrinker, true, 0, can_backup);
> +	return nr_scanned ? freed : SHRINK_STOP;
> +}

[ ... ]

> @@ -294,6 +305,7 @@ static void xe_shrinker_fini(struct drm_device *drm, void *arg)
>  	xe_assert(shrinker->xe, !shrinker->purgeable_pages);
>  	xe_assert(shrinker->xe, !shrinker->populated_tts);
>  	shrinker_free(shrinker->shrink);
> +	shrinker_free(shrinker->purge_shrink);
>  	flush_work(&shrinker->pm_worker);
>  	kfree(shrinker);
>  }

[Severity: High]
Does this kfree(shrinker) leave xe->mem.shrinker as a dangling pointer?

Because drmm actions execute in LIFO order upon device release,
xe_shrinker_fini() is called before xe_device_destroy(). 

Later, xe_device_destroy() runs and calls xe_bo_dev_fini() which flushes
the async_free workqueue. Pending BOs in the workqueue will have their
final references dropped, leading to xe_ttm_tt_account_subtract():

drivers/gpu/drm/xe/xe_bo.c:xe_ttm_tt_account_subtract() {
    ...
    if (xe_tt->purgeable)
        xe_shrinker_mod_pages(xe->mem.shrinker, 0, -(long)tt->num_pages, -1);
    else
        xe_shrinker_mod_pages(xe->mem.shrinker, -(long)tt->num_pages, 0, -1);
    ...
}

Can this lead to a use-after-free and memory corruption when driver unloads?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016133523.3622-1-thomas.hellstrom@linux.intel.com?part=3