Message-ID: <sucdvuv4upf24srwf4ki5z2zubpu3m5rkz2k4opzgcdtc5yovw@jm3dslwedctc>
Date: Fri, 12 Sep 2025 11:57:51 +0100
From: Adrián Larumbe <adrian.larumbe@...labora.com>
To: Steven Price <steven.price@....com>
Cc: Boris Brezillon <boris.brezillon@...labora.com>, 
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, kernel@...labora.com, 
	Rob Herring <robh@...nel.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Subject: Re: [PATCH v2 2/4] drm/panfrost: Introduce JM contexts for manging
 job resources

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.price@....com> wrote:
> >
> >> On 10/09/2025 16:52, Boris Brezillon wrote:
> >>> On Wed, 10 Sep 2025 16:42:32 +0100
> >>> Steven Price <steven.price@....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

Powered by blists - more mailing lists