Re: [RFC PATCH v2 0/3] Solve dma_fence's problems by locking

[Philipp Stanner <phasta@mailbox.org>]

On Tue, 2026-10-27 at 15:59 +0100, Christian König wrote:
> On 10/21/26 14:49, Philipp Stanner wrote:
> > On Wed, 2026-10-21 at 13:53 +0200, Christian König wrote:
> > > On 10/20/26 17:36, Philipp Stanner wrote:
> > […]
> > 
> > > > 
> > > > Nope, I want to protect shared data, which is the signaled-state. Which
> > > > is currently a race-condition worked around with RCU (and sometimes
> > > > with manually doing the right thing (locking!)) [1], stemming from the
> > > > fact that the fence's spinlock is not used consistently.
> > > 
> > > The fence spinlock is consistently used for protecting the fence
> > > signaling, in other words it always happens with the spinlock held,
> > > same for adding callbacks.
> > > 
> > > The only thing which happens without the spinlock held is the fast
> > > path checking of the signaled and enable_signaling bits and as far as
> > > I can see that is ok as well.
> > 
> > No, it isn't. It makes the API fragile.
> > 
> > 	commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> 
> This bug is exactly what I'm trying to prevent here.

What is your proposed strategy for preventing it?

> 
> This problem only exists because we call the backend operations with
> the lock held!

Nope, this particular bug exists because Nouveau thought that it can
drop its reference when dma_fence_is_signaled() returns true, which is
not the case, because the fence backend has a race condition there.

A race condition that was allowed for performance reasons.


> What nouveau does is to delegate the cleanup since it can't do that
> in the signaling handler because of the lock inversion.

What signaling handler are you referring to?

> 
> This problem doesn't happen any more if you just completely drop the
> approach of calling the backend operations with the fence lock held
> and then re-structure the cleanup handling in Nouveau.

ops->signaled() is typically not called with the lock held. You might
be referring to ops->enable_signaling(), which is consistently locked.

> 
> > 	commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free
> > of VM")
> 
> I already discussed that with Simona back then. Simona actually
> explicitly rejected my approach to take the spinlock to check for
> signaling. Pointing out that amdgpu is using the API in a wrong way.
> 
> The problem is that amdgpu doesn't track the callback structure and
> so can't remove the callback. It's on my TODO list every since to fix
> that miss use of the API.

You mean the issue is that amdgpu cannot deregister callbacks?

I don't see how that's related. The lock-unlock-cycle clearly ensures
that amdgpu cannot continue while the fence is signaling.

To clarify: our conversation circles around two partially independent
topics:

a) the signaled-bit is read without lock-protection as a fast path in
dma_fence_is_signaled() and

b) the ops are not consistently called with the lock held.

> 
> > Tell me one thing:
> > If locking is the wrong approach, then how come it was added to
> > literally all callbacks except for set_deadline()? Seems to me that it
> > must have solved some issues.
> 
> Well, it's because concerns with the lock inversion brought up by
> Alex and me was ignored and the patches merged anyway.
> 
> This is just another one of those DMA-buf design problems which would
> have been fully preventable if I would have clearly NAKed that stuff
> from the very beginning.

My impression was more that central design criteria for dma_fence (like
module unload of the driver who issued your fence, which is designed to
live arbitrarily long) were ignored, there was no clear guidance for
drivers how they should use what callback, and then driver problems
were solved in dma_fence, for instance with this shared lock. So same
story as with drm_sched.

If *everything* would have been locked from the get-go, that would have
forced drivers to design their locking correctly, and decide wisely
what tasks they want to perform where.

> 
> Back then I made the mistake to believe the argumentation you come up
> here as well and so I don't explicitly rejected that approach, but by
> now we have strong evidence that this was a mistake and I'm trying to
> fix that mistake ever since.

Well, I am a different person with my own background that informs my
thinking. Please keep that in mind.

The "strong evidence" you claim to exist has still not been shown to
me. I show you concrete bugs that we could have prevented, but the only
piece of code you ever showed me where something would break is the
list_del() in nouveau.

> 
> We should have never taken the lock for the callbacks in the first
> place, this just made the implementation much more complicated and
> caused way more problems than the few raised by putting the ops
> pointer under RCU.

Lock-inversions are not being solved by just not taking the lock, they
are addressed by having good lock design. We recently solved exactly
that in the Rust JobQueue, where drivers also reported lock-inversions,
but we could eliminate them with just a few discussions, discovering
that the driver does not need these locks when interacting with the
JobQueue.

Often enough btw the only object the driver really needs is the
sequence number anyways, which can trivially be passed and shared
around.

Now with dma_fence, I strongly suspect that the problem came to be
because of no clear guidance by dma_fence as to what the driver must
and must not do in its ops implementation.

The only evidence I have ever seen is the fence-list-lock, and it's
just not clear at all why any callback would need access to that.


Let's walk it through together, shall we?

ops->release() and ->wait(): deprecated

ops->get_XXX_name() -> never needs driver locks

ops->signaled(): "Driver, here you read an atomic integer, the sequence
number, and tell the fence backend whether this fence shall be regarded
as completed"
No need for any driver lock.
If this particular driver also signals fences through an interrupt
handler, it can take a list lock, then signal the fence. The ops must
not do that.
Why would a driver need both modes, sw *and* hw signaling? And why then
is it supposedly the case that both paths need to keep a list updated?


ops->enable_signaling(): "Make sure that this fence will get signaled.
Register an interrupt or schedule a work item". These locks also don't
interfere with the fence-lock.

ops->set_deadline(): clock up the CPU frequency. The only implementor
trivially does that through a work item, too – after voluntarily taking
the fence lock over the entire section.


As you saw from our recent amdgpu discussions, the confusion of what a
ops is even good for caused drivers to mess around with the fence
directly in their callbacks, checking for the signaled state etc.

It was too much freedom that has caused the trouble. There are no
canonical solutions now.


So except for updating the linked list in ops, can you show me any code
example where a lock inversion would occur? I'm quite confident that it
will turn out that the driver has no good justification for performing
the related acts in the ops in the first place.


P.