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

[Tvrtko Ursulin <tursulin@ursulin.net>]

On 07/09/2026 11:28, Tvrtko Ursulin wrote:
> 
> On 07/09/2026 10:42, Philipp Stanner wrote:
>> On Mon, 2026-09-07 at 10:15 +0100, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 04/09/2026 20:06, Philipp Stanner wrote:
>>>
>>> 8><
>>>
>>>> If you can think of a stupid and simple solution, shoot. The only thing
>>>> I can think of is moving the string into the dma_fence, as a hard copy
>>>> :)
>>>>
>>>>
>>>> In the mean time, my proposal is to keep aiming for removing
>>>> sched_fence->ops->release and fixing pvr and amdgpu.
>>>
>>> Fixing the drivers sounds like an obvious thing to try indeed. Along the
>>> same lines as it was done for xe and panthor. It is an already
>>> established and well understood approach so shouldn't be controversial.
>>> After that we can discuss in leisurely pace if something better is
>>> possible in the scheduler core.
>>>
>>> I understand its amdxdna, nouveau, and msm. Was it attempted so far? Is
>>> it significantly more complicated than it was for panthor and xe?
>>
>> How did the others fix that?
> 
> Combination of kfree_rcu, synchronize_rcu and storing the name in an 
> object protected by those:
> 
> 6bd90e700b42 ("drm/xe: Make dma-fences compliant with the safe access 
> rules")
> 299bc6d50b1b ("drm/xe/guc: Keep scheduler timeline name alive")
> efe24898485c ("drm/panthor: fix for dma-fence safe access rules")
> 
> Not too complicated on the overall. Simply ensure RCU grace period 
> between signaling the hw fence and freeing the ops, scheduler, name, all 
> that it is in the externally accessible dereference chain.
> 
>> If we look at nouveau:
>>
>> static void
>> nouveau_sched_fini(struct nouveau_sched *sched)
>> {
>>   struct drm_gpu_scheduler *drm_sched = &sched->base;
>>   struct drm_sched_entity *entity = &sched->entity;
>>
>>   wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
>>
>>   drm_sched_entity_fini(entity);
>>   drm_sched_fini(drm_sched);
>>
>>   /* Destroy workqueue after scheduler tear down, otherwise it might 
>> still
>>   * be in use.
>>   */
>>   if (sched->wq)
>>   destroy_workqueue(sched->wq);
>> }
>>
>>
>> We see that it
>>     1. stops accepting jobs from userspace (not visible here)
>>     2. waits until all hardware fences in this ring are signaled
>>     3. only then tears down drm_sched
> 
> I cannot do a very deep dive into nouveau at the moment. I see fence 
> itself is already freed with kfree_rcu so that's good. What is reachable 
> via the timeline name callback:
> 
>      struct nouveau_fence *fence = to_nouveau_fence(f);
>      struct nouveau_fence_chan *fctx = nouveau_fctx(fence);
> 
>      return !fctx->dead ? fctx->name : "dead channel";
> 
> Fence is presumably the fence so channel. Chagning to kfree_rcu in 
> nouveau_fence_context_put() there might be enough for that one.
> 
> For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
> 
> That makes scheduler timeline name vfunc safe:
> 
> static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
> {
>      struct drm_sched_fence *fence = to_drm_sched_fence(f);
>      return (const char *)fence->sched->name;
> 
> sched is then RCU protected. sched->name is already static so not a 
> concern.
> 
> As you say nouveau_sched_fini() only tears down the scheduler after 
> fences have been signaled it seems adding two new kfree_rcu make is safe.

P.S. Idea on how to test it from userspace:

https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2

But as nouveau does not export via sync_file you would need to adapt to 
export sync_file from syncobj.

Regards,

Tvrtko