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

[Jonghyuk Kim(MalHyuk)]

On 9/4/26 10:20, Christian König wrote:
>> +	return fence->sched_name;
>
> I don't think that this actually solves the problem, the sched_name still
> needs to be kept alive until all fences are destroyed and that is something
> drivers don't want/can do.

Agreed, and that is the same objection Tvrtko raised against v1. Caching the
pointer only moves the lifetime requirement from the scheduler to the string,
and the documentation hunk I added just pushes that requirement onto drivers.
I will drop that patch.

>> +/*
>> + * TODO: Both fences implement .release, so dma_fence keeps their ops attached
>> + * after signalling. Dropping the callbacks would let dma_fence detach the ops,
>
> That sounds like a bad idea as well.
>
> Dropping the fence->ops is to detach the fence from the module which
> originally issued it and not solve lifetime problems between the scheduler
> and the driver.

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.

> 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.

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.

So for v5 I plan:

  1. dma-buf/dma-fence: add the signaled check back to dma_fence_driver_name()
     and dma_fence_timeline_name(), Fixes: 035219a760ed, Cc: stable.
  2. Keep the KUnit regression test - it exercises exactly this path through
     dma_fence_timeline_name() and needs no change; it also picked up the
     teardown issue the review bot flagged, which I have fixed locally by
     using kunit_add_action_or_reset() + kunit_release_action().

and drop the drm/sched caching and TODO patches. I will wait for your and
Philipp's input before sending it.

Thanks,
Jonghyuk