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

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 04, 2026 at 12:31:44AM +0000, sashiko-bot@kernel.org wrote:
> 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?
> 

Execlist backend is completely broken and not even able to be enabled
via a modparam (i.e., it is deadcode).

> >  
> >  	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?
> 

The PT BOs hold a reference to the VM and PT BOs are only finally
destroyed after the jobs runs.

> [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?
> 

Flagged eariler, will fix.

> > +	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?
> 

Non-issue. Once xe_sched_job_push is called run_job must be called. We
do not a failure path between xe_pt_job_ops_get and run_job eventually
executing which drops the ref pt_job_ops.

Matt

> > +
> >  	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