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

[Philipp Stanner]

On Fri, 2026-09-04 at 14:49 +0200, Christian König wrote:
> On 9/4/26 10:31, Philipp Stanner wrote:
> > On Fri, 2026-09-04 at 10:20 +0200, Christian König wrote:
> > > 

[…]

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

The issue we're facing is not so much related with life times, is it?

Even if the driver correctly tears down the scheduler (i.e., signal all
hardware fences), this problem still exists.

> 
> We should keep this fix simple and focused so that we can easily backport it.

My hope would be that the dma_fence backend solution can be used that
*already exists*. That could then both be simple and maintainable.


Correct me if I'm wrong, but it seems we have not found an alternative
solution that can work yet?

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

Right.

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

I hope that wasn't me because that again looks very racy.

I think that there is no way around using the spinlock. As I have
pointed out many times, the fact that the signaled-bit is set with lock
protection and read without it is fundamentally broken :(

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

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.

I think that will have to entail a signaled-check. Question would then
be whether we place it in the driver or the backend.


P.