linux-kernel - Re: [PATCH] drm/sched: Document run

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z2VunIJ4ltfW_xqD@pollux>
Date: Fri, 20 Dec 2024 14:18:20 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Christian König <christian.koenig@....com>
Cc: Philipp Stanner <phasta@...nel.org>, Luben Tuikov <ltuikov89@...il.com>,
	Matthew Brost <matthew.brost@...el.com>,
	Philipp Stanner <pstanner@...hat.com>,
	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>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
	Tvrtko Ursulin <tursulin@...ulin.net>,
	Andrey Grodzovsky <andrey.grodzovsky@....com>
Subject: Re: [PATCH] drm/sched: Document run_job() refcount hazard

On Fri, Dec 20, 2024 at 01:53:34PM +0100, Christian König wrote:
> Am 20.12.24 um 13:45 schrieb Philipp Stanner:
> > From: Philipp Stanner <pstanner@...hat.com>
> > 
> > drm_sched_backend_ops.run_job() returns a dma_fence for the scheduler.
> > That fence is signalled by the driver once the hardware completed the
> > associated job. The scheduler does not increment the reference count on
> > that fence, but implicitly expects to inherit this fence from run_job().
> > 
> > This is relatively subtle and prone to misunderstandings.
> > 
> > This implies that, to keep a reference for itself, a driver needs to
> > call dma_fence_get() in addition to dma_fence_init() in that callback.
> > 
> > It's further complicated by the fact that the scheduler even decrements
> > the refcount in drm_sched_run_job_work() since it created a new
> > reference in drm_sched_fence_scheduled(). It does, however, still use
> > its pointer to the fence after calling dma_fence_put() - which is safe
> > because of the aforementioned new reference, but actually still violates
> > the refcounting rules.
> > 
> > Improve the explanatory comment for that decrement.
> > 
> > Move the call to dma_fence_put() to the position behind the last usage
> > of the fence.
> > 
> > Document the necessity to increment the reference count in
> > drm_sched_backend_ops.run_job().
> > 
> > Cc: Christian König <christian.koenig@....com>
> > Cc: Tvrtko Ursulin <tursulin@...ulin.net>
> > Cc: Andrey Grodzovsky <andrey.grodzovsky@....com>
> > Signed-off-by: Philipp Stanner <pstanner@...hat.com>
> > ---
> >   drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
> >   include/drm/gpu_scheduler.h            | 20 ++++++++++++++++----
> >   2 files changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 7ce25281c74c..d6f8df39d848 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > +	 *
> > +	 * @sched_job: the job to run
> > +	 *
> > +	 * Returns: dma_fence the driver must signal once the hardware has
> > +	 *	completed the job ("hardware fence").
> > +	 *
> > +	 * Note that the scheduler expects to 'inherit' its own reference to
> > +	 * this fence from the callback. It does not invoke an extra
> > +	 * dma_fence_get() on it. Consequently, this callback must return a
> > +	 * fence whose refcount is at least 2: One for the scheduler's
> > +	 * reference returned here, another one for the reference kept by the
> > +	 * driver.
> 
> Well the driver actually doesn't need any extra reference. The scheduler
> just needs to guarantee that this reference isn't dropped before it is
> signaled.

I think he means the reference the driver's fence context has to have in order
to signal that thing eventually.

> 
> Regards,
> Christian.
> 
> >   	 */
> >   	struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
>