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

[Tvrtko Ursulin]

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