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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

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 ?

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