Re: [PATCH 2/2] drm/panfrost: Stop a job queued after its JM context was destroyed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/panfrost: unreachable panfrost_stop_job() in
  panfrost_jm_ctx_destroy()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] drm/sched: null pointer dereference in
  drm_sched_entity_flush() [1]
--

commit 1e5a8eff6ae859bd583772ff4513448b2fcf6c55
Author: Jiwoo Kim <arpe1618@gmail.com>

drm/panfrost: Stop a job queued after its JM context was destroyed

This commit extracts a panfrost_stop_job() helper from
panfrost_jm_ctx_destroy() and calls it in panfrost_job_hw_submit() under
job_lock if the context was destroyed while the job was being submitted.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 7022b1b2e0206..fbfdee42a4ae6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c

[ ... ]

> @@ -272,6 +303,16 @@ static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  			"JS: Submitting atom %p to js[%d][%d] with head=0x%llx AS %d",
>  			job, js, subslot, jc_head, cfg & 0xf);
>  	}
> +
> +	/*
> +	 * panfrost_jm_ctx_destroy() only stops the jobs it finds in
> +	 * pfdev->jobs[], so stop this one here if its context was destroyed
> +	 * while we were getting here, e.g. while waiting for the GPU to
> +	 * resume. Pairs with the WRITE_ONCE() in panfrost_jm_ctx_destroy().
> +	 */
> +	if (READ_ONCE(job->ctx->destroyed))
> +		panfrost_stop_job(pfdev, js, subslot, job);
> +
>  	spin_unlock(&pfdev->js->job_lock);
>  
>  	return 0;

[ ... ]

> @@ -1100,7 +1141,7 @@ int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
>  	if (!jm_ctx)
>  		return -EINVAL;
>  
> -	jm_ctx->destroyed = true;
> +	WRITE_ONCE(jm_ctx->destroyed, true);
>  
>  	/* Kill in-flight jobs */
>  	spin_lock(&pfdev->js->job_lock);
> @@ -1109,32 +1150,11 @@ int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
>  
>  		for (int j = ARRAY_SIZE(pfdev->jobs[0]) - 1; j >= 0; j--) {
>  			struct panfrost_job *job = pfdev->jobs[i][j];
> -			u32 cmd;
>  
>  			if (!job || job->base.entity != entity)
>  				continue;
>  
> -			if (j == 1) {
> -				/* Try to cancel the job before it starts */
> -				job_write(pfdev, JS_COMMAND_NEXT(i), JS_COMMAND_NOP);
> -				/* Reset the job head so it doesn't get restarted if
> -				 * the job in the first slot failed.
> -				 */
> -				job->jc = 0;
> -			}
> -
> -			if (panfrost_has_hw_feature(pfdev, HW_FEATURE_JOBCHAIN_DISAMBIGUATION)) {
> -				cmd = panfrost_get_job_chain_flag(job) ?
> -				      JS_COMMAND_HARD_STOP_1 :
> -				      JS_COMMAND_HARD_STOP_0;
> -			} else {
> -				cmd = JS_COMMAND_HARD_STOP;
> -			}
> -
> -			job_write(pfdev, JS_COMMAND(i), cmd);
> -
> -			/* Jobs can outlive their file context */
> -			job->engine_usage = NULL;
> +			panfrost_stop_job(pfdev, i, j, job);
>  		}
>  	}
>  	spin_unlock(&pfdev->js->job_lock);

[Severity: High]
Can panfrost_jm_ctx_destroy() ever reach panfrost_stop_job() when checking
job->base.entity != entity?

Before calling sched->ops->run_job() (panfrost_job_run()),
drm_sched_run_job_work() pops the job via drm_sched_entity_pop_job(), which
clears sched_job->entity:

drivers/gpu/drm/scheduler/sched_entity.c:drm_sched_entity_pop_job() {
    ...
	sched_job->entity = NULL;
	return sched_job;
}

Every job in pfdev->jobs[i][j] therefore has job->base.entity == NULL, so
job->base.entity != entity is always true and panfrost_jm_ctx_destroy()
never stops any already-queued job or clears its job->engine_usage, leaving
panfrost_dequeue_job() to write to freed panfrost_file_priv memory after
panfrost_postclose() when profiling is enabled.  Matching jobs by
job->ctx != jm_ctx instead of job->base.entity != entity would allow
panfrost_jm_ctx_destroy() to find the context's in-flight jobs.

[Severity: Critical]
This problem wasn't introduced by this patch, but can
panfrost_jm_ctx_release() trigger a NULL pointer dereference in
drm_sched_entity_flush() [1] when context creation fails?

In panfrost_jm_ctx_create(), if jm_ctx_prio_to_drm_sched_prio() or
drm_sched_entity_init() fails, err_put_jm_ctx calls panfrost_jm_ctx_put()
-> panfrost_jm_ctx_release(), which runs drm_sched_entity_destroy() on
every element of jm_ctx->slot_entity[]:

drivers/gpu/drm/panfrost/panfrost_job.c:panfrost_jm_ctx_release() {
    ...
	for (u32 i = 0; i < ARRAY_SIZE(jm_ctx->slot_entity); i++)
		drm_sched_entity_destroy(&jm_ctx->slot_entity[i]);
}

For any slot_entity where drm_sched_entity_init() failed or was not yet
reached, entity->rq is NULL, and drm_sched_entity_destroy() ->
drm_sched_entity_flush() unconditionally dereferences entity->rq->sched.

[1] https://sashiko.dev/#/bug/linux-1f36c24e-a878-442e-991c-1b387017bee6

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114062637.2447-1-arpe1618@gmail.com?part=2