Re: [PATCH v3 1/2] drm/sched: fix use-after-free of the fence timeline name

[Christian König]

On 9/3/26 10:46, Philipp Stanner wrote:
> On Wed, 2026-09-02 at 23:42 +0900, Jonghyuk Kim(MalHyuk) wrote:
>>
> 
> […]
> 
>>
>> The finished fence carried a .release callback solely to drop the
>> scheduled fence's reference. That callback kept the ops attached, leaving
>> get_timeline_name() reachable on a signalled finished fence with a
>> dangling ->sched. Drop the callback and move the reference handling
>> instead:
>>
>>  - The scheduled fence now holds a reference on the finished fence, so
>>    the finished fence, and with it the shared allocation, is released
>>    last. Its release drops the parent fence and that finished-fence
>>    reference; the finished fence is then freed from dma_fence_free().
> 
> The backend doesn't know about this (admittedly weird) shared-
> allocation phenomenon. I suppose the reason why we don't run into
> double-free is that the scheduled-fence does still implement ops-
>> release.
> 
> If that's the case, that needs to be documented in the code as a
> groundlayer for future cleanups (I suppose we should eliminate that
> shared allocation. If objects have distinct lifetimes, they should have
> distinct memory. It was probably done like that so that drivers can
> access both subfences through container_of()).
> 
> a la

Yes, the reason why that construct is necessary is because to_drm_sched_fence() needs to be able to cast from both scheduled fence and finished fence back to the parent object.

IIRC that was used by both the scheduler itself as well as and drivers, but I'm not sure if that is still the case. At least in amdgpu I tried to avoid casting from scheduled -> finished and only do from finished -> scheduled.

> 
> "TODO: this release callback should be removed, too, but can't because
> double-free"

Yes, I pointed that out before as well.

But those release callbacks are actually unproblematic for the problem at hand as far as I can see.

Regards,
Christian.

> 
>> scheduler. Unlike caching the name string, this also covers drivers whose
>> timeline name is dynamically allocated (drm/panthor, drm/xe).
> 
> It's good to have detailed commit messages, but I don't think hinting
> at that alternative solution (caching), which we don't implement, is
> necessary.
> 
>>
> 
> […]
> 
>> - it is the normal result for a foreign fence - and a signalled fence is
>> an already-satisfied dependency, so the scheduler's dependency-collapsing
>> optimisation is unaffected.
>>
> 
> I don't understand this phrase. You're saying that someone trying to
> register a dependency won't bother if he sees NULL?
> 
> 
>>
>>
> 
> […]
> 
>>  
>> -static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
>> -{
>> -	struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
>> -	struct drm_sched_fence *fence = to_drm_sched_fence(f);
>> -
>> -	if (!WARN_ON_ONCE(!fence))
>> -		kmem_cache_free(sched_fence_slab, fence);
>> -}
>> -
>>  /**
>>   * drm_sched_fence_free - free up an uninitialized fence
>>   *
>> @@ -132,21 +123,12 @@ static void drm_sched_fence_release_scheduled(struct dma_fence *f)
>>  	struct drm_sched_fence *fence = to_drm_sched_fence(f);
>>  
>>  	dma_fence_put(fence->parent);
>> -	call_rcu(&fence->finished.rcu, drm_sched_fence_free_rcu);
> 
> @Christian, Tvrtko, opinions on that?
> 
>> -}
>> -
>> -/**
>> - * drm_sched_fence_release_finished - drop extra reference
>> - *
>> - * @f: fence
>> - *
>> - * Drop the extra reference from the scheduled fence to the base fence.
>> - */
>> -static void drm_sched_fence_release_finished(struct dma_fence *f)
>> -{
>> -	struct drm_sched_fence *fence = to_drm_sched_fence(f);
>> -
>> -	dma_fence_put(&fence->scheduled);
>> +	/*
>> +	 * Drop the reference the scheduled fence holds on the finished fence.
>> +	 * The finished fence is released last and frees the shared allocation
>> +	 * from its dma_fence_free() (see drm_sched_fence_init()).
>> +	 */
>> +	dma_fence_put(&fence->finished);
>>  }
>>  
>>  static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
>> @@ -189,7 +171,13 @@ static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
>>  static const struct dma_fence_ops drm_sched_fence_ops_finished = {
>>  	.get_driver_name = drm_sched_fence_get_driver_name,
>>  	.get_timeline_name = drm_sched_fence_get_timeline_name,
>> -	.release = drm_sched_fence_release_finished,
>> +	/*
>> +	 * No .release callback: dma_fence detaches ->ops on signalling for
>> +	 * fences without .release/.wait, so get_timeline_name() is never called
>> +	 * on a signalled finished fence and cannot dereference a freed
>> +	 * scheduler. The shared allocation is freed from dma_fence_free() once
>> +	 * this fence's refcount drops - it is released last, after @scheduled.
>> +	 */
> 
> That comment is not necessary. The new code simply complies with the
> current idiomatic fence design. Comments are necessary at tricky bits
> or when one deviates from idiomatic usage.
> 
>>  	.set_deadline = drm_sched_fence_set_deadline_finished,
>>  };
>>  
>> @@ -233,6 +221,14 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
>>  		       &fence->lock, entity->fence_context, seq);
>>  	dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished,
>>  		       &fence->lock, entity->fence_context + 1, seq);
>> +
>> +	/*
>> +	 * Hold a reference on the finished fence from the scheduled fence, so
>> +	 * the finished fence (and the shared allocation) outlives @scheduled.
>> +	 * drm_sched_fence_release_scheduled() drops it; the finished fence is
>> +	 * therefore released last and frees the allocation via dma_fence_free().
>> +	 */
>> +	dma_fence_get(&fence->finished);
> 
> So what was the counter-part of this dma_fence_get() before?
> 
>>  }
>>  
>>  module_init(drm_sched_fence_slab_init);
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 6cb6f9546493..fb238f51c0ed 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -842,6 +842,15 @@ void drm_sched_job_cleanup(struct drm_sched_job *job)
>>  		 * been called.
>>  		 */
>>  		dma_fence_put(&job->s_fence->finished);
>> +		/*
>> +		 * Drop the initial reference on the scheduled fence. It no
>> +		 * longer has a .release callback dropping it (the finished
>> +		 * fence's .release was removed to allow ops-detach on signal),
> 
> Same as above, I think the comment should not focus on the past
> situation. The past should be tracked by the commit message; a comment
> should only mention the past if it's still relevant, for example to
> solve an open TODO.
> 
>> +		 * so the last put here lets drm_sched_fence_release_scheduled()
> 
> Strictly speaking, you don't know whether it's the last put().
> drm_sched_fence is a public object and drivers might have taken various
> references.
> 
>> +		 * run, which drops @parent and the scheduled fence's reference
>> +		 * on @finished. @finished is freed last, from dma_fence_free().
> 
> finished and scheduled would be freed through dma_fence_free()
> simultaneously, since they still share the allocation.
> 
> I think the shared allocation and how things are freed should be
> documented, but is drm_sched_job_cleanup() the right place?
> 
> 
>> +		 */
>> +		dma_fence_put(&job->s_fence->scheduled);
>>  		drm_sched_entity_stats_put(job->entity_stats);
>>  	} else {
>>  		/* The job was aborted before it has been committed to be run;
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 7a64cc11de08..686c3687944f 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -287,48 +287,58 @@ struct drm_sched_rq {
>>   * struct drm_sched_fence - fences corresponding to the scheduling of a job.
>>   */
>>  struct drm_sched_fence {
>>
> 
> […]
> 
>>  	struct dma_fence		finished;
>>  
>> +	/**
>> +	 * @scheduled: this fence is what will be signaled by the scheduler
>> +	 * when the job is scheduled.
>> +	 *
>> +	 * It holds a reference on @finished so that the shared allocation is
>> +	 * released only after @scheduled itself is done; its release drops
> 
> Won't it drop the endire drm_sched_fence?
> 
> I think this struct's documentation is the right place to document the
> life time and allocation pattern, together with maybe the relevant
> places in sched_fence.c
> 
> Then you could be a bit less verbose in the other code places; see
> above.
> 
>> +	 * that reference and the @parent one.
>> +	 */
>> +	struct dma_fence		scheduled;
>> +
>>  	/**
>>  	 * @deadline: deadline set on &drm_sched_fence.finished which
>>  	 * potentially needs to be propagated to &drm_sched_fence.parent
>>  	 */
>>  	ktime_t				deadline;
>>  
>> -        /**
>> -         * @parent: the fence returned by &drm_sched_backend_ops.run_job
>> -         * when scheduling the job on hardware. We signal the
>> -         * &drm_sched_fence.finished fence once parent is signalled.
>> -         */
>> +	/**
>> +	 * @parent: the fence returned by &drm_sched_backend_ops.run_job
>> +	 * when scheduling the job on hardware. We signal the
>> +	 * &drm_sched_fence.finished fence once parent is signalled.
>> +	 */
>>  	struct dma_fence		*parent;
>> -        /**
>> -         * @sched: the scheduler instance to which the job having this struct
>> -         * belongs to.
>> -         */
>> +	/**
>> +	 * @sched: the scheduler instance to which the job having this struct
>> +	 * belongs to.
>> +	 */
>>  	struct drm_gpu_scheduler	*sched;
>> -        /**
>> -         * @lock: the lock used by the scheduled and the finished fences.
>> -         */
>> +	/**
>> +	 * @lock: the lock used by the scheduled and the finished fences.
>> +	 */
>>  	spinlock_t			lock;
>> -        /**
>> -         * @owner: job owner for debugging
>> -         */
>> +	/**
>> +	 * @owner: job owner for debugging
>> +	 */
>>  	void				*owner;
> 
> Formatting fixes in a separate patch please, unless you need to modify
> those lines for semantically related reasons.
> 
> 
> Thanks
> P.