Re: [PATCH] drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Oct 13, 2025 at 06:01:49PM +0100, Tvrtko Ursulin wrote:
> 
> On 13/10/2025 15:39, Philipp Stanner wrote:
> > On Fri, 2025-10-03 at 10:26 +0100, Tvrtko Ursulin wrote:
> > > Drm_sched_job_add_dependency() consumes the fence reference both on
> > 
> > s/D/d
> 
> I cannot find the source right now but I am 99% sure someone pulled it on me
> in the past and educated me sentences should always start with a capital
> letter, even when it is not a proper word. Because in the past I was always
> doing the same as you suggest. I don't mind TBH.

Drm_sched_job_add_dependency breaks grep / searches, so I'm with Philipp s/D/d

Matt

> > > success and failure, so in the latter case the dma_fence_put() on the
> > > error path (xarray failed to expand) is a double free.
> > > 
> > > Interestingly this bug appears to have been present ever since
> > > ebd5f74255b9 ("drm/sched: Add dependency tracking"), since the code back
> > > then looked like this:
> > > 
> > > drm_sched_job_add_implicit_dependencies():
> > > ...
> > >         for (i = 0; i < fence_count; i++) {
> > >                 ret = drm_sched_job_add_dependency(job, fences[i]);
> > >                 if (ret)
> > >                         break;
> > >         }
> > > 
> > >         for (; i < fence_count; i++)
> > >                 dma_fence_put(fences[i]);
> > > 
> > > Which means for the failing 'i' the dma_fence_put was already a double
> > > free. Possibly there were no users at that time, or the test cases were
> > > insufficient to hit it.
> > > 
> > > The bug was then only noticed and fixed after
> > > 9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
> > > landed, with its fixup of
> > > 4eaf02d6076c ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies").
> > > 
> > > At that point it was a slightly different flavour of a double free, which
> > > 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> > > noticed and attempted to fix.
> > > 
> > > But it only moved the double free from happening inside the
> > > drm_sched_job_add_dependency(), when releasing the reference not yet
> > > obtained, to the caller, when releasing the reference already released by
> > > the former in the failure case.
> > 
> > That's certainly interesting, but is there a specific reason why you
> > include all of that?
> 
> Yes, because bug was attempted to be fixed two times already. So it is IMO
> worth having a write up on the history.
> > The code is as is, and AFAICS it's just a bug stemming from original
> > bugs present and then refactorings happening.
> > 
> > I would at least remove the old 'implicit_dependencies' function from
> > the commit message. It's just confusing and makes one look for that in
> > the current code or patch.It does say "back then" and it references the
> > commit so shouldn't be
> confusing.
> 
> The discussion is long for an additional reason that Fixes: targets not the
> original commit which did not get this right, but the last change which
> tried to fix it but did not. It sounded more logical to continue the chain
> on fixes but dunno. Could replace it with the original one just as well.
> > > As such it is not easy to identify the right target for the fixes tag so
> > > lets keep it simple and just continue the chain.
> > > 
> > > We also drop the misleading comment about additional reference, since it
> > > is not additional but the only one from the point of view of dependency
> > > tracking.
> > 
> > 
> > IMO that comment is nonsense. It's useless, too, because I can *see*
> > that a reference is being taken there, but not *why*.
> > 
> > Argh, these comments. See also my commit 72ebc18b34993
> > 
> > 
> > Anyways. Removing it is fine, but adding a better comment is better.
> > See below.
> > 
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > Fixes: 963d0b356935 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder")
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > 
> > Is there an error report that could be included here with a Closes:
> > tag?No, it did not come from any such system. Could perhaps put a link
> > to
> the report as Reference:
> https://lore.kernel.org/dri-devel/aNFbXq8OeYl3QSdm@stanley.mountain/
> 
> ?
> 
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Rob Clark <robdclark@chromium.org>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: Philipp Stanner <phasta@kernel.org>
> > > Cc: "Christian König" <ckoenig.leichtzumerken@gmail.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v5.16+
> > > ---
> > >   drivers/gpu/drm/scheduler/sched_main.c | 14 +++++---------
> > >   1 file changed, 5 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > > index 46119aacb809..aff34240f230 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -960,20 +960,16 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
> > >   {
> > >   	struct dma_resv_iter cursor;
> > >   	struct dma_fence *fence;
> > > -	int ret;
> > > +	int ret = 0;
> > >   	dma_resv_assert_held(resv);
> > >   	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
> > > -		/* Make sure to grab an additional ref on the added fence */
> > > -		dma_fence_get(fence);
> > > -		ret = drm_sched_job_add_dependency(job, fence);
> > > -		if (ret) {
> > > -			dma_fence_put(fence);
> > > -			return ret;
> > > -		}
> > > +		ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));
> > 
> > You still take a reference as before, but there is no comment anymore.
> > Can you add one explaining why a new reference is taken here?
> > 
> > I guess it will be something like "This needs a new reference for the
> > job", since you cannot rely on the one from resv.
> 
> I was mulling it over but then thought the kerneldoc for
> drm_sched_job_add_dependency() is good enough since it explains the function
> always consumes the reference and dma_resv_for_each_fence() does not hold
> one over the iteration. I can add it.
> 
> > 
> > > +		if (ret)
> > > +			break;
> > >   	}
> > > -	return 0;
> > > +	return ret;
> > 
> > 
> > That's an unnecessarily enlargement of the git diff because of style,
> > isn't it? Better keep the diff minimal here for git blame.
> 
> Given the relative size of the diff versus the function itself it did not
> look like a big deal to reduce to one return statement while touching it,
> but I can keep two returns just as well, no problem.
> 
> Regards,
> 
> Tvrtko
>