Re: [PATCH v4 13/25] drm/xe: Enable CPU binds for jobs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] A Use-After-Free of `xe_vm` occurs because CPU-based PT updates execute asynchronously on the device-global bind queue without holding a reference to the VM.
- [High] execlist backend ignores is_pt_job and submits PT jobs to hardware, skipping CPU updates.
- [High] The `xe_pt_job_ops` reference is leaked when a PT job is aborted or destroyed without running.
- [Medium] xe_sched_job_snapshot_capture reads from the wrong union member for PT jobs.
--

commit 1bc0a17fc99eb78f42df916d66ac3038c0338a83
Author: Matthew Brost <matthew.brost@intel.com>
Subject: drm/xe: Enable CPU binds for jobs

This commit updates the migration layer to formulate a PT job which will issue
CPU binds in the submission backend. All code related to GPU-based binding has
been removed.

Link: https://patch.msgid.link/20260228013501.106680-14-matthew.brost@intel.com

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 100e84b7b81df..62ae0e2e85be5 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -1938,162 +1867,18 @@ __xe_migrate_update_pgtables(struct xe_migrate *m,
>  {

[ ... ]

>  	int err;
>  
> -	job = xe_bb_create_migration_job(pt_update_ops->q, bb,
> -					 xe_migrate_batch_base(m, usm),
> -					 update_idx);
> +	job = xe_sched_job_create(pt_update_ops->q, NULL);
>  	if (IS_ERR(job)) {
>  		err = PTR_ERR(job);
> -		goto err_sa;
> +		goto err_out;
>  	}
>  
> -	xe_sched_job_add_migrate_flush(job, MI_INVALIDATE_TLB);
> +	xe_tile_assert(tile, job->is_pt_job);

[Severity: High]
If the platform uses the execlist backend (e.g., GuC disabled), does
execlist_run_job() properly handle CPU-only PT jobs?

Looking at the execlist backend, execlist_run_job() appears to unconditionally
call q->ring_ops->emit_job(job) without checking job->is_pt_job. Because
job->ptrs and job->pt_update share a union, the ring ops might read a garbage
value (like the ops kernel pointer) as batch_addr and emit a hardware migration
job instead.

Could this regression cause an immediate GPU fault or hang from executing an
invalid address, while completely skipping the actual page table update on the
CPU?

>  
>  	if (ops->pre_commit) {
>  		pt_update->job = job;
> @@ -2104,6 +1889,12 @@ __xe_migrate_update_pgtables(struct xe_migrate *m,
>  	if (is_migrate)
>  		mutex_lock(&m->job_mutex);
>  
> +	job->pt_update[0].vm = pt_update->vops->vm;

[Severity: Critical]
Is it possible for the xe_vm pointer assigned here to be used after it is freed?

PT jobs are assigned a raw VM pointer without taking an xe_vm_get() reference.
CPU binds are queued and executed on a device-global queue (xe->cpu_bind->q).
If the user closes the VM, xe_vm_close_and_put() kills the VM's specific
queues and frees the struct xe_vm, but it does not appear to wait for jobs
pending on the global cpu_bind->q.

When the DRM scheduler later executes the pending PT job on the cpu_bind->q,
could it access the now-freed vm pointer during CPU execution, resulting in a
use-after-free regression?

[Severity: Medium]
When xe_sched_job_snapshot_capture() iterates over q->width, will it safely
handle PT jobs?

It appears that xe_sched_job_snapshot_capture() accesses job->ptrs[i].batch_addr
unconditionally. For PT jobs, job->ptrs aliases with job->pt_update via a union.

This causes batch_addr to overlap with the ops kernel pointer (or similar
members) from xe_pt_update_args. Could this mangled pointer then be
uncanonicalized and included in GPU devcoredumps, leading to corrupted state
data and a potential kernel pointer information leak?

> +	job->pt_update[0].tile = tile;
> +	job->pt_update[0].ops = ops;
> +	job->pt_update[0].pt_job_ops =
> +		xe_pt_job_ops_get(pt_update_ops->pt_job_ops);

[Severity: High]
What happens to the pt_job_ops reference if the PT job is aborted or destroyed
without running?

The job destruction path in xe_sched_job_destroy() does not seem to check for
is_pt_job, and thus skips calling xe_pt_job_ops_put(). If the job is canceled,
aborted, or its execution queue is destroyed before it runs, the DRM scheduler
bypasses the normal execution path and directly calls the free_job callback.

Would this permanently leak the pt_job_ops structure on those failure paths?

> +
>  	xe_sched_job_arm(job);
>  	fence = dma_fence_get(&job->drm.s_fence->finished);
>  	xe_sched_job_push(job);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903235842.3401722-1-matthew.brost@intel.com?part=13