Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] The defrag worker can be scheduled during probe teardown but is never canceled if probe fails early, leading to a UAF of the `xe_device`. - [High] The defrag worker continuously polls at a high frequency (40Hz / 25ms) while the device is runtime-suspended or idle, causing significant power regression. - [High] The defrag worker enters an infinite 40Hz retry loop on the same BO when memory is fragmented, bypassing exponential backoff. --
--- Patch [20]: [PATCH v3 20/33] drm/xe: Add a page defragmentation worker --- commit 1f23cf5472cb7a98f4987467477a919155eb0dc0 Author: Matthew Brost <[email protected]> Subject: drm/xe: Add a page defragmentation worker This patch adds a delayed worker to the DRM XE driver to periodically defragment buffer objects whose backing TT pages were allocated below the optimal order. It bounds the work done per run and applies exponential backoff on failures. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > +static void xe_bo_defrag_worker(struct work_struct *w) > +{ [ ... ] > + if (!drm_dev_enter(&xe->drm, &idx)) > + return; > + > + if (!xe_pm_runtime_get_if_in_use(xe)) { > + /* > + * The device is (runtime) suspending/suspended. Reschedule so > + * the pass retries once it is active again: the enqueue-time > + * kick in xe_bo_defrag_add() only fires on an empty->non-empty > + * transition, so it will not re-arm the worker while the list > + * is still non-empty. Requeue here to avoid stranding tracked > + * BOs. The get attempt does not resume the device, so this is > + * just a lightweight CPU-side timer while suspended. > + */ > + if (atomic_read(&xe->mem.defrag.count)) > + xe_bo_defrag_schedule(xe); > + drm_dev_exit(idx); > + return; > + } [Severity: High] Will this cause the worker to endlessly poll at a high frequency while the device is suspended? Looking at xe_bo_defrag_worker() in xe_bo.c, when the device is runtime suspended, xe_pm_runtime_get_if_in_use(xe) returns false. The worker then hits this early return path and schedules itself via xe_bo_defrag_schedule(xe) if the defrag count is non-zero. Since this rescheduling uses the current interval_ms (which is typically 25ms) and applies no backoff, the worker will repeatedly wake up the CPU every 25ms (40Hz) endlessly while the device is suspended, preventing deep sleep C-states. > + > + /* > + * Process at most XE_BO_DEFRAG_SIZE_LIMIT bytes of newly (re)allocated [ ... ] > + if (ret || needs_more) { > + 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; > + } > + > + xe_bo_put(bo); > + requeue = true; > + break; > + } [Severity: High] Does this create an infinite 40Hz retry loop on the same BO when memory is fragmented, bypassing the exponential backoff? When xe_bo_defrag_one() successfully satisfies an allocation using lower-order pages, ret is 0 but needs_more is true. In this block, because ret is 0, the else branch resets the interval_ms to 25ms, and the loop breaks with requeue set to true. Since the BO is never moved to the back of the list or removed, it remains at the head. On the next run in 25ms, the worker attempts to defrag the exact same BO again, likely hitting the exact same fragmentation fallback, and reschedules at 25ms again. > + > + xe_bo_put(bo); > + } [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c [ ... ] > @@ -1069,6 +1069,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); > + if (err) > + return err; [Severity: Critical] Is there a use-after-free risk if the device probe fails before this point? The devm action to cleanly cancel the worker (xe_bo_defrag_init) is registered late in xe_device_probe(). If the probe fails before this registration (e.g., during xe_display_init_early() or xe_gt_init()), the devm teardown will unpin BOs. Unpinning a BO calls xe_bo_unpin() -> xe_bo_defrag_update(), which can schedule the worker. Because the devm cleanup action was never registered, the worker won't be cancelled during teardown, leaving it active after the xe struct is freed. > + > err = xe_pagefault_init(xe); > if (err) > return err; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20
