[PATCH 1/2] drm/panfrost: Hold a reference to the MMU context of a job

[PATCH 1/2] drm/panfrost: Hold a reference to the MMU context of a job

[Jiwoo Kim]

A job stores the MMU context of its file in job->mmu without taking a
reference to it. Once the file is closed, the context is only kept
alive by the BO mappings that still reference it, and a job may be
submitted with no BOs at all.

When the file is closed while such a job is still running,
panfrost_jm_close() only hard-stops the job without waiting for it, and
panfrost_postclose() then drops the file's reference to the context,
which frees it unless BO mappings of other jobs still hold it. The job
interrupt handling that follows calls panfrost_mmu_as_put() on the
freed job->mmu, and panfrost_core_dump() also reads job->mmu if the job
times out.

Take a reference to the MMU context when the job is created and drop
it when the job is cleaned up.

This was found with an AI coding assistant, which also drafted this
patch and its changelog. It has not been reproduced, and the change
has only been compile-tested.

Fixes: 6e516faf0431 ("drm/panfrost: Job should reference MMU not file_priv")
Cc: stable@vger.kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
1/2 and 2/2 fix separate lifetime problems and do not depend on each
other.

 drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 7c348a0bf1ee..afda794e2aeb 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -331,7 +331,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 	job->jc = args->jc;
 	job->requirements = args->requirements;
 	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
-	job->mmu = file_priv->mmu;
+	job->mmu = panfrost_mmu_ctx_get(file_priv->mmu);
 	job->ctx = panfrost_jm_ctx_get(jm_ctx);
 	job->engine_usage = &file_priv->engine_usage;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 4ddf4b722ddf..7022b1b2e020 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -377,6 +377,7 @@ static void panfrost_job_cleanup(struct kref *ref)
 		kvfree(job->bos);
 	}
 
+	panfrost_mmu_ctx_put(job->mmu);
 	panfrost_jm_ctx_put(job->ctx);
 	kfree(job);
 }

base-commit: d48ea86919434695da12d280dccab72b094397e1
-- 
2.43.0

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

[Jiwoo Kim]

panfrost_jm_ctx_destroy(), which runs on JM_CTX_DESTROY and when the
file is closed, sets jm_ctx->destroyed and then stops the jobs of the
context that it finds in pfdev->jobs[]. panfrost_job_run() checks the
flag once, without any lock, and panfrost_job_hw_submit() can then
sleep in pm_runtime_get_sync() before it queues the job in
pfdev->jobs[] under job_lock. A job that is between the check and the
queueing when its context is destroyed is therefore neither cancelled
nor stopped, and can then be started after its context, and possibly
its file, are gone. With profiling enabled, its engine_usage also still
points into the panfrost_file_priv that panfrost_postclose() frees, and
panfrost_dequeue_job() writes to it when the job completes.

Before commit 6aa8bc58ac88 ("drm/panfrost: Introduce JM contexts for
manging job resources"), the close path called
drm_sched_entity_destroy(), which waits for a run_job() call in
progress on the entity, before it looked for the jobs to stop. The
entities are now only destroyed when the last reference to the context
is dropped.

Move the per-job part of panfrost_jm_ctx_destroy() into a helper, and
call it from panfrost_job_hw_submit() on the job it has just queued if
the context has been destroyed by then. The check in
panfrost_job_hw_submit() and the loop in panfrost_jm_ctx_destroy() both
run under job_lock, and panfrost_jm_ctx_destroy() sets the flag before
it takes the lock, so such a job is either found by
panfrost_jm_ctx_destroy() or sees the flag, and it then goes through
the same stop and completion path in both cases. Cancelling the job in
panfrost_job_hw_submit() instead would leave a gap in the fence seqnos
of the slot, which the job chain flags rely on. Use READ_ONCE() and
WRITE_ONCE() for the flag, as it is written outside job_lock and read
without it in panfrost_job_run().

This was found with an AI coding assistant, which also drafted this
patch and its changelog. It has not been reproduced, and the change
has only been compile-tested.

Fixes: 6aa8bc58ac88 ("drm/panfrost: Introduce JM contexts for manging job resources")
Cc: stable@vger.kernel.org
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---
panfrost_stop_job() is the loop body of panfrost_jm_ctx_destroy() moved
into a helper unchanged, apart from the lockdep assertion.

Arun Iyengar's "Resume long jobs instead of resetting" series also
changes panfrost_job_hw_submit(); this patch does not depend on it:
https://lore.kernel.org/r/20261113-panfrost-long-jobs-v3-0-80ae78d38f6e@gmail.com

 drivers/gpu/drm/panfrost/panfrost_job.c | 68 ++++++++++++++++---------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7022b1b2e020..fbfdee42a4ae 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -202,6 +202,37 @@ panfrost_enqueue_job(struct panfrost_device *pfdev, int slot,
 	return 1;
 }
 
+/* Stop a job of a destroyed JM context, found in pfdev->jobs[slot][subslot]. */
+static void panfrost_stop_job(struct panfrost_device *pfdev, int slot,
+			      unsigned int subslot, struct panfrost_job *job)
+{
+	u32 cmd;
+
+	lockdep_assert_held(&pfdev->js->job_lock);
+
+	if (subslot == 1) {
+		/* Try to cancel the job before it starts */
+		job_write(pfdev, JS_COMMAND_NEXT(slot), 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(slot), cmd);
+
+	/* Jobs can outlive their file context */
+	job->engine_usage = NULL;
+}
+
 static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
 {
 	struct panfrost_device *pfdev = job->pfdev;
@@ -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;
@@ -404,7 +445,7 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	struct dma_fence *fence = NULL;
 	int ret;
 
-	if (job->ctx->destroyed)
+	if (READ_ONCE(job->ctx->destroyed))
 		return ERR_PTR(-ECANCELED);
 
 	if (unlikely(job->base.s_fence->finished.error))
@@ -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);
-- 
2.43.0

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

[sashiko-bot]

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