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 <[email protected]> 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/[email protected]?part=20
