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