Re: [PATCH v6 10/24] drm/xe: Update GuC submission backend to run PT jobs

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

On Mon, Sep 21, 2026 at 05:25:16PM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 05-09-2026 03:09, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [Critical] PT jobs swallow dependency and cancellation errors, silently reporting false success and potentially leaving page tables in an inconsistent state.
> > - [High] Memory leak of xe_pt_job_ops and a dma_fence reference when a PT job is canceled or destroyed without being executed.
> > --
> > 
> > commit c6e167292efa7934a75cd29809e1057eae0d18c2
> > Author: Matthew Brost <matthew.brost@intel.com>
> > 
> > drm/xe: Update GuC submission backend to run PT jobs
> > 
> > PT jobs bypass GPU execution for the final step of a bind job, using the
> > CPU to program the required page tables. Teach the GuC submission backend
> > how to execute these jobs.
> > 
> > Link: https://patch.msgid.link/20260228013501.106680-11-matthew.brost@intel.com
> > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index 8b8673f4e037c..b7ec96650405a 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > [ ... ]
> > > @@ -1262,14 +1278,25 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
> > >   				register_exec_queue(primary, GUC_CONTEXT_NORMAL);
> > >   		}
> > > -		if (!exec_queue_registered(q))
> > > -			register_exec_queue(q, GUC_CONTEXT_NORMAL);
> > > -		if (!job->restore_replay)
> > > -			q->ring_ops->emit_job(job);
> > > -		submit_exec_queue(q, job);
> > > +		if (is_pt_job(job)) {
> > > +			xe_gt_assert(guc_to_gt(guc), !exec_queue_registered(q));
> > > +			run_pt_job(job);
> > > +		} else {
> > > +			if (!exec_queue_registered(q))
> > > +				register_exec_queue(q, GUC_CONTEXT_NORMAL);
> > > +			if (!job->restore_replay)
> > > +				q->ring_ops->emit_job(job);
> > > +			submit_exec_queue(q, job);
> > > +		}
> > >   		job->restore_replay = false;
> > >   	}
> > > +	if (is_pt_job(job)) {
> > > +		xe_pt_job_ops_put(job->pt_update[0].pt_job_ops);
> > > +		dma_fence_put(job->fence);	/* Drop ref from xe_sched_job_arm */
> > > +		return NULL;
> > > +	}
> > 
> > [Severity: High]
> > Does this code leak the xe_pt_job_ops array and dma_fence objects when a PT
> > job is canceled before execution?
> > 
> > Because the cleanup operations (xe_pt_job_ops_put() and the extra
> > dma_fence_put()) are tied to job execution here in guc_exec_queue_run_job(),
> > they appear to be missed if the job is destroyed before running.
> > 
> > Looking at xe_sched_job_destroy() in drivers/gpu/drm/xe/xe_sched_job.c:
> > 
> > void xe_sched_job_destroy(struct kref *ref)
> > {
> > 	...
> > 	xe_sched_job_free_fences(job);
> > 	dma_fence_put(job->fence);
> > 	drm_sched_job_cleanup(&job->drm);
> > 	job_free(job);
> > 	...
> > }
> > 
> > If a PT job is armed, it takes a reference to pt_job_ops and an extra
> > reference to job->fence. If the queue is killed and the job aborted before it
> > executes, xe_sched_job_destroy() misses the xe_pt_job_ops_put() call and
> > only drops one dma_fence_put() reference.
> > 
> > Can an unprivileged user repeatedly submit PT jobs via bind ioctls and
> > immediately destroy the queue before execution to trigger this?
> 
> Seems really a valid comment. better to handle xe_pt_job_ops_put and
> dma_fence_put in guc_exec_queue_free_job ?
> 

I can move this but the comment is invalid. Once arm is called, run_job
is always called - i.e., even if a queue is closed we still get a
run_job(). This is existing bookkeeping that certainly works.

Matt

> > 
> > [Severity: Critical]
> > Does returning NULL unconditionally here swallow dependency and cancellation
> > errors?
> 
> How about we return -ECANCELLED for banned jobs here  in
> guc_exec_queue_run_job?
> 
> 
> > 
> > If a queue is banned, guc_exec_queue_run_job() skips run_pt_job() but still
> > returns NULL here, which the DRM scheduler treats as a synchronous success.
> > 
> > Additionally, PT jobs are initialized with a stub fence:
> > 
> > struct xe_sched_job *xe_sched_job_create(...)
> > {
> > 	...
> > 	if (!batch_addr) {
> > 		job->fence = dma_fence_get_stub();
> > 		job->is_pt_job = true;
> > 	}
> > 	...
> > }
> > 
> > Since a stub fence is permanently signaled with error = 0,
> > xe_sched_job_set_error() fails to set any error state, and
> > xe_sched_job_is_error() evaluates to false.
> > 
> > Can this result in page table updates silently failing while reporting success
> > to userspace? For unbind operations, this could leave memory mapped in the
> > GPU after it is freed, potentially allowing read or write access to reallocated
> > memory from another queue in the same VM.
> > 
> > >   run_job_out:
> > >   	return job->fence;
> > 
>