Skip to content

Commit 5801e65

Browse files
Tvrtko UrsulinPhilipp Stanner
authored andcommitted
drm/sched: Fix potential double free in drm_sched_job_add_resv_dependencies
When adding dependencies with drm_sched_job_add_dependency(), that function 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 commit ebd5f74 ("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 commit 9c2ba26 ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2") landed, with its fixup of commit 4eaf02d ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies"). At that point it was a slightly different flavour of a double free, which commit 963d0b3 ("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. While fixing we also improve the comment and explain the reason for taking the reference and not dropping it. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Fixes: 963d0b3 ("drm/scheduler: fix drm_sched_job_add_implicit_dependencies harder") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: 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+ Signed-off-by: Philipp Stanner <phasta@kernel.org> Link: https://lore.kernel.org/r/20251015084015.6273-1-tvrtko.ursulin@igalia.com
1 parent 2aab1f9 commit 5801e65

File tree

1 file changed

+7
-6
lines changed

1 file changed

+7
-6
lines changed

drivers/gpu/drm/scheduler/sched_main.c

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -965,13 +965,14 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
965965
dma_resv_assert_held(resv);
966966

967967
dma_resv_for_each_fence(&cursor, resv, usage, fence) {
968-
/* Make sure to grab an additional ref on the added fence */
969-
dma_fence_get(fence);
970-
ret = drm_sched_job_add_dependency(job, fence);
971-
if (ret) {
972-
dma_fence_put(fence);
968+
/*
969+
* As drm_sched_job_add_dependency always consumes the fence
970+
* reference (even when it fails), and dma_resv_for_each_fence
971+
* is not obtaining one, we need to grab one before calling.
972+
*/
973+
ret = drm_sched_job_add_dependency(job, dma_fence_get(fence));
974+
if (ret)
973975
return ret;
974-
}
975976
}
976977
return 0;
977978
}

0 commit comments

Comments
 (0)