On 9/4/26 10:31, Philipp Stanner wrote:
> On Fri, 2026-09-04 at 10:20 +0200, Christian König wrote:
>>
>
> […]
>
>>>
>>> +/*
>>> + * TODO: Both fences implement .release, so dma_fence keeps their ops
>>> attached
>>> + * after signalling. Dropping the callbacks would let dma_fence detach the
>>> ops,
>>> + * after which neither get_timeline_name() nor get_driver_name() can run
>>> against
>>> + * a freed scheduler or an unloaded module - the complete fix. It first
>>> requires
>>> + * auditing every to_drm_sched_fence() caller, since ops-detach makes the
>>> helper
>>> + * return NULL for a signalled fence. See Documentation/gpu/todo.rst.
>>> + */
>>
>> 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.
>
> It can be used to solve that problem though, can it not?
Yes, but I think forcing dma_fence implementations to drop their release
callback to fix lifetime problems with the driver and timeline name functions
is a bad idea.
We should keep this fix simple and focused so that we can easily backport it.
Fixing all dma_fence implementations to not need the release callback is
something I really like to have as well, but not to fix this issue here.
>
> The underlying problem is that the driver has no chance to figure out
> when the scheduler is actually done with all the sched_fences.
>
> Remember our lengthy discussions about drm_sched_fini(). Maybe we want
> to reconsider providing a function with which the driver can wait until
> the scheduler is done with all finished_fences?
The problem is that won't help unless we either add more checks or fix the
checks in dma_fence_driver_name()/dma_fence_timeline_name().
The dma_fence object can trivially outlive both the driver and the scheduler
instance it originally issued.
>>
>> I think we should rather re-consider patch
>> 035219a760edb35ae9a9e96beba7f122e26a997b ("dma-buf: dma-fence: Fix potential
>> NULL pointer dereference"):
>>
>> Here we changed the check in dma_fence_driver_name() and
>> dma_fence_timeline_name():
>>
>> @@ -1167,7 +1167,7 @@ const char __rcu *dma_fence_driver_name(struct
>> dma_fence *fence)
>>
>> /* RCU protection is required for safe access to returned string */
>> ops = rcu_dereference(fence->ops);
>> - if (!dma_fence_test_signaled_flag(fence))
>> + if (ops)
>> return (const char __rcu *)ops->get_driver_name(fence);
>> else
>> return (const char __rcu *)"detached-driver";
>>
>> 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.
>>
>
> Could we move the signaled check to amdgpu and pvr?
Yes we could. I also considered that. But I would rather like to see it handled
in the common dma_fence code.
If I remember correctly either Tvrko, you or somebody else was in favor of
doing "if (!dma_fence_test_signaled_flag(fence) && ops)" here but I though that
this was unnecessary and we would rather remove the release callbacks. Maybe I
was wrong with that.
> IOW, we keep the solution presented here (removing ops->release for
> finished-fence) and the few drivers that check whether a fence is their
> own first do a locked dma_fence_is_signaled() check?
Works for me as well, but as I said I would rather like to keep it simple and
stupid for backporting.
Regards,
Christian.
>
>
> P.
