Re: [PATCH v2 20/33] drm/xe: Add a page defragmentation worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Accessing `bo->ttm.ttm` outside of the BO's dma-resv lock can cause a NULL pointer dereference or data race.
- [Critical] The defrag delayed worker can be scheduled during device probe before its teardown is registered, leading to a UAF if the probe fails.
- [High] The defrag worker permanently stalls if the device enters runtime suspend.
- [High] The defrag worker fails to apply exponential backoff on sub-optimal fallback, causing an infinite spin loop.
--

--- Patch [20]: [PATCH v2 20/33] drm/xe: Add a page defragmentation worker ---
commit 7f5402dce2516caf0e93f47b50a3f18b01fe034e
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Add a page defragmentation worker
    
This patch adds a per-device defrag list and delayed worker to track and
reallocate BOs whose backing TT pages were allocated below the device's
beneficial order. The worker runs periodically and attempts to re-back BOs
with beneficial-order pages, up to a maximum size budget per run.

> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
[ ... ]
> @@ -1271,6 +1364,96 @@ static void xe_bo_defrag_worker(struct work_struct *w)
>  	struct delayed_work *dwork = to_delayed_work(w);
>  	struct xe_device *xe =
>  		container_of(dwork, struct xe_device, mem.defrag.worker);
>  	u64 defrag_bytes = 0;
>  	bool requeue = false;
>  	int idx;
>  
>  	if (!drm_dev_enter(&xe->drm, &idx))
>  		return;
>  
>  	if (!xe_pm_runtime_get_if_in_use(xe)) {
>  		drm_dev_exit(idx);
>  		return;
>  	}

[Severity: High]
Does the defrag worker permanently stall if the device enters runtime suspend?

If the worker executes while the device is suspending,
xe_pm_runtime_get_if_in_use() returns false and the worker exits immediately
without rescheduling itself.

Because the list is not emptied, subsequent additions to the list in
xe_bo_defrag_add() will not see an empty-to-non-empty transition, meaning
xe_bo_defrag_schedule() will never be called again.

[ ... ]
> +		ret = xe_bo_defrag_one(xe, bo, XE_BO_DEFRAG_SIZE_LIMIT - defrag_bytes,
> +				       &consumed);
> +		defrag_bytes += consumed;
> +
> +		if (ret || ttm_tt_is_beneficial_order_failed(bo->ttm.ttm)) {

[Severity: Critical]
Can accessing bo->ttm.ttm here cause a NULL pointer dereference?

The xe_bo_defrag_one() function releases the BO's dma-resv lock before it
returns. If a concurrent operation, such as evicting to VRAM or swapping out,
destroys the TT while the BO is unlocked, checking
ttm_tt_is_beneficial_order_failed(bo->ttm.ttm) might dereference a NULL
pointer.

> +			scoped_guard(spinlock, &xe->mem.defrag.lock) {
> +				if (ret)
> +					/*
> +					 * Abort the pass and retry the whole
> +					 * list later, backing off exponentially
> +					 * on every failure.
> +					 */
> +					xe->mem.defrag.interval_ms =
> +						min(xe->mem.defrag.interval_ms * 2,
> +						    (unsigned int)XE_BO_DEFRAG_INTERVAL_MAX_MS);
> +				else
> +					/* Progress, reset interval */
> +					xe->mem.defrag.interval_ms = XE_BO_DEFRAG_INTERVAL_MS;
> +			}

[Severity: High]
Does the backoff logic fail to apply when a fallback allocation succeeds?

If system memory is highly fragmented, the TTM pool allocator might fail to
allocate beneficial-order pages but successfully fall back to a smaller order.
In this case, xe_bo_defrag_one() returns 0 for success, but
ttm_tt_is_beneficial_order_failed() remains true.

The backoff block checks if (ret). Since ret is 0, it takes the else branch,
interprets the run as progress, and resets the interval to 25ms. This could
result in a tight loop of repeated failed upgrades that wastes CPU time without
ever backing off.

> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -1063,6 +1063,15 @@ int xe_device_probe(struct xe_device *xe)
>  			return err;
>  	}
>  
> +	/*
> +	 * Register the defrag worker teardown now that the migrate contexts it
> +	 * depends on are initialized, so devm's reverse-order cleanup stops the
> +	 * worker before those contexts are torn down.
> +	 */
> +	err = xe_bo_defrag_init(xe);

[Severity: Critical]
Can the delayed worker outlive the device if probe fails before the teardown
is registered?

During early probe, components might allocate BOs. If these BOs fail to get
beneficial order pages, they are added to the defrag list, which schedules
the delayed worker via xe_bo_defrag_schedule().

If a probe step fails before reaching this late xe_bo_defrag_init() call,
the devm cleanup will roll back and free the device without cancelling the
worker. When the timer expires, the worker will access freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710215442.2444235-1-matthew.brost@intel.com?part=20