Re: [PATCH] drm/xe: Free job before xe_exec_queue_put

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

On Wed, Aug 21, 2024 at 11:52:26AM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 21-08-2024 01:53, Matthew Brost wrote:
> > Free job depends on job->vm being valid, the last xe_exec_queue_put can
> > destroy the VM. Prevent UAF by freeing job before xe_exec_queue_put.
> 
> 
> Seems this order needs to be fixed also in error handling/cleanup of
> xe_sched_job_create.
> 

We should likely have an additional exec queue ref from the outer layers
when creating jobs but for uniformity agree that should be changed too.
Will post a follow up for that.

Matt 

> 
> > 
> > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_sched_job.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c
> > index 44d534e362cd..9628f9deb3c0 100644
> > --- a/drivers/gpu/drm/xe/xe_sched_job.c
> > +++ b/drivers/gpu/drm/xe/xe_sched_job.c
> > @@ -171,12 +171,13 @@ void xe_sched_job_destroy(struct kref *ref)
> >   	struct xe_sched_job *job =
> >   		container_of(ref, struct xe_sched_job, refcount);
> >   	struct xe_device *xe = job_to_xe(job);
> > +	struct xe_exec_queue *q = job->q;
> >   	xe_sched_job_free_fences(job);
> > -	xe_exec_queue_put(job->q);
> >   	dma_fence_put(job->fence);
> >   	drm_sched_job_cleanup(&job->drm);
> >   	job_free(job);
> > +	xe_exec_queue_put(q);
> >   	xe_pm_runtime_put(xe);
> >   }