Re: [RFC PATCH 1/3] drm/xe: Capture devcoredump on TLB invalidation timeout

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 te=
ardown leaves the `fence_tdr` worker running after `xe_device` is freed, le=
ading 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 invalidat=
ion timeout ---
commit cb8081669b45d5d264497cf1169cfc3a90d4aadd
Author: Tales A. Mendon=C3=A7a <[email protected]>

drm/xe: Capture devcoredump on TLB invalidation timeout
   =20
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.
   =20
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);
>  }
> =20
> +static void devcoredump_snapshot_gt(struct xe_devcoredump *coredump,
> +				    struct xe_gt *gt)
> +{
> +	struct xe_devcoredump_snapshot *ss =3D &coredump->snapshot;
> +	struct xe_guc *guc =3D &gt->uc.guc;
> +	bool cookie;
> +
> +	ss->snapshot_time =3D ktime_get_real();
> +	ss->boot_time =3D ktime_get_boottime();
> +
> +	strscpy(ss->process_name, "no process");
> +
> +	ss->gt =3D 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 =3D dma_fence_begin_signalling();
> +
> +	ss->guc.log =3D xe_guc_log_snapshot_capture(&guc->log, true);
> +	ss->guc.ct =3D 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_tl=
b_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_st=
ruct *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=3D%d recv=3D%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?

>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804021441.3054=
[email protected]?part=3D1