Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing synchronization in `xe_tlb_inval_reset()` during device teardown leaves the `fence_tdr` worker running after `xe_device` is freed, leading to Use-After-Free. - [High] Devcoredump snapshot worker `ss->work` is not explicitly canceled during device teardown if it hasn't run yet, leading to Use-After-Free. -- --- Patch [1]: [RFC PATCH 1/3] drm/xe: Capture devcoredump on TLB invalidation timeout --- commit cb8081669b45d5d264497cf1169cfc3a90d4aadd Author: Tales A. Mendonça <[email protected]> drm/xe: Capture devcoredump on TLB invalidation timeout Adds a new xe_devcoredump_gt variant to capture GuC logs and CT state for GT-level hangs, particularly hooking it into the TLB invalidation timeout path. Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8678 > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c > b/drivers/gpu/drm/xe/xe_devcoredump.c > --- a/drivers/gpu/drm/xe/xe_devcoredump.c > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c [ ... ] > @@ -403,6 +403,74 @@ void xe_devcoredump(struct xe_exec_queue *q, struct > xe_sched_job *job, const cha > mutex_unlock(&coredump->lock); > } > > +static void devcoredump_snapshot_gt(struct xe_devcoredump *coredump, > + struct xe_gt *gt) > +{ > + struct xe_devcoredump_snapshot *ss = &coredump->snapshot; > + struct xe_guc *guc = >->uc.guc; > + bool cookie; > + > + ss->snapshot_time = ktime_get_real(); > + ss->boot_time = ktime_get_boottime(); > + > + strscpy(ss->process_name, "no process"); > + > + ss->gt = gt; > + INIT_WORK(&ss->work, xe_devcoredump_deferred_snap_work); > + > + /* keep going if fw fails as we still want to save the SW data */ > + CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL); > + > + cookie = dma_fence_begin_signalling(); > + > + ss->guc.log = xe_guc_log_snapshot_capture(&guc->log, true); > + ss->guc.ct = xe_guc_ct_snapshot_capture(&guc->ct); > + > + queue_work(system_dfl_wq, &ss->work); > + [Severity: High] This is a pre-existing issue, but is it possible for this deferred work to cause a use-after-free if the device is unbound before the worker runs? If the device is unbound before xe_devcoredump_deferred_snap_work() executes, xe_driver_devcoredump_fini() calls dev_coredump_put(). However, since the devcoredump device hasn't been registered yet, dev_coredump_put() does nothing and xe_devcoredump_free() is not called: drivers/gpu/drm/xe/xe_devcoredump.c:xe_driver_devcoredump_fini() { ... dev_coredump_put(drm->dev); } Since there is no explicit cancel_work_sync(&coredump->snapshot.work) when the core dump hasn't been created, does this leave the worker queued on system_dfl_wq to eventually wake up and dereference freed device memory? > + dma_fence_end_signalling(cookie); > +} > + [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_tlb_inval.c > b/drivers/gpu/drm/xe/xe_tlb_inval.c > --- a/drivers/gpu/drm/xe/xe_tlb_inval.c > +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c [ ... ] > @@ -97,6 +107,16 @@ static void xe_tlb_inval_fence_timeout(struct work_struct > *work) > queue_delayed_work(tlb_inval->timeout_wq, &tlb_inval->fence_tdr, > timeout_delay); > spin_unlock_irq(&tlb_inval->pending_lock); > + > + /* > + * Capture the GuC log and CT state so the firmware side of the hang > + * can be inspected; there is no queue or job to blame here. Must be > + * outside pending_lock as the capture takes sleeping locks. > + */ > + if (timedout_seqno) > + xe_devcoredump_gt(tlb_inval_to_gt(tlb_inval), > + "TLB invalidation fence timeout, seqno=%d > recv=%d", > + timedout_seqno, tlb_inval->seqno_recv); [Severity: High] This is a pre-existing issue, but can this lead to a use-after-free during device teardown? Looking at xe_tlb_inval_reset() in drivers/gpu/drm/xe/xe_tlb_inval.c, it cancels the timeout worker using cancel_delayed_work() rather than cancel_delayed_work_sync(): xe_tlb_inval_reset() { ... cancel_delayed_work(&tlb_inval->fence_tdr); ... } Since the cancellation is not synchronous and the worker is scheduled on system_wq, the teardown can proceed and free the device memory while xe_tlb_inval_fence_timeout() is still running. If the worker drops pending_lock and executes this new call to xe_devcoredump_gt(), won't it access the already freed tlb_inval outside of the lock? > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
