On 11.09.2025 10:30, Steven Price wrote:
> On 10/09/2025 17:50, Boris Brezillon wrote:
> > On Wed, 10 Sep 2025 16:56:43 +0100
> > Steven Price <steven.pr...@arm.com> wrote:
> >
> >> On 10/09/2025 16:52, Boris Brezillon wrote:
> >>> On Wed, 10 Sep 2025 16:42:32 +0100
> >>> Steven Price <steven.pr...@arm.com> wrote:
> >>>
> >>>>> +int panfrost_jm_ctx_create(struct drm_file *file,
> >>>>> + struct drm_panfrost_jm_ctx_create *args)
> >>>>> +{
> >>>>> + struct panfrost_file_priv *priv = file->driver_priv;
> >>>>> + struct panfrost_device *pfdev = priv->pfdev;
> >>>>> + enum drm_sched_priority sched_prio;
> >>>>> + struct panfrost_jm_ctx *jm_ctx;
> >>>>> +
> >>>>> + int ret;
> >>>>> +
> >>>>> + jm_ctx = kzalloc(sizeof(*jm_ctx), GFP_KERNEL);
> >>>>> + if (!jm_ctx)
> >>>>> + return -ENOMEM;
> >>>>> +
> >>>>> + kref_init(&jm_ctx->refcnt);
> >>>>> +
> >>>>> + /* Same priority for all JS within a single context */
> >>>>> + jm_ctx->config = JS_CONFIG_THREAD_PRI(args->priority);
> >>>>> +
> >>>>> + ret = jm_ctx_prio_to_drm_sched_prio(file, args->priority,
> >>>>> &sched_prio);
> >>>>> + if (ret)
> >>>>> + goto err_put_jm_ctx;
> >>>>> +
> >>>>> + for (u32 i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> >>>>> + struct drm_gpu_scheduler *sched =
> >>>>> &pfdev->js->queue[i].sched;
> >>>>> + struct panfrost_js_ctx *js_ctx = &jm_ctx->slots[i];
> >>>>> +
> >>>>> + ret = drm_sched_entity_init(&js_ctx->sched_entity,
> >>>>> sched_prio,
> >>>>> + &sched, 1, NULL);
> >>>>> + if (ret)
> >>>>> + goto err_put_jm_ctx;
> >>>>> +
> >>>>> + js_ctx->enabled = true;
> >>>>> + }
> >>>>> +
> >>>>> + ret = xa_alloc(&priv->jm_ctxs, &args->handle, jm_ctx,
> >>>>> + XA_LIMIT(0, MAX_JM_CTX_PER_FILE), GFP_KERNEL);
> >>>>> + if (ret)
> >>>>> + goto err_put_jm_ctx;
> >>>>
> >>>> On error here we just jump down and call panfrost_jm_ctx_put() which
> >>>> will free jm_ctx but won't destroy any of the drm_sched_entities. There
> >>>> seems to be something a bit off with the lifetime management here.
> >>>>
> >>>> Should panfrost_jm_ctx_release() be responsible for tearing down the
> >>>> context, and panfrost_jm_ctx_destroy() be nothing more than dropping the
> >>>> reference?
> >>>
> >>> The idea was to kill/cancel any pending jobs as soon as userspace
> >>> releases the context, like we were doing previously when the FD was
> >>> closed. If we defer this ctx teardown to the release() function, we're
> >>> basically waiting for all jobs to complete, which:
> >>>
> >>> 1. doesn't encourage userspace to have proper control over the contexts
> >>> lifetime
> >>> 2. might use GPU/mem resources to execute jobs no one cares about
> >>> anymore
> >>
> >> Ah, good point - yes killing the jobs in panfrost_jm_ctx_destroy() makes
> >> sense. But we still need to ensure the clean-up happens in the other
> >> paths ;)
> >>
> >> So panfrost_jm_ctx_destroy() should keep the killing jobs part, butthe
> >> drm scheduler entity cleanup should be moved.
> >
> > The thing is, we need to call drm_sched_entity_fini() if we want all
> > pending jobs that were not queued to the HW yet to be cancelled
> > (_fini() calls _flush() + _kill()). This has to happen before we cancel
> > the jobs at the JM level, otherwise drm_sched might pass us new jobs
> > while we're trying to get rid of the currently running ones. Once we've
> > done that, there's basically nothing left to defer, except the kfree().
>
> Ok, I guess that makes sense.
>
> In which case panfrost_jm_ctx_create() just needs fixing to fully tear
> down the context in the event the xa_alloc() fails. Although that makes
> me wonder whether the reference counting on the JM context really
> achieves anything. Are we ever expecting the context to live past the
> panfrost_jm_ctx_destroy() call?
We still need reference counting, otherwise there would be a racy window between
the submission and context destruction ioctls, in which a context that has just
been released is still owned by a newly created job leading to UAF.
> Indeed is it even possible to have any in-flight jobs after
> drm_sched_entity_destroy() has returned?
My understanding of drm_sched_entity_destroy() is that, after it returns,
no jobs can be in-flight any more and the entity is rendered unusable by
any new jobs. This can lead to the unpleasant situation in which a thread
tries to submit a new job and gets a context reference right before another
thread takes precedence and destroys it, causing the scheduler entities to
be unusable.
Then drm_sched_job_init() would contain a reference to an invalid entity,
which further down the line would cause drm_sched_entity_push_job() to report
a DRM_ERROR warning that te entity is stopped, which should never happen,
because drm_sched_entity_push_job() must always suceed.
> Once all the sched entities have been destroyed there doesn't really
> seem to be anything left in struct panfrost_jm_ctx.
We've thought of a new approach whereby a context would be flagged as destroyed
inside panfrost_jm_ctx_destroy(), destruction of scheduler entities done at
context
release time and then cancelling new jobs that had been queued after context
destruction in the .run_job scheduler function if they notice the context
is so flagged.
> Thanks,
> Steve
Cheers,
Adrian Larumbe
