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

[Christian König]

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.