Re: [PATCH 2/2] drm/xe: Don't free job in TDR

[Matthew Auld <matthew.auld@intel.com>]

On 03/10/2024 15:05, Matthew Brost wrote:
> On Thu, Oct 03, 2024 at 08:06:24AM +0100, Matthew Auld wrote:
>> On 03/10/2024 01:16, Matthew Brost wrote:
>>> Freeing job in TDR is not safe as TDR can pass the run_job thread
>>> resulting in UAF. It is only safe for free job to naturally be called by
>>> the scheduler. Rather free job in TDR, add to pending list.
>>
>> s/Rather free/Rather than free/
>> ?
>>
> 
> Yes, will fix.
> 
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2811
>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>> Fixes: e275d61c5f3f ("drm/xe/guc: Handle timing out of signaled jobs gracefully")
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>
>> I think we still have the other issue with fence signalling in run_job.
>>
> 
> I think this actually ok given free_job as owns a ref to job->fence and
> free_job now must run after run_job - that is why I didn't include this
> change in this patch. But I also agree a better design would be move the
> dma_fence_get from run_job to arm - I will do that in a follow up.

Here I mean the race in run_job() itself, before we hand over the fence 
to the scheduler. i.e do the dma_fence_get() before the submission part 
like in: 
https://patchwork.freedesktop.org/patch/615249/?series=138921&rev=1.

> 
> Matt
> 
>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>>
>>> ---
>>>    drivers/gpu/drm/xe/xe_guc_submit.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>>> index 80062e1d3f66..9ecd1661c1b5 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>>> @@ -1106,10 +1106,13 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>>>    	/*
>>>    	 * TDR has fired before free job worker. Common if exec queue
>>> -	 * immediately closed after last fence signaled.
>>> +	 * immediately closed after last fence signaled. Add back to pending
>>> +	 * list so job can be freed and kick scheduler ensuring free job is not
>>> +	 * lost.
>>>    	 */
>>>    	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &job->fence->flags)) {
>>> -		guc_exec_queue_free_job(drm_job);
>>> +		xe_sched_add_pending_job(sched, job);
>>> +		xe_sched_submission_start(sched);
>>>    		return DRM_GPU_SCHED_STAT_NOMINAL;
>>>    	}