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

[Tvrtko Ursulin <tursulin@ursulin.net>]

A gentle ping - any takers to double check my analysis and review the below?

Regards,

Tvrtko

On 03/10/2025 10:26, Tvrtko Ursulin wrote:
> Drm_sched_job_add_dependency() consumes the fence reference both on
> 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.
> 
> 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.
> 
> 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>
> 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));
> +		if (ret)
> +			break;
>   	}
> -	return 0;
> +	return ret;
>   }
>   EXPORT_SYMBOL(drm_sched_job_add_resv_dependencies);
>