Re: [PATCH v7 2/3] drm/xe/guc: Protect timeout handling from device unplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe/guc: memory leak of xe_sched_job when timeout handler returns NO_HANG
--

commit 178ee20625b5c3cfd23b5dbaf2b17c6d37fd5010
Author: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>

drm/xe/guc: Protect timeout handling from device unplug

This commit guards the GuC scheduler timeout callback with drm_dev_enter()
and drm_dev_exit() to ensure device unplug waits for in-flight timeout
handling. If the unplug has already started, it sets the job error to
-ENODEV and returns it to the scheduler for cleanup.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 7ce73489c35fc..e0a3ce6015e02 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -1614,6 +1615,11 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>  	    vf_recovery(guc))
>  		return DRM_GPU_SCHED_STAT_NO_HANG;
>  
> +	if (!drm_dev_enter(&xe->drm, &idx)) {
> +		xe_sched_job_set_error(job, -ENODEV);
> +		return DRM_GPU_SCHED_STAT_NO_HANG;
> +	}

[Severity: High]
Does returning DRM_GPU_SCHED_STAT_NO_HANG here cause a permanent memory
leak of the xe_sched_job?

When drm_dev_enter() fails, the timeout handler returns NO_HANG, which
causes the DRM scheduler to reinsert the job into sched->pending_list.
The unplug thread then destroys the queue, calling drm_sched_fini(),
which cancels the asynchronous work_free_job using cancel_work_sync().

Because the Xe driver does not implement the cancel_job callback in its
drm_sched_ops:

static const struct drm_sched_backend_ops drm_sched_ops = {
        .run_job = guc_exec_queue_run_job,
        .free_job = guc_exec_queue_free_job,
        .timedout_job = guc_exec_queue_timedout_job,
};

drm_sched_fini() does not free the remaining jobs in pending_list. The
jobs are permanently leaked, accompanied by a "Tearing down scheduler
while jobs are pending" warning.

As a quick note, a use-after-free concern regarding job->q here would
be structurally impossible, because guc_exec_queue_do_destroy() calls
cancel_delayed_work_sync(&work_tdr), strictly serializing queue teardown
against the timeout handler.

> +
>  	/* Kill the run_job entry point */
>  	if (xe_exec_queue_is_multi_queue(q))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013152417.3189567-5-satyanarayana.k.v.p@intel.com?part=2