Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free

[Philipp Stanner]

On Fri, 2026-09-04 at 17:31 +0900, Jonghyuk Kim(MalHyuk) wrote:
> On 9/4/26 10:20, Christian König wrote:
> Understood - ops-detach is about producer/module decoupling, not about the
> scheduler's lifetime relative to the driver, so framing it as "the complete
> fix" for this bug was wrong. I will drop the TODO patch as well.

Please wait a bit until we discussed it for a bit.

> 
> > I think we should rather re-consider patch 035219a760edb35ae9a9e96beba7f122e26a997b
> > ("dma-buf: dma-fence: Fix potential NULL pointer dereference"):
> > [...]
> > The problem is that we didn't considered that there a fence implementations
> > which still have a release or wait callbacks but rely on not needing to
> > return a string for a signaled fence.
> 
> That matches what I see in the code, thanks - this is the actual root cause and
> it is not drm/sched specific.
> 
> dma_fence_signal_timestamp_locked() only clears the ops pointer for fences that
> carry neither .release nor .wait:
> 
> 	ops = rcu_dereference_protected(fence->ops, true);
> 	if (!ops->release && !ops->wait)
> 		RCU_INIT_POINTER(fence->ops, NULL);
> 
> drm_sched_fence implements .release, so its ops survive signalling. Before
> 035219a760ed the helpers gated on the signaled bit, so such a fence returned
> the static string and the producer callback was never reached. Since that
> commit they gate on the ops pointer alone, so get_timeline_name() /
> get_driver_name() are called on a long-signalled fence - which is exactly the
> window my report hits, with ->sched already freed.
> 
> To be clear, I am not suggesting a revert: 035219a760ed fixes a real problem,
> namely that "set signaled bit, then NULL the ops" and "load ops, then check the
> signaled bit" can be reordered on weakly ordered platforms, and using the ops
> pointer as the synchronization point solves that elegantly. That property
> should stay.
> 
> What seems to be missing is that the ops check answers "may I dereference the
> pointer", not "may I call into the producer". The dma-fence rules say the
> latter is not allowed once the fence is signalled, so I think both conditions
> are needed:
> 
> 	ops = rcu_dereference(fence->ops);
> 	if (ops && !dma_fence_test_signaled_flag(fence))
> 		return (const char __rcu *)ops->get_timeline_name(fence);
> 	else
> 		return (const char __rcu *)"signaled-timeline";
> 
> The ops load keeps the RCU/ordering guarantee from 035219a760ed, and the
> signaled check restores the contract. That fixes every implementation which
> keeps .release or .wait and assumes it is not called after signalling, rather
> than just drm/sched, and it puts no lifetime burden on drivers.
> 
> Philipp, since 035219a760ed is yours - do you agree with adding the signaled
> check back on top of the ops check? I would rather have your ack on that before
> I respin.

That commit has to stay because it solves an ordering problem on some
architectures.

I do agree to having a conversation with Christian in my other mail,
where I wonder whether it would be better if a driver does the is-
signaled check, holding the lock.

These things are extremely complicated and cannot be rushed. We
discussed dma_fence synchronization for weeks this summer and concluded
that there is no easy solution at hands for some problems, notably
because of callbacks and legacy callbacks, and also because Christian
and I disagree on to what degree a fence should be locked.

> 
> One thing I noticed while checking the callers: the tracepoints in
> include/trace/events/dma_fence.h and drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> call fence->ops->get_driver_name() / get_timeline_name() directly instead of
> going through the helpers, so they are not covered by the above. That looks
> like a pre-existing and much narrower exposure (tracing only), but let me know
> if you want it addressed in the same series or separately.

Do you generate part of your mails with an LLM? It seems to be a huge
corpus for 11min of time between receiving and answer


P.