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

On 10/21/26 14:49, Philipp Stanner wrote:

On Wed, 2026-10-21 at 13:53 +0200, Christian König wrote:

quoted

On 10/20/26 17:36, Philipp Stanner wrote:

[…]

quoted

quoted

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.

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

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

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.

	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.

There are certainly more related bugs around.

Also think of the various ordering issues between signaled-bit and ops
pointer that you and I observed.

It's just that we changed the behavior so that we drop the ops pointer and are missing places where that pointer is used.

I wouldn't really call that ordering issues, but rather something we have overlooked.

quoted

The problem here is not about the state of the fence, but if the
callbacks are still executed or not. And that is not a good task for
a lock, instead what is usually used for that is a barrier and that
is exactly what RCU provides.

Locks are for mutual exclusion. Which is what you want here: you want
to exclude someone from signaling the fence while you are working on
it.

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.

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.

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.

So even if it is possible to implement the backend with those locks being held I will keep pushing hard for removing that.

Regards,
Christian.