Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing cancellation of deferred snapshot work during driver unload leads to a use-after-free. - [High] Calling cancel_work_sync() from within the work item itself causes a self-deadlock on the dev_coredumpm_timeout() error path. - [High] dev_coredumpm_timeout() synchronous error path clears the snapshot memory, causing a NULL pointer dereference in the worker thread. -- commit 9607c0ac0490954ce494b7fd2f2026e574352b10 Author: Zhanjun Dong <[email protected]> Subject: drm/xe: Fix null pointer dereference in devcoredump cleanup In xe_devcoredump_snapshot_free(), ss->gt may be NULL when the snapshot was never fully populated. Guard the xe_guc_capture_put_matched_nodes() call with IS_ERR_OR_NULL() to prevent a null dereference. Link: https://patch.msgid.link/[email protected] > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c > b/drivers/gpu/drm/xe/xe_devcoredump.c > index 203e3038cc817..26a294f79e79f 100644 > --- a/drivers/gpu/drm/xe/xe_devcoredump.c > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c [ ... ] > @@ -255,7 +256,8 @@ static void xe_devcoredump_free(void *data) > if (!data || !coredump_to_xe(coredump)) > return; > > - cancel_work_sync(&coredump->snapshot.work); > + if (coredump->captured) > + cancel_work_sync(&coredump->snapshot.work); [Severity: High] This is a pre-existing issue, but does calling cancel_work_sync() from within the work item itself cause a self-deadlock? In xe_devcoredump_deferred_snap_work(), which executes on the workqueue, there is a call to dev_coredumpm_timeout(). If dev_coredumpm_timeout() fails (for example, if devcoredump is disabled via sysfs or memory allocation fails), it synchronously invokes the free callback, xe_devcoredump_free(). Because coredump->captured is true while this work is executing, xe_devcoredump_free() calls cancel_work_sync() on the same work item. Since this executes in the context of the work item itself, it blocks forever waiting for the work to complete, deadlocking the system_unbound_wq worker. [Severity: High] This isn't a bug introduced by this patch, but can the synchronous error path described above also result in a null pointer dereference within the worker thread? Assuming the deadlock in cancel_work_sync() is bypassed, xe_devcoredump_free() will clear the snapshot memory: /* To prevent stale data on next snapshot, clear everything */ memset(&coredump->snapshot, 0, sizeof(coredump->snapshot)); Once dev_coredumpm_timeout() returns control to xe_devcoredump_deferred_snap_work(), the worker thread immediately attempts to evaluate gt_to_fw(ss->gt). Since ss->gt was just zeroed out, this results in an invalid pointer offset, which is then dereferenced by xe_force_wake_get(), crashing the kernel. [Severity: Critical] This is another pre-existing issue in the devcoredump lifecycle, but is there a risk of a use-after-free here due to missing cancellation of deferred snapshot work during driver unload? When the driver is unloaded or unbound, xe_driver_devcoredump_fini() executes: static void xe_driver_devcoredump_fini(void *arg) { struct drm_device *drm = arg; dev_coredump_put(drm->dev); } If a GPU hang recently queued xe_devcoredump_deferred_snap_work() on the system workqueue, and the driver is unloaded before this work executes, dev_coredumpm_timeout() hasn't been called yet. This means the devcd device does not exist, so dev_coredump_put() does nothing. The driver teardown completes and frees the xe_device memory. The workqueue subsequently executes the pending work, dereferencing the freed coredump structure. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
