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

[Alessio Belle <Alessio.Belle@imgtec.com>]

Hi,

On Mon, 2026-09-07 at 11:42 +0200, 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?
> 
> 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
> 
> Then nouveau might unload or free up resources.
> 
> The nasty thing is that I don't see how nouveau misbehaves here and how
> the stuff might be fixed.
> 
> The problem is that the sched_fence implements ops->release, so the
> check doesn't take effect.
> 
> Moreover, even if we did remove ops->release in drm_sched, it would
> still be a race: a driver's contract is the hardware_fence, the rule
> being that you have to signal those. So after signaling the last
> hardware_fence, you could actually start releasing resources, but it
> might be that finished_fences are still in-flight and are unsignaled.
> 
> So we have some sort of fence -> fence race here, too.
> 
> > 
> > As for regarding the 035219a760ed ("dma-buf: dma-fence: Fix potential
> > NULL pointer dereference") sub-thread - I did not manage to penetrate
> > the consensus there - whether it was established that it needs adding
> > the is signaled check back (with additional memory barriers, like v1 of 
> > that patch) or not? Regardless of fixing the drivers or what?
> 
> 
> As far as my understanding goes this is the only way to get this right
> for everyone, i.e. also users who implement ops->release(). Then at
> least the driver could unload after signalling all its fences (with the
> exception of those who have a shared spinlock maybe).
> 
> 
> However, I suppose then we would then have two mechanisms, one dancing
> with RCU around the ops pointer, the other checking whether the fence
> is signaled, presumably with manual ordering through barriers.
> 
> My first guess would be that maybe we should only rely on the signaled-
> state and leave the ops-pointer untouched? This should also work for
> pvr, notably.

It seems in the meantime the discussion moved towards fixing the common code, at
least in the short term (correct?), but pointing out anyway that if needed,
having a similar fix on the powervr side would also be fine for us especially
for backporting purposes.

We are looking at alternatives to avoid accessing sched_fence->sched->ops, but
can't tell yet if it can be done without loss of functionality and/or
performance and how safe it is to backport.

Thanks,
Alessio

> 
> 
> P.