dma_fence: force users to take the lock manually

dma_fence: force users to take the lock manually

[Philipp Stanner]

Yo Christian,

a while ago we were discussing this problem

dma_fence_set_error(f, -ECANCELED);
dma_fence_signal(f); // racy!


I think you mentioned that you are considering to redesign the
dma_fence API so that users have to take the lock themselves to touch
the fence:

dma_fence_lock(f);
dma_fence_set_error(f, -ECANCELED);
dma_fence_signal(f);
dme_fence_unlock(f);


Is that still up to date? Is there work in progress about that?

I discovered that I might need / want that for the Rust abstractions.


Regards,
P.

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/5/26 14:54, Philipp Stanner wrote:
> Yo Christian,
> 
> a while ago we were discussing this problem
> 
> dma_fence_set_error(f, -ECANCELED);
> dma_fence_signal(f); // racy!
> 
> 
> I think you mentioned that you are considering to redesign the
> dma_fence API so that users have to take the lock themselves to touch
> the fence:
> 
> dma_fence_lock(f);
> dma_fence_set_error(f, -ECANCELED);
> dma_fence_signal(f);
> dme_fence_unlock(f);
> 
> 
> Is that still up to date? Is there work in progress about that?

It's on my "maybe if I ever have time for that" list, but yeah I think it would be really nice to have and a great cleanup.

We have a bunch of different functions which provide both a _locked() and _unlocked() variant just because callers where to lazy to lock the fence.

Especially the dma_fence_signal function is overloaded 4 (!) times with locked/unlocked and with and without timestamp functions.

> I discovered that I might need / want that for the Rust abstractions.

Well my educated guess is for Rust you only want the locked function and never allow callers to be lazy.

Regards,
Christian.

> 
> 
> Regards,
> P.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

Hi,

On Thu, 5 Mar 2026 14:59:02 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/5/26 14:54, Philipp Stanner wrote:
> > Yo Christian,
> > 
> > a while ago we were discussing this problem
> > 
> > dma_fence_set_error(f, -ECANCELED);

If you really have two concurrent threads setting the error, this part
is racy, though I can't think of any situation where concurrent
signaling of a set of fences wouldn't be protected by another external
lock.

> > dma_fence_signal(f); // racy!

This is not racy because dma_fence_signal() takes/releases the
lock internally. Besides, calling dma_fence_signal() on an already
signaled fence is considered an invalid pattern if I trust the -EINVAL
returned here[1].

> > 
> > 
> > I think you mentioned that you are considering to redesign the
> > dma_fence API so that users have to take the lock themselves to touch
> > the fence:
> > 
> > dma_fence_lock(f);
> > dma_fence_set_error(f, -ECANCELED);
> > dma_fence_signal(f);

I guess you mean dma_fence_signal_locked().

> > dme_fence_unlock(f);
> > 
> > 
> > Is that still up to date? Is there work in progress about that?  
> 
> It's on my "maybe if I ever have time for that" list, but yeah I think it would be really nice to have and a great cleanup.
> 
> We have a bunch of different functions which provide both a _locked() and _unlocked() variant just because callers where to lazy to lock the fence.
> 
> Especially the dma_fence_signal function is overloaded 4 (!) times with locked/unlocked and with and without timestamp functions.
> 
> > I discovered that I might need / want that for the Rust abstractions.  
> 
> Well my educated guess is for Rust you only want the locked function and never allow callers to be lazy.

I don't think we have an immediate need for manual locking in rust
drivers (no signaling done under an already dma_fence-locked section
that I can think of), especially after the inline_lock you've
introduced. Now, I don't think it matters if only the _locked() variant
is exposed and the rust code is expected to acquire/release the lock
manually, all I'm saying is that we probably don't need that in drivers
(might be different if we start implementing fence containers like
arrays and chain in rust, but I don't think we have an immediate need
for that).

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v6.19.3/source/drivers/dma-buf/dma-fence.c#L375

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/5/26 16:12, Boris Brezillon wrote:
> Hi,
> 
> On Thu, 5 Mar 2026 14:59:02 +0100
> Christian König <christian.koenig@amd.com> wrote:
> 
>> On 3/5/26 14:54, Philipp Stanner wrote:
>>> Yo Christian,
>>>
>>> a while ago we were discussing this problem
>>>
>>> dma_fence_set_error(f, -ECANCELED);
> 
> If you really have two concurrent threads setting the error, this part
> is racy, though I can't think of any situation where concurrent
> signaling of a set of fences wouldn't be protected by another external
> lock.

This is actually massively problematic and the reason why we have the WARN_ON in dma_fence_set_error().

What drivers usually do is to disable the normal signaling path, e.g. turn off interrupts for example, and then set and error and signal the fence manually.

The problem is that this has a *huge* potential for being racy, for example when you tell the HW to not give you an interrupt any more it can always been than interrupt processing has already started but wasn't able yet to grab a lock or similar.

I think we should start enforcing correct handling and have a lockdep check in dma_fence_set_error() that the dma_fence lock is hold while calling it.

>>> dma_fence_signal(f); // racy!
> 
> This is not racy because dma_fence_signal() takes/releases the
> lock internally. Besides, calling dma_fence_signal() on an already
> signaled fence is considered an invalid pattern if I trust the -EINVAL
> returned here[1].

No, that is also something we want to remove. IIRC Philip proposed some patches to clean that up already.

>>>
>>>
>>> I think you mentioned that you are considering to redesign the
>>> dma_fence API so that users have to take the lock themselves to touch
>>> the fence:
>>>
>>> dma_fence_lock(f);
>>> dma_fence_set_error(f, -ECANCELED);
>>> dma_fence_signal(f);
> 
> I guess you mean dma_fence_signal_locked().
> 
>>> dme_fence_unlock(f);
>>>
>>>
>>> Is that still up to date? Is there work in progress about that?  
>>
>> It's on my "maybe if I ever have time for that" list, but yeah I think it would be really nice to have and a great cleanup.
>>
>> We have a bunch of different functions which provide both a _locked() and _unlocked() variant just because callers where to lazy to lock the fence.
>>
>> Especially the dma_fence_signal function is overloaded 4 (!) times with locked/unlocked and with and without timestamp functions.
>>
>>> I discovered that I might need / want that for the Rust abstractions.  
>>
>> Well my educated guess is for Rust you only want the locked function and never allow callers to be lazy.
> 
> I don't think we have an immediate need for manual locking in rust
> drivers (no signaling done under an already dma_fence-locked section
> that I can think of), especially after the inline_lock you've
> introduced. Now, I don't think it matters if only the _locked() variant
> is exposed and the rust code is expected to acquire/release the lock
> manually, all I'm saying is that we probably don't need that in drivers
> (might be different if we start implementing fence containers like
> arrays and chain in rust, but I don't think we have an immediate need
> for that).

Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.

Regards,
Christian.

> 
> Regards,
> 
> Boris
> 
> [1]https://elixir.bootlin.com/linux/v6.19.3/source/drivers/dma-buf/dma-fence.c#L375

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 6 Mar 2026 09:10:52 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/5/26 16:12, Boris Brezillon wrote:
> > Hi,
> > 
> > On Thu, 5 Mar 2026 14:59:02 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> >   
> >> On 3/5/26 14:54, Philipp Stanner wrote:  
> >>> Yo Christian,
> >>>
> >>> a while ago we were discussing this problem
> >>>
> >>> dma_fence_set_error(f, -ECANCELED);  
> > 
> > If you really have two concurrent threads setting the error, this part
> > is racy, though I can't think of any situation where concurrent
> > signaling of a set of fences wouldn't be protected by another external
> > lock.  
> 
> This is actually massively problematic and the reason why we have the WARN_ON in dma_fence_set_error().
> 
> What drivers usually do is to disable the normal signaling path, e.g. turn off interrupts for example, and then set and error and signal the fence manually.
> 
> The problem is that this has a *huge* potential for being racy, for example when you tell the HW to not give you an interrupt any more it can always been than interrupt processing has already started but wasn't able yet to grab a lock or similar.
> 
> I think we should start enforcing correct handling and have a lockdep check in dma_fence_set_error() that the dma_fence lock is hold while calling it.

Sure, I don't mind you dropping the non-locked variants and forcing
users to lock around set_error() + signal().

> 
> >>> dma_fence_signal(f); // racy!  
> > 
> > This is not racy because dma_fence_signal() takes/releases the
> > lock internally. Besides, calling dma_fence_signal() on an already
> > signaled fence is considered an invalid pattern if I trust the -EINVAL
> > returned here[1].  
> 
> No, that is also something we want to remove. IIRC Philip proposed some patches to clean that up already.

What do you mean? You want dma_fence_signal_locked() (or the variants
of it) to not return an error when the fence is already signaled, or
you want to prevent this double-signal from happening. The plan for the
rust abstraction is to do the latter.

> 
> >>>
> >>>
> >>> I think you mentioned that you are considering to redesign the
> >>> dma_fence API so that users have to take the lock themselves to touch
> >>> the fence:
> >>>
> >>> dma_fence_lock(f);
> >>> dma_fence_set_error(f, -ECANCELED);
> >>> dma_fence_signal(f);  
> > 
> > I guess you mean dma_fence_signal_locked().
> >   
> >>> dme_fence_unlock(f);
> >>>
> >>>
> >>> Is that still up to date? Is there work in progress about that?    
> >>
> >> It's on my "maybe if I ever have time for that" list, but yeah I think it would be really nice to have and a great cleanup.
> >>
> >> We have a bunch of different functions which provide both a _locked() and _unlocked() variant just because callers where to lazy to lock the fence.
> >>
> >> Especially the dma_fence_signal function is overloaded 4 (!) times with locked/unlocked and with and without timestamp functions.
> >>  
> >>> I discovered that I might need / want that for the Rust abstractions.    
> >>
> >> Well my educated guess is for Rust you only want the locked function and never allow callers to be lazy.  
> > 
> > I don't think we have an immediate need for manual locking in rust
> > drivers (no signaling done under an already dma_fence-locked section
> > that I can think of), especially after the inline_lock you've
> > introduced. Now, I don't think it matters if only the _locked() variant
> > is exposed and the rust code is expected to acquire/release the lock
> > manually, all I'm saying is that we probably don't need that in drivers
> > (might be different if we start implementing fence containers like
> > arrays and chain in rust, but I don't think we have an immediate need
> > for that).  
> 
> Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.

Not really. With rust's ownership model, you can make it so only one
thread gets to own the DriverFence (the signal-able fence object), and
the DriverFence::signal() method consumes this object. This implies
that only one path gets to signal the DriverFence, and after that it
vanishes, so no one else can signal it anymore. Just to clarify, by
vanishes, I mean that the signal-able view disappears, but the
observable object (Fence) can stay around, so it can be monitored (and
only monitored) by others. With this model, it doesn't matter that
_set_error() is set under a dma_fence locked section or not, because
the concurrency is addressed at a higher level.

Again, I'm not saying the changes Christian and you have been
discussing are pointless (they might help with the C implementations to
get things right), I'm just saying it's not strictly needed for the rust
abstraction, that's all.

Re: dma_fence: force users to take the lock manually

[Philipp Stanner]

On Fri, 2026-03-06 at 10:46 +0100, Boris Brezillon wrote:
> On Fri, 6 Mar 2026 09:10:52 +0100
> Christian König <christian.koenig@amd.com> wrote:
> 
> > On 3/5/26 16:12, Boris Brezillon wrote:
> > > Hi,
> > > 
> > > On Thu, 5 Mar 2026 14:59:02 +0100
> > > Christian König <christian.koenig@amd.com> wrote:
> > >   
> > > > On 3/5/26 14:54, Philipp Stanner wrote:  
> > > > > Yo Christian,
> > > > > 
> > > > > a while ago we were discussing this problem
> > > > > 
> > > > > dma_fence_set_error(f, -ECANCELED);  
> > > 
> > > If you really have two concurrent threads setting the error, this part
> > > is racy, though I can't think of any situation where concurrent
> > > signaling of a set of fences wouldn't be protected by another external
> > > lock.  
> > 
> > This is actually massively problematic and the reason why we have the WARN_ON in dma_fence_set_error().
> > 
> > What drivers usually do is to disable the normal signaling path, e.g. turn off interrupts for example, and then set and error and signal the fence manually.
> > 
> > The problem is that this has a *huge* potential for being racy, for example when you tell the HW to not give you an interrupt any more it can always been than interrupt processing has already started but wasn't able yet to grab a lock or similar.
> > 
> > I think we should start enforcing correct handling and have a lockdep check in dma_fence_set_error() that the dma_fence lock is hold while calling it.
> 
> Sure, I don't mind you dropping the non-locked variants and forcing
> users to lock around set_error() + signal().
> 
> > 
> > > > > dma_fence_signal(f); // racy!  
> > > 
> > > This is not racy because dma_fence_signal() takes/releases the
> > > lock internally. Besides, calling dma_fence_signal() on an already
> > > signaled fence is considered an invalid pattern if I trust the -EINVAL
> > > returned here[1].  
> > 
> > No, that is also something we want to remove. IIRC Philip proposed some patches to clean that up already.
> 
> What do you mean? You want dma_fence_signal_locked() (or the variants
> of it) to not return an error when the fence is already signaled,
> 

Yes. That's already implemented:

https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/dma-buf/dma-fence.c#L486


Reason being that
a) no one was ever checking that error code
b) you cannot *prevent* multiple signaling in C anyways
c) it's not even sure AFAICT whether signaling an already signaled
fence is even an error. The function will simply ignore the action.
It's not ideal design, sure, but what's the harm? The most important
fence rule is that fences do get eventually signaled. Firing WARN_ONs
or sth because you try to signal a signaled fence sounds bad to me,
because what's the issue?

>  or
> you want to prevent this double-signal from happening. The plan for the
> rust abstraction is to do the latter.

In Rust we sort of get that for free by having signal() consume the
fence.


> 
> > 
> > > > > 
> > > > > 
> > > > > I think you mentioned that you are considering to redesign the
> > > > > dma_fence API so that users have to take the lock themselves to touch
> > > > > the fence:
> > > > > 
> > > > > dma_fence_lock(f);
> > > > > dma_fence_set_error(f, -ECANCELED);
> > > > > dma_fence_signal(f);  
> > > 
> > > I guess you mean dma_fence_signal_locked().
> > >   
> > > > > dme_fence_unlock(f);
> > > > > 
> > > > > 
> > > > > Is that still up to date? Is there work in progress about that?    
> > > > 
> > > > It's on my "maybe if I ever have time for that" list, but yeah I think it would be really nice to have and a great cleanup.
> > > > 
> > > > We have a bunch of different functions which provide both a _locked() and _unlocked() variant just because callers where to lazy to lock the fence.
> > > > 
> > > > Especially the dma_fence_signal function is overloaded 4 (!) times with locked/unlocked and with and without timestamp functions.
> > > >  
> > > > > I discovered that I might need / want that for the Rust abstractions.    
> > > > 
> > > > Well my educated guess is for Rust you only want the locked function and never allow callers to be lazy.  
> > > 
> > > I don't think we have an immediate need for manual locking in rust
> > > drivers (no signaling done under an already dma_fence-locked section
> > > that I can think of), especially after the inline_lock you've
> > > introduced. Now, I don't think it matters if only the _locked() variant
> > > is exposed and the rust code is expected to acquire/release the lock
> > > manually, all I'm saying is that we probably don't need that in drivers
> > > (might be different if we start implementing fence containers like
> > > arrays and chain in rust, but I don't think we have an immediate need
> > > for that).  
> > 
> > Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.
> 
> Not really. With rust's ownership model, you can make it so only one
> thread gets to own the DriverFence (the signal-able fence object),
> 

Not strictly speaking. They can always stuff it into some locked
refcounted container.

>  and
> the DriverFence::signal() method consumes this object. This implies
> that only one path gets to signal the DriverFence, and after that it
> vanishes, so no one else can signal it anymore. Just to clarify, by
> vanishes, I mean that the signal-able view disappears, but the
> observable object (Fence) can stay around, so it can be monitored (and
> only monitored) by others. With this model, it doesn't matter that
> _set_error() is set under a dma_fence locked section or not, because
> the concurrency is addressed at a higher level.
> 
> Again, I'm not saying the changes Christian and you have been
> discussing are pointless (they might help with the C implementations to
> get things right), I'm just saying it's not strictly needed for the rust
> abstraction, that's all.


Sounds correct to me.


P.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 06 Mar 2026 10:54:07 +0100
Philipp Stanner <phasta@mailbox.org> wrote:

> On Fri, 2026-03-06 at 10:46 +0100, Boris Brezillon wrote:
> > On Fri, 6 Mar 2026 09:10:52 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> >   
> > > On 3/5/26 16:12, Boris Brezillon wrote:  
> > > > Hi,
> > > > 
> > > > On Thu, 5 Mar 2026 14:59:02 +0100
> > > > Christian König <christian.koenig@amd.com> wrote:
> > > >     
> > > > > On 3/5/26 14:54, Philipp Stanner wrote:    
> > > > > > Yo Christian,
> > > > > > 
> > > > > > a while ago we were discussing this problem
> > > > > > 
> > > > > > dma_fence_set_error(f, -ECANCELED);    
> > > > 
> > > > If you really have two concurrent threads setting the error, this part
> > > > is racy, though I can't think of any situation where concurrent
> > > > signaling of a set of fences wouldn't be protected by another external
> > > > lock.    
> > > 
> > > This is actually massively problematic and the reason why we have the WARN_ON in dma_fence_set_error().
> > > 
> > > What drivers usually do is to disable the normal signaling path, e.g. turn off interrupts for example, and then set and error and signal the fence manually.
> > > 
> > > The problem is that this has a *huge* potential for being racy, for example when you tell the HW to not give you an interrupt any more it can always been than interrupt processing has already started but wasn't able yet to grab a lock or similar.
> > > 
> > > I think we should start enforcing correct handling and have a lockdep check in dma_fence_set_error() that the dma_fence lock is hold while calling it.  
> > 
> > Sure, I don't mind you dropping the non-locked variants and forcing
> > users to lock around set_error() + signal().
> >   
> > >   
> > > > > > dma_fence_signal(f); // racy!    
> > > > 
> > > > This is not racy because dma_fence_signal() takes/releases the
> > > > lock internally. Besides, calling dma_fence_signal() on an already
> > > > signaled fence is considered an invalid pattern if I trust the -EINVAL
> > > > returned here[1].    
> > > 
> > > No, that is also something we want to remove. IIRC Philip proposed some patches to clean that up already.  
> > 
> > What do you mean? You want dma_fence_signal_locked() (or the variants
> > of it) to not return an error when the fence is already signaled,
> >   
> 
> Yes. That's already implemented:
> 
> https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/dma-buf/dma-fence.c#L486

Okay, I guess I was looking at an older version of the code. My bad.

> 
> 
> Reason being that
> a) no one was ever checking that error code
> b) you cannot *prevent* multiple signaling in C anyways
> c) it's not even sure AFAICT whether signaling an already signaled
> fence is even an error. The function will simply ignore the action.
> It's not ideal design, sure, but what's the harm? The most important
> fence rule is that fences do get eventually signaled. Firing WARN_ONs
> or sth because you try to signal a signaled fence sounds bad to me,
> because what's the issue?

Fair enough. Not really questioning those changes to be honest, I'm
just here to point that the rust abstraction will hopefully be immune
to the stuff you're trying to protect against.

> 
> >  or
> > you want to prevent this double-signal from happening. The plan for the
> > rust abstraction is to do the latter.  
> 
> In Rust we sort of get that for free by having signal() consume the
> fence.

Exactly.

> 
> 
> >   
> > >   
> > > > > > 
> > > > > > 
> > > > > > I think you mentioned that you are considering to redesign the
> > > > > > dma_fence API so that users have to take the lock themselves to touch
> > > > > > the fence:
> > > > > > 
> > > > > > dma_fence_lock(f);
> > > > > > dma_fence_set_error(f, -ECANCELED);
> > > > > > dma_fence_signal(f);    
> > > > 
> > > > I guess you mean dma_fence_signal_locked().
> > > >     
> > > > > > dme_fence_unlock(f);
> > > > > > 
> > > > > > 
> > > > > > Is that still up to date? Is there work in progress about that?      
> > > > > 
> > > > > It's on my "maybe if I ever have time for that" list, but yeah I think it would be really nice to have and a great cleanup.
> > > > > 
> > > > > We have a bunch of different functions which provide both a _locked() and _unlocked() variant just because callers where to lazy to lock the fence.
> > > > > 
> > > > > Especially the dma_fence_signal function is overloaded 4 (!) times with locked/unlocked and with and without timestamp functions.
> > > > >    
> > > > > > I discovered that I might need / want that for the Rust abstractions.      
> > > > > 
> > > > > Well my educated guess is for Rust you only want the locked function and never allow callers to be lazy.    
> > > > 
> > > > I don't think we have an immediate need for manual locking in rust
> > > > drivers (no signaling done under an already dma_fence-locked section
> > > > that I can think of), especially after the inline_lock you've
> > > > introduced. Now, I don't think it matters if only the _locked() variant
> > > > is exposed and the rust code is expected to acquire/release the lock
> > > > manually, all I'm saying is that we probably don't need that in drivers
> > > > (might be different if we start implementing fence containers like
> > > > arrays and chain in rust, but I don't think we have an immediate need
> > > > for that).    
> > > 
> > > Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.  
> > 
> > Not really. With rust's ownership model, you can make it so only one
> > thread gets to own the DriverFence (the signal-able fence object),
> >   
> 
> Not strictly speaking. They can always stuff it into some locked
> refcounted container.

And that's my point: you've protected the container with some lock, and
if the DriverFence is signaled under that lock, it goes away, meaning
the other thread walking that very some container later on won't see it
anymore. So yes, there are ways you can move DriverFence between
threads (`Send` trait in rust), but there's no way rust will let you
signal DriverFence objects concurrently (either you wrap it in a Lock
that serializes accesses, or it will just refuse to compile).

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 10:46, Boris Brezillon wrote:
> On Fri, 6 Mar 2026 09:10:52 +0100
> Christian König <christian.koenig@amd.com> wrote:
>> Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.
> 
> Not really. With rust's ownership model, you can make it so only one
> thread gets to own the DriverFence (the signal-able fence object), and
> the DriverFence::signal() method consumes this object. This implies
> that only one path gets to signal the DriverFence, and after that it
> vanishes, so no one else can signal it anymore. Just to clarify, by
> vanishes, I mean that the signal-able view disappears, but the
> observable object (Fence) can stay around, so it can be monitored (and
> only monitored) by others. With this model, it doesn't matter that
> _set_error() is set under a dma_fence locked section or not, because
> the concurrency is addressed at a higher level.

That whole approach won't work. You have at least the IRQ handler which signals completion and the timeout handler which signals completion with an error.

We have documented that this handling is mandatory for DMA-fences since so many driver implementations got it wrong.

That's why Philips patch set removed the return value from dma_fence_signal().

Regards,
Christian.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 6 Mar 2026 10:58:07 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/6/26 10:46, Boris Brezillon wrote:
> > On Fri, 6 Mar 2026 09:10:52 +0100
> > Christian König <christian.koenig@amd.com> wrote:  
> >> Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.  
> > 
> > Not really. With rust's ownership model, you can make it so only one
> > thread gets to own the DriverFence (the signal-able fence object), and
> > the DriverFence::signal() method consumes this object. This implies
> > that only one path gets to signal the DriverFence, and after that it
> > vanishes, so no one else can signal it anymore. Just to clarify, by
> > vanishes, I mean that the signal-able view disappears, but the
> > observable object (Fence) can stay around, so it can be monitored (and
> > only monitored) by others. With this model, it doesn't matter that
> > _set_error() is set under a dma_fence locked section or not, because
> > the concurrency is addressed at a higher level.  
> 
> That whole approach won't work. You have at least the IRQ handler which signals completion and the timeout handler which signals completion with an error.

From a pure rust standpoint, and assuming both path (IRQ handler and
timeout handler) are written in rust, the compiler won't let you signal
concurrently if we design the thing properly, that's what I'm trying to
say. Just to be clear, it doesn't mean you can't have one worker (in a
workqueue context) that can signal a fence and an IRQ handler that can
signal the same fence. It just means that rust won't let you do that
unless you have proper locking in place, and rust will also guarantee
you won't be able to signal a fence that has already been signaled,
because as soon as it's signaled, the signal-able fence should be
consumed.


> 
> We have documented that this handling is mandatory for DMA-fences since so many driver implementations got it wrong.

Again, I'm just talking about the rust implementation we're aiming for.
If you start mixing C and rust in the same driver, you're back to the
original problem you described.

> 
> That's why Philips patch set removed the return value from dma_fence_signal().

I don't mind that. Just saying that's hopefully a non-issue in the rust
abstraction.

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 11:37, Boris Brezillon wrote:
> On Fri, 6 Mar 2026 10:58:07 +0100
> Christian König <christian.koenig@amd.com> wrote:
> 
>> On 3/6/26 10:46, Boris Brezillon wrote:
>>> On Fri, 6 Mar 2026 09:10:52 +0100
>>> Christian König <christian.koenig@amd.com> wrote:  
>>>> Well as I wrote above you either have super reliable locking in your signaling path or you will need that for error handling.  
>>>
>>> Not really. With rust's ownership model, you can make it so only one
>>> thread gets to own the DriverFence (the signal-able fence object), and
>>> the DriverFence::signal() method consumes this object. This implies
>>> that only one path gets to signal the DriverFence, and after that it
>>> vanishes, so no one else can signal it anymore. Just to clarify, by
>>> vanishes, I mean that the signal-able view disappears, but the
>>> observable object (Fence) can stay around, so it can be monitored (and
>>> only monitored) by others. With this model, it doesn't matter that
>>> _set_error() is set under a dma_fence locked section or not, because
>>> the concurrency is addressed at a higher level.  
>>
>> That whole approach won't work. You have at least the IRQ handler which signals completion and the timeout handler which signals completion with an error.
> 
> From a pure rust standpoint, and assuming both path (IRQ handler and
> timeout handler) are written in rust, the compiler won't let you signal
> concurrently if we design the thing properly, that's what I'm trying to
> say. Just to be clear, it doesn't mean you can't have one worker (in a
> workqueue context) that can signal a fence and an IRQ handler that can
> signal the same fence. It just means that rust won't let you do that
> unless you have proper locking in place, and rust will also guarantee
> you won't be able to signal a fence that has already been signaled,
> because as soon as it's signaled, the signal-able fence should be
> consumed.

Ah got it! I've worked a lot with OCaml in the past which has some similarities, but doesn't push things that far.

>>
>> We have documented that this handling is mandatory for DMA-fences since so many driver implementations got it wrong.
> 
> Again, I'm just talking about the rust implementation we're aiming for.
> If you start mixing C and rust in the same driver, you're back to the
> original problem you described.

The key point is the Rust implementation should not repeat the mistakes we made in the C implementation.

For example blocking that multiple threads can't signal a DMA-fence is completely irrelevant.

What we need to guarantee is correct timeout handling and that DMA-fence can only signal from something delivered from a HW event, e.g. a HW interrupt or interrupt worker or similar.

A DMA-fence should *never* signal because of an IOCTL or because some object runs out of scope. E.g. when you cleanup a HW ring buffer, FW queue, etc...

Regards,
Christian.

> 
>>
>> That's why Philips patch set removed the return value from dma_fence_signal().
> 
> I don't mind that. Just saying that's hopefully a non-issue in the rust
> abstraction.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 6 Mar 2026 12:03:19 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/6/26 11:37, Boris Brezillon wrote:
> > On Fri, 6 Mar 2026 10:58:07 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> >   
> >> On 3/6/26 10:46, Boris Brezillon wrote:  
> >>> On Fri, 6 Mar 2026 09:10:52 +0100
> >>> Christian König <christian.koenig@amd.com> wrote:    
> >>>> Well as I wrote above you either have super reliable locking in
> >>>> your signaling path or you will need that for error handling.    
> >>>
> >>> Not really. With rust's ownership model, you can make it so only
> >>> one thread gets to own the DriverFence (the signal-able fence
> >>> object), and the DriverFence::signal() method consumes this
> >>> object. This implies that only one path gets to signal the
> >>> DriverFence, and after that it vanishes, so no one else can
> >>> signal it anymore. Just to clarify, by vanishes, I mean that the
> >>> signal-able view disappears, but the observable object (Fence)
> >>> can stay around, so it can be monitored (and only monitored) by
> >>> others. With this model, it doesn't matter that _set_error() is
> >>> set under a dma_fence locked section or not, because the
> >>> concurrency is addressed at a higher level.    
> >>
> >> That whole approach won't work. You have at least the IRQ handler
> >> which signals completion and the timeout handler which signals
> >> completion with an error.  
> > 
> > From a pure rust standpoint, and assuming both path (IRQ handler and
> > timeout handler) are written in rust, the compiler won't let you
> > signal concurrently if we design the thing properly, that's what
> > I'm trying to say. Just to be clear, it doesn't mean you can't have
> > one worker (in a workqueue context) that can signal a fence and an
> > IRQ handler that can signal the same fence. It just means that rust
> > won't let you do that unless you have proper locking in place, and
> > rust will also guarantee you won't be able to signal a fence that
> > has already been signaled, because as soon as it's signaled, the
> > signal-able fence should be consumed.  
> 
> Ah got it! I've worked a lot with OCaml in the past which has some
> similarities, but doesn't push things that far.
> 
> >>
> >> We have documented that this handling is mandatory for DMA-fences
> >> since so many driver implementations got it wrong.  
> > 
> > Again, I'm just talking about the rust implementation we're aiming
> > for. If you start mixing C and rust in the same driver, you're back
> > to the original problem you described.  
> 
> The key point is the Rust implementation should not repeat the
> mistakes we made in the C implementation.
> 
> For example blocking that multiple threads can't signal a DMA-fence
> is completely irrelevant.

From a correctness standpoint, I think it's important to ensure no more
than one thread gets to signal the object.

> 
> What we need to guarantee is correct timeout handling and that
> DMA-fence can only signal from something delivered from a HW event,
> e.g. a HW interrupt or interrupt worker or similar.

We've mostly focused on coming up with a solution that would annotate
signaling paths in an automated way, and making sure dma_fence_signal()
is never called outside of a non-annotated path:
- creation of DmaFenceWorkqueue/DmaFence[Delayed]Work that guarantees
  all works are executed in a dma_fence_signalling_{begin,end}()
  section, so we can properly detect deadlocks (through lockdep)
- creation of a DmaFenceIrqHandler for the same reason
- we'll need variants for each new deferred mechanism drivers might
  want to use (kthread_worker?)

But there's currently no restriction on calling dma_fence_signal() in a
user thread context (IOCTL()). I guess that shouldn't be too hard to
add (is_user_task() to the rescue).

> 
> A DMA-fence should *never* signal because of an IOCTL

Okay, that's understandable.

> or because some
> object runs out of scope. E.g. when you cleanup a HW ring buffer, FW
> queue, etc...

We were actually going in the opposite direction:
auto-signal(ECANCELED) on DriverFenceTimeline object destruction (which
is the thing that would be attached to the HW ringbuf. The reason is:
we don't want to leave unsignalled fences behind, and if the HW ring is
gone, there's nothing that can signal it. Mind explaining why you think
this shouldn't be done, because I originally interpreted your
suggestion as exactly the opposite.

Re: dma_fence: force users to take the lock manually

[Philipp Stanner]

On Fri, 2026-03-06 at 12:24 +0100, Boris Brezillon wrote:
> On Fri, 6 Mar 2026 12:03:19 +0100
> Christian König <christian.koenig@amd.com> wrote:
> 
> > On 3/6/26 11:37, Boris Brezillon wrote:
> > > On Fri, 6 Mar 2026 10:58:07 +0100
> > > Christian König <christian.koenig@amd.com> wrote:
> > >   
> > > > On 3/6/26 10:46, Boris Brezillon wrote:  
> > > > > On Fri, 6 Mar 2026 09:10:52 +0100
> > > > > Christian König <christian.koenig@amd.com> wrote:    
> > > > > > Well as I wrote above you either have super reliable locking in
> > > > > > your signaling path or you will need that for error handling.    
> > > > > 
> > > > > Not really. With rust's ownership model, you can make it so only
> > > > > one thread gets to own the DriverFence (the signal-able fence
> > > > > object), and the DriverFence::signal() method consumes this
> > > > > object. This implies that only one path gets to signal the
> > > > > DriverFence, and after that it vanishes, so no one else can
> > > > > signal it anymore. Just to clarify, by vanishes, I mean that the
> > > > > signal-able view disappears, but the observable object (Fence)
> > > > > can stay around, so it can be monitored (and only monitored) by
> > > > > others. With this model, it doesn't matter that _set_error() is
> > > > > set under a dma_fence locked section or not, because the
> > > > > concurrency is addressed at a higher level.    
> > > > 
> > > > That whole approach won't work. You have at least the IRQ handler
> > > > which signals completion and the timeout handler which signals
> > > > completion with an error.  
> > > 
> > > From a pure rust standpoint, and assuming both path (IRQ handler and
> > > timeout handler) are written in rust, the compiler won't let you
> > > signal concurrently if we design the thing properly, that's what
> > > I'm trying to say. Just to be clear, it doesn't mean you can't have
> > > one worker (in a workqueue context) that can signal a fence and an
> > > IRQ handler that can signal the same fence. It just means that rust
> > > won't let you do that unless you have proper locking in place, and
> > > rust will also guarantee you won't be able to signal a fence that
> > > has already been signaled, because as soon as it's signaled, the
> > > signal-able fence should be consumed.  
> > 
> > Ah got it! I've worked a lot with OCaml in the past which has some
> > similarities, but doesn't push things that far.
> > 
> > > > 
> > > > We have documented that this handling is mandatory for DMA-fences
> > > > since so many driver implementations got it wrong.  
> > > 
> > > Again, I'm just talking about the rust implementation we're aiming
> > > for. If you start mixing C and rust in the same driver, you're back
> > > to the original problem you described.  
> > 
> > The key point is the Rust implementation should not repeat the
> > mistakes we made in the C implementation.
> > 
> > For example blocking that multiple threads can't signal a DMA-fence
> > is completely irrelevant.
> 
> From a correctness standpoint, I think it's important to ensure no more
> than one thread gets to signal the object.

If you have two paths that can signal a fence, that will result
effectively in you in Rust having to use yet another lock for a fence,
and likely some mechanism for revoking the access.

I would at least consider whether it isn't much easier to have the
signalling-function ignore multiple signal attempts.

AFAIU in Rust we originaly ended up at signal() consuming the fence
because of the code UAF problem with data: T.

> 
> > 
> > What we need to guarantee is correct timeout handling and that
> > DMA-fence can only signal from something delivered from a HW event,
> > e.g. a HW interrupt or interrupt worker or similar.
> 
> We've mostly focused on coming up with a solution that would annotate
> signaling paths in an automated way, and making sure dma_fence_signal()
> is never called outside of a non-annotated path:
> - creation of DmaFenceWorkqueue/DmaFence[Delayed]Work that guarantees
>   all works are executed in a dma_fence_signalling_{begin,end}()
>   section, so we can properly detect deadlocks (through lockdep)
> - creation of a DmaFenceIrqHandler for the same reason
> - we'll need variants for each new deferred mechanism drivers might
>   want to use (kthread_worker?)
> 
> But there's currently no restriction on calling dma_fence_signal() in a
> user thread context (IOCTL()). I guess that shouldn't be too hard to
> add (is_user_task() to the rescue).
> 
> > 
> > A DMA-fence should *never* signal because of an IOCTL
> 
> Okay, that's understandable.
> 
> > or because some
> > object runs out of scope. E.g. when you cleanup a HW ring buffer, FW
> > queue, etc...
> 
> We were actually going in the opposite direction:
> auto-signal(ECANCELED) on DriverFenceTimeline object destruction (which
> is the thing that would be attached to the HW ringbuf. The reason is:
> we don't want to leave unsignalled fences behind,
> 

Not only do we not "want to", we actually *cannot*. We have to make
sure all fences are signaled because only this way the C backend plus
RCU can protect also the Rust code against UAF.

>  and if the HW ring is
> gone, there's nothing that can signal it. Mind explaining why you think
> this shouldn't be done, because I originally interpreted your
> suggestion as exactly the opposite.

I also don't get it. All fences must always get signaled, that's one of
the most fundamental fence rules. Thus, if the last accessor to a fence
drops, you do want to signal it with -ECANCELED


P.

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 12:57, Philipp Stanner wrote:
> On Fri, 2026-03-06 at 12:24 +0100, Boris Brezillon wrote:
>> On Fri, 6 Mar 2026 12:03:19 +0100
>> Christian König <christian.koenig@amd.com> wrote:
>>
>>> On 3/6/26 11:37, Boris Brezillon wrote:
>>>> On Fri, 6 Mar 2026 10:58:07 +0100
>>>> Christian König <christian.koenig@amd.com> wrote:
>>>>   
>>>>> On 3/6/26 10:46, Boris Brezillon wrote:  
>>>>>> On Fri, 6 Mar 2026 09:10:52 +0100
>>>>>> Christian König <christian.koenig@amd.com> wrote:    
>>>>>>> Well as I wrote above you either have super reliable locking in
>>>>>>> your signaling path or you will need that for error handling.    
>>>>>>
>>>>>> Not really. With rust's ownership model, you can make it so only
>>>>>> one thread gets to own the DriverFence (the signal-able fence
>>>>>> object), and the DriverFence::signal() method consumes this
>>>>>> object. This implies that only one path gets to signal the
>>>>>> DriverFence, and after that it vanishes, so no one else can
>>>>>> signal it anymore. Just to clarify, by vanishes, I mean that the
>>>>>> signal-able view disappears, but the observable object (Fence)
>>>>>> can stay around, so it can be monitored (and only monitored) by
>>>>>> others. With this model, it doesn't matter that _set_error() is
>>>>>> set under a dma_fence locked section or not, because the
>>>>>> concurrency is addressed at a higher level.    
>>>>>
>>>>> That whole approach won't work. You have at least the IRQ handler
>>>>> which signals completion and the timeout handler which signals
>>>>> completion with an error.  
>>>>
>>>> From a pure rust standpoint, and assuming both path (IRQ handler and
>>>> timeout handler) are written in rust, the compiler won't let you
>>>> signal concurrently if we design the thing properly, that's what
>>>> I'm trying to say. Just to be clear, it doesn't mean you can't have
>>>> one worker (in a workqueue context) that can signal a fence and an
>>>> IRQ handler that can signal the same fence. It just means that rust
>>>> won't let you do that unless you have proper locking in place, and
>>>> rust will also guarantee you won't be able to signal a fence that
>>>> has already been signaled, because as soon as it's signaled, the
>>>> signal-able fence should be consumed.  
>>>
>>> Ah got it! I've worked a lot with OCaml in the past which has some
>>> similarities, but doesn't push things that far.
>>>
>>>>>
>>>>> We have documented that this handling is mandatory for DMA-fences
>>>>> since so many driver implementations got it wrong.  
>>>>
>>>> Again, I'm just talking about the rust implementation we're aiming
>>>> for. If you start mixing C and rust in the same driver, you're back
>>>> to the original problem you described.  
>>>
>>> The key point is the Rust implementation should not repeat the
>>> mistakes we made in the C implementation.
>>>
>>> For example blocking that multiple threads can't signal a DMA-fence
>>> is completely irrelevant.
>>
>> From a correctness standpoint, I think it's important to ensure no more
>> than one thread gets to signal the object.
> 
> If you have two paths that can signal a fence, that will result
> effectively in you in Rust having to use yet another lock for a fence,
> and likely some mechanism for revoking the access.
> 
> I would at least consider whether it isn't much easier to have the
> signalling-function ignore multiple signal attempts.
> 
> AFAIU in Rust we originaly ended up at signal() consuming the fence
> because of the code UAF problem with data: T.

+1

>>>
>>> What we need to guarantee is correct timeout handling and that
>>> DMA-fence can only signal from something delivered from a HW event,
>>> e.g. a HW interrupt or interrupt worker or similar.
>>
>> We've mostly focused on coming up with a solution that would annotate
>> signaling paths in an automated way, and making sure dma_fence_signal()
>> is never called outside of a non-annotated path:
>> - creation of DmaFenceWorkqueue/DmaFence[Delayed]Work that guarantees
>>   all works are executed in a dma_fence_signalling_{begin,end}()
>>   section, so we can properly detect deadlocks (through lockdep)
>> - creation of a DmaFenceIrqHandler for the same reason
>> - we'll need variants for each new deferred mechanism drivers might
>>   want to use (kthread_worker?)
>>
>> But there's currently no restriction on calling dma_fence_signal() in a
>> user thread context (IOCTL()). I guess that shouldn't be too hard to
>> add (is_user_task() to the rescue).
>>
>>>
>>> A DMA-fence should *never* signal because of an IOCTL
>>
>> Okay, that's understandable.
>>
>>> or because some
>>> object runs out of scope. E.g. when you cleanup a HW ring buffer, FW
>>> queue, etc...
>>
>> We were actually going in the opposite direction:
>> auto-signal(ECANCELED) on DriverFenceTimeline object destruction

Absolutely clear NAK to that, we have iterated that many times before on the C side as well.

See below for the explanation of the background.

>> (which
>> is the thing that would be attached to the HW ringbuf. The reason is:
>> we don't want to leave unsignalled fences behind,
>>
> 
> Not only do we not "want to", we actually *cannot*. We have to make
> sure all fences are signaled because only this way the C backend plus
> RCU can protect also the Rust code against UAF.
> 
>>  and if the HW ring is
>> gone, there's nothing that can signal it. Mind explaining why you think
>> this shouldn't be done, because I originally interpreted your
>> suggestion as exactly the opposite.
> 
> I also don't get it. All fences must always get signaled, that's one of
> the most fundamental fence rules. Thus, if the last accessor to a fence
> drops, you do want to signal it with -ECANCELED

All fences must always signal because the HW operation must always complete or be terminated by a timeout.

If a fence signals only because it runs out of scope than that means that you have a huge potential for data corruption and that is even worse than not signaling a fence.

In other words not signaling a fence can leave the system in a deadlock state, but signaling it incorrectly usually results in random data corruption.

Saying that we could potentially make dma_fence_release() more resilient to ref-counting issues.

Regards,
Christian.

> 
> 
> P.

Re: dma_fence: force users to take the lock manually

[Philipp Stanner]

On Fri, 2026-03-06 at 13:31 +0100, Christian König wrote:
> On 3/6/26 12:57, Philipp Stanner wrote:
> > On Fri, 2026-03-06 at 12:24 +0100, Boris Brezillon wrote:
> > > On Fri, 6 Mar 2026 12:03:19 +0100
> > > Christian König <christian.koenig@amd.com> wrote:
> > > 
> > > > On 3/6/26 11:37, Boris Brezillon wrote:
> > > > > On Fri, 6 Mar 2026 10:58:07 +0100
> > > > > Christian König <christian.koenig@amd.com> wrote:
> > > > >   
> > > > > > On 3/6/26 10:46, Boris Brezillon wrote:  
> > > > > > > On Fri, 6 Mar 2026 09:10:52 +0100
> > > > > > > Christian König <christian.koenig@amd.com> wrote:    
> > > > > > > > Well as I wrote above you either have super reliable locking in
> > > > > > > > your signaling path or you will need that for error handling.    
> > > > > > > 
> > > > > > > Not really. With rust's ownership model, you can make it so only
> > > > > > > one thread gets to own the DriverFence (the signal-able fence
> > > > > > > object), and the DriverFence::signal() method consumes this
> > > > > > > object. This implies that only one path gets to signal the
> > > > > > > DriverFence, and after that it vanishes, so no one else can
> > > > > > > signal it anymore. Just to clarify, by vanishes, I mean that the
> > > > > > > signal-able view disappears, but the observable object (Fence)
> > > > > > > can stay around, so it can be monitored (and only monitored) by
> > > > > > > others. With this model, it doesn't matter that _set_error() is
> > > > > > > set under a dma_fence locked section or not, because the
> > > > > > > concurrency is addressed at a higher level.    
> > > > > > 
> > > > > > That whole approach won't work. You have at least the IRQ handler
> > > > > > which signals completion and the timeout handler which signals
> > > > > > completion with an error.  
> > > > > 
> > > > > From a pure rust standpoint, and assuming both path (IRQ handler and
> > > > > timeout handler) are written in rust, the compiler won't let you
> > > > > signal concurrently if we design the thing properly, that's what
> > > > > I'm trying to say. Just to be clear, it doesn't mean you can't have
> > > > > one worker (in a workqueue context) that can signal a fence and an
> > > > > IRQ handler that can signal the same fence. It just means that rust
> > > > > won't let you do that unless you have proper locking in place, and
> > > > > rust will also guarantee you won't be able to signal a fence that
> > > > > has already been signaled, because as soon as it's signaled, the
> > > > > signal-able fence should be consumed.  
> > > > 
> > > > Ah got it! I've worked a lot with OCaml in the past which has some
> > > > similarities, but doesn't push things that far.
> > > > 
> > > > > > 
> > > > > > We have documented that this handling is mandatory for DMA-fences
> > > > > > since so many driver implementations got it wrong.  
> > > > > 
> > > > > Again, I'm just talking about the rust implementation we're aiming
> > > > > for. If you start mixing C and rust in the same driver, you're back
> > > > > to the original problem you described.  
> > > > 
> > > > The key point is the Rust implementation should not repeat the
> > > > mistakes we made in the C implementation.
> > > > 
> > > > For example blocking that multiple threads can't signal a DMA-fence
> > > > is completely irrelevant.
> > > 
> > > From a correctness standpoint, I think it's important to ensure no more
> > > than one thread gets to signal the object.
> > 
> > If you have two paths that can signal a fence, that will result
> > effectively in you in Rust having to use yet another lock for a fence,
> > and likely some mechanism for revoking the access.
> > 
> > I would at least consider whether it isn't much easier to have the
> > signalling-function ignore multiple signal attempts.
> > 
> > AFAIU in Rust we originaly ended up at signal() consuming the fence
> > because of the code UAF problem with data: T.
> 
> +1
> 
> > > > 
> > > > What we need to guarantee is correct timeout handling and that
> > > > DMA-fence can only signal from something delivered from a HW event,
> > > > e.g. a HW interrupt or interrupt worker or similar.
> > > 
> > > We've mostly focused on coming up with a solution that would annotate
> > > signaling paths in an automated way, and making sure dma_fence_signal()
> > > is never called outside of a non-annotated path:
> > > - creation of DmaFenceWorkqueue/DmaFence[Delayed]Work that guarantees
> > >   all works are executed in a dma_fence_signalling_{begin,end}()
> > >   section, so we can properly detect deadlocks (through lockdep)
> > > - creation of a DmaFenceIrqHandler for the same reason
> > > - we'll need variants for each new deferred mechanism drivers might
> > >   want to use (kthread_worker?)
> > > 
> > > But there's currently no restriction on calling dma_fence_signal() in a
> > > user thread context (IOCTL()). I guess that shouldn't be too hard to
> > > add (is_user_task() to the rescue).
> > > 
> > > > 
> > > > A DMA-fence should *never* signal because of an IOCTL
> > > 
> > > Okay, that's understandable.
> > > 
> > > > or because some
> > > > object runs out of scope. E.g. when you cleanup a HW ring buffer, FW
> > > > queue, etc...
> > > 
> > > We were actually going in the opposite direction:
> > > auto-signal(ECANCELED) on DriverFenceTimeline object destruction
> 
> Absolutely clear NAK to that, we have iterated that many times before on the C side as well.
> 
> See below for the explanation of the background.
> 
> > > (which
> > > is the thing that would be attached to the HW ringbuf. The reason is:
> > > we don't want to leave unsignalled fences behind,
> > > 
> > 
> > Not only do we not "want to", we actually *cannot*. We have to make
> > sure all fences are signaled because only this way the C backend plus
> > RCU can protect also the Rust code against UAF.
> > 
> > >  and if the HW ring is
> > > gone, there's nothing that can signal it. Mind explaining why you think
> > > this shouldn't be done, because I originally interpreted your
> > > suggestion as exactly the opposite.
> > 
> > I also don't get it. All fences must always get signaled, that's one of
> > the most fundamental fence rules. Thus, if the last accessor to a fence
> > drops, you do want to signal it with -ECANCELED
> 
> All fences must always signal because the HW operation must always complete or be terminated by a timeout.
> 
> If a fence signals only because it runs out of scope than that means that you have a huge potential for data corruption and that is even worse than not signaling a fence.
> 
> In other words not signaling a fence can leave the system in a deadlock state, but signaling it incorrectly usually results in random data corruption.

It all stands and falls with the question whether a fence can drop by
accident in Rust, or if it will only ever drop when the hw-ring is
closed.

What do you believe is the right thing to do when a driver unloads?

Ideally we could design it in a way that the driver closes its rings,
the pending fences drop and get signaled with ECANCELED.

Your concern seems to be a driver by accident droping a fence while the
hardware is still processing the associated job.

(how's that dangerous, though? Shouldn't parties waiting for the fence
detect the error? ECANCELED ⇒ you must not access the associated
memory)


P.


> 
> Saying that we could potentially make dma_fence_release() more resilient to ref-counting issues.
> 
> Regards,
> Christian.
> 
> > 
> > 
> > P.
> 

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 13:36, Philipp Stanner wrote:
>>>> (which
>>>> is the thing that would be attached to the HW ringbuf. The reason is:
>>>> we don't want to leave unsignalled fences behind,
>>>>
>>>
>>> Not only do we not "want to", we actually *cannot*. We have to make
>>> sure all fences are signaled because only this way the C backend plus
>>> RCU can protect also the Rust code against UAF.
>>>
>>>>  and if the HW ring is
>>>> gone, there's nothing that can signal it. Mind explaining why you think
>>>> this shouldn't be done, because I originally interpreted your
>>>> suggestion as exactly the opposite.
>>>
>>> I also don't get it. All fences must always get signaled, that's one of
>>> the most fundamental fence rules. Thus, if the last accessor to a fence
>>> drops, you do want to signal it with -ECANCELED
>>
>> All fences must always signal because the HW operation must always complete or be terminated by a timeout.
>>
>> If a fence signals only because it runs out of scope than that means that you have a huge potential for data corruption and that is even worse than not signaling a fence.
>>
>> In other words not signaling a fence can leave the system in a deadlock state, but signaling it incorrectly usually results in random data corruption.
> 
> It all stands and falls with the question whether a fence can drop by
> accident in Rust, or if it will only ever drop when the hw-ring is
> closed.
> 
> What do you believe is the right thing to do when a driver unloads?

Do a dma_fence_wait() to make sure that all HW operations have completed and all fences signaled.

> Ideally we could design it in a way that the driver closes its rings,
> the pending fences drop and get signaled with ECANCELED.

No, exactly that is a really bad idea.

Just do it the other way around, use the dma_fence to wait for the HW operation to be completed.

Then wait for an RCU grace period to make sure that nobody is still inside your DMA fence ops.

And then you can continue with unloading the module.

> Your concern seems to be a driver by accident droping a fence while the
> hardware is still processing the associated job.
> 
> (how's that dangerous, though? Shouldn't parties waiting for the fence
> detect the error? ECANCELED ⇒ you must not access the associated
> memory)

The dma_fence is the SW object which represents the HW operation.

And that HW operation is doing DMA, e.g. accessing and potentially writing into memory. That's where the name Direct Memory Access comes from.

So when that is messed up the memory which gets written to is potentially re-used with the absolutely dire consequences we have seen so many times.

Keep in mind that this framework is not only used by GPU where at least modern ones have VM protection, but also old ones and stuff like V4L were such things is just not present in any way.

Regards,
Christian.

> 
> 
> P.
> 
> 
>>
>> Saying that we could potentially make dma_fence_release() more resilient to ref-counting issues.
>>
>> Regards,
>> Christian.
>>
>>>
>>>
>>> P.
>>
> 

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 6 Mar 2026 13:54:15 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/6/26 13:36, Philipp Stanner wrote:
> >>>> (which
> >>>> is the thing that would be attached to the HW ringbuf. The
> >>>> reason is: we don't want to leave unsignalled fences behind,
> >>>>  
> >>>
> >>> Not only do we not "want to", we actually *cannot*. We have to
> >>> make sure all fences are signaled because only this way the C
> >>> backend plus RCU can protect also the Rust code against UAF.
> >>>  
> >>>>  and if the HW ring is
> >>>> gone, there's nothing that can signal it. Mind explaining why
> >>>> you think this shouldn't be done, because I originally
> >>>> interpreted your suggestion as exactly the opposite.  
> >>>
> >>> I also don't get it. All fences must always get signaled, that's
> >>> one of the most fundamental fence rules. Thus, if the last
> >>> accessor to a fence drops, you do want to signal it with
> >>> -ECANCELED  
> >>
> >> All fences must always signal because the HW operation must always
> >> complete or be terminated by a timeout.
> >>
> >> If a fence signals only because it runs out of scope than that
> >> means that you have a huge potential for data corruption and that
> >> is even worse than not signaling a fence.
> >>
> >> In other words not signaling a fence can leave the system in a
> >> deadlock state, but signaling it incorrectly usually results in
> >> random data corruption.  
> > 
> > It all stands and falls with the question whether a fence can drop
> > by accident in Rust, or if it will only ever drop when the hw-ring
> > is closed.
> > 
> > What do you believe is the right thing to do when a driver unloads?
> >  
> 
> Do a dma_fence_wait() to make sure that all HW operations have
> completed and all fences signaled.
> 
> > Ideally we could design it in a way that the driver closes its
> > rings, the pending fences drop and get signaled with ECANCELED.  
> 
> No, exactly that is a really bad idea.
> 
> Just do it the other way around, use the dma_fence to wait for the HW
> operation to be completed.

But in practice you don't just wait for the HW to finish most of the
time. You instruct the HW to stop processing stuff, and then wait for it
to acknowledge that it indeed stopped. And the HWRing object will only
be dropped when that happens. There's of course a fallback for the case
where the STOP operation fails (with reset, etc), which would just
delay the drop of the HWRing. But the point is, when the HWRing is
dropped, you should be guaranteed that the HW is no longer executing
any of the possibly pending jobs. Now, of course you can decide that
it's the driver responsibility to walk the list of jobs that were
pending after a STOP has been acked and manually signal all fences, or
you can assume that the HWRing being dropped is what provides this
guarantee. And because the HWRing embeds you DmaFenceCtx that
auto-signal on drops, you don't have to do anything.

> 
> Then wait for an RCU grace period to make sure that nobody is still
> inside your DMA fence ops.
> 
> And then you can continue with unloading the module.
> 
> > Your concern seems to be a driver by accident droping a fence while
> > the hardware is still processing the associated job.
> > 
> > (how's that dangerous, though? Shouldn't parties waiting for the
> > fence detect the error? ECANCELED ⇒ you must not access the
> > associated memory)  
> 
> The dma_fence is the SW object which represents the HW operation.
> 
> And that HW operation is doing DMA, e.g. accessing and potentially
> writing into memory. That's where the name Direct Memory Access comes
> from.
> 
> So when that is messed up the memory which gets written to is
> potentially re-used with the absolutely dire consequences we have
> seen so many times.

But then, I'd argue that the HWRing (and the underlying FenceCtx
keeping track of emitted fences on this ring) should live at least as
long as the HW is assumed to be running commands. That's IMHO the
guarantee we need to provide: don't drop your HWRing object until
you're sure the HW has stopped pulling stuff from there. I can think
of the following two cases:

1. The HW has a way to prematurely stop stuff that are currently
executing. First thing we do is ensure the HW can't pull anything new,
then we issue a STOP and wait for an ACK. When this ACK is received, we
proceed with the rest of the cleanup. If the ACK doesn't come in time,
we flag the HWRing unusable, schedule a reset, and wait for this reset
to be effective before dropping the HWRing.

2. The HW can't stop what it started doing. We make sure nothing new
can be pushed to the HWRing, we wait for the in-flight ops to land. If
it's taking too long, the timeout handler will take over, schedule a
reset, and drop the faulty HWRing only after the reset is effective.

> 
> Keep in mind that this framework is not only used by GPU where at
> least modern ones have VM protection, but also old ones and stuff
> like V4L were such things is just not present in any way.

I'm not questioning the fact signaling fences prematurely can lead to
terrible security holes which are worse than deadlocks, I'm questioning
the fact this "dont-signal-before-HW-is-done" guarantee needs to happen
at the fence level, rather than at the fence emitter level.

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 15:55, Boris Brezillon wrote:
> On Fri, 6 Mar 2026 13:54:15 +0100
> Christian König <christian.koenig@amd.com> wrote:
> 
>> On 3/6/26 13:36, Philipp Stanner wrote:
>>>>>> (which
>>>>>> is the thing that would be attached to the HW ringbuf. The
>>>>>> reason is: we don't want to leave unsignalled fences behind,
>>>>>>  
>>>>>
>>>>> Not only do we not "want to", we actually *cannot*. We have to
>>>>> make sure all fences are signaled because only this way the C
>>>>> backend plus RCU can protect also the Rust code against UAF.
>>>>>  
>>>>>>  and if the HW ring is
>>>>>> gone, there's nothing that can signal it. Mind explaining why
>>>>>> you think this shouldn't be done, because I originally
>>>>>> interpreted your suggestion as exactly the opposite.  
>>>>>
>>>>> I also don't get it. All fences must always get signaled, that's
>>>>> one of the most fundamental fence rules. Thus, if the last
>>>>> accessor to a fence drops, you do want to signal it with
>>>>> -ECANCELED  
>>>>
>>>> All fences must always signal because the HW operation must always
>>>> complete or be terminated by a timeout.
>>>>
>>>> If a fence signals only because it runs out of scope than that
>>>> means that you have a huge potential for data corruption and that
>>>> is even worse than not signaling a fence.
>>>>
>>>> In other words not signaling a fence can leave the system in a
>>>> deadlock state, but signaling it incorrectly usually results in
>>>> random data corruption.  
>>>
>>> It all stands and falls with the question whether a fence can drop
>>> by accident in Rust, or if it will only ever drop when the hw-ring
>>> is closed.
>>>
>>> What do you believe is the right thing to do when a driver unloads?
>>>  
>>
>> Do a dma_fence_wait() to make sure that all HW operations have
>> completed and all fences signaled.
>>
>>> Ideally we could design it in a way that the driver closes its
>>> rings, the pending fences drop and get signaled with ECANCELED.  
>>
>> No, exactly that is a really bad idea.
>>
>> Just do it the other way around, use the dma_fence to wait for the HW
>> operation to be completed.
> 
> But in practice you don't just wait for the HW to finish most of the
> time. You instruct the HW to stop processing stuff, and then wait for it
> to acknowledge that it indeed stopped.

And how does the HW acknowledged that it has indeed stopped? Maybe by sending an interrupt which signals a DMA-fence?

The point here is that all acknowledgement from the HW that a DMA operation was indeed stopped, independent if it's the normal operation completed use case or if it's the I have aborted use case, *must* always take the same HW and SW path.

It is *not* sufficient that you do something like busy waiting for a bit in a register to flip in the abortion path and for a DMA memory write in the normal completion path.

That's why MMU/VM inside a device is usually not sufficient to prevent freed memory from being written to. You need an IOMMU for that, e.g. close to the CPU/memory and without caches behind the HW path.

I should probably write that down in some documentation.

Regards,
Christian.

> And the HWRing object will only
> be dropped when that happens. There's of course a fallback for the case
> where the STOP operation fails (with reset, etc), which would just
> delay the drop of the HWRing. But the point is, when the HWRing is
> dropped, you should be guaranteed that the HW is no longer executing
> any of the possibly pending jobs. Now, of course you can decide that
> it's the driver responsibility to walk the list of jobs that were
> pending after a STOP has been acked and manually signal all fences, or
> you can assume that the HWRing being dropped is what provides this
> guarantee. And because the HWRing embeds you DmaFenceCtx that
> auto-signal on drops, you don't have to do anything.
> 
>>
>> Then wait for an RCU grace period to make sure that nobody is still
>> inside your DMA fence ops.
>>
>> And then you can continue with unloading the module.
>>
>>> Your concern seems to be a driver by accident droping a fence while
>>> the hardware is still processing the associated job.
>>>
>>> (how's that dangerous, though? Shouldn't parties waiting for the
>>> fence detect the error? ECANCELED ⇒ you must not access the
>>> associated memory)  
>>
>> The dma_fence is the SW object which represents the HW operation.
>>
>> And that HW operation is doing DMA, e.g. accessing and potentially
>> writing into memory. That's where the name Direct Memory Access comes
>> from.
>>
>> So when that is messed up the memory which gets written to is
>> potentially re-used with the absolutely dire consequences we have
>> seen so many times.
> 
> But then, I'd argue that the HWRing (and the underlying FenceCtx
> keeping track of emitted fences on this ring) should live at least as
> long as the HW is assumed to be running commands. That's IMHO the
> guarantee we need to provide: don't drop your HWRing object until
> you're sure the HW has stopped pulling stuff from there. I can think
> of the following two cases:
> 
> 1. The HW has a way to prematurely stop stuff that are currently
> executing. First thing we do is ensure the HW can't pull anything new,
> then we issue a STOP and wait for an ACK. When this ACK is received, we
> proceed with the rest of the cleanup. If the ACK doesn't come in time,
> we flag the HWRing unusable, schedule a reset, and wait for this reset
> to be effective before dropping the HWRing.
> 
> 2. The HW can't stop what it started doing. We make sure nothing new
> can be pushed to the HWRing, we wait for the in-flight ops to land. If
> it's taking too long, the timeout handler will take over, schedule a
> reset, and drop the faulty HWRing only after the reset is effective.
> 
>>
>> Keep in mind that this framework is not only used by GPU where at
>> least modern ones have VM protection, but also old ones and stuff
>> like V4L were such things is just not present in any way.
> 
> I'm not questioning the fact signaling fences prematurely can lead to
> terrible security holes which are worse than deadlocks, I'm questioning
> the fact this "dont-signal-before-HW-is-done" guarantee needs to happen
> at the fence level, rather than at the fence emitter level.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Mon, 9 Mar 2026 10:33:10 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/6/26 15:55, Boris Brezillon wrote:
> > On Fri, 6 Mar 2026 13:54:15 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> >   
> >> On 3/6/26 13:36, Philipp Stanner wrote:  
> >>>>>> (which
> >>>>>> is the thing that would be attached to the HW ringbuf. The
> >>>>>> reason is: we don't want to leave unsignalled fences behind,
> >>>>>>    
> >>>>>
> >>>>> Not only do we not "want to", we actually *cannot*. We have to
> >>>>> make sure all fences are signaled because only this way the C
> >>>>> backend plus RCU can protect also the Rust code against UAF.
> >>>>>    
> >>>>>>  and if the HW ring is
> >>>>>> gone, there's nothing that can signal it. Mind explaining why
> >>>>>> you think this shouldn't be done, because I originally
> >>>>>> interpreted your suggestion as exactly the opposite.    
> >>>>>
> >>>>> I also don't get it. All fences must always get signaled, that's
> >>>>> one of the most fundamental fence rules. Thus, if the last
> >>>>> accessor to a fence drops, you do want to signal it with
> >>>>> -ECANCELED    
> >>>>
> >>>> All fences must always signal because the HW operation must
> >>>> always complete or be terminated by a timeout.
> >>>>
> >>>> If a fence signals only because it runs out of scope than that
> >>>> means that you have a huge potential for data corruption and that
> >>>> is even worse than not signaling a fence.
> >>>>
> >>>> In other words not signaling a fence can leave the system in a
> >>>> deadlock state, but signaling it incorrectly usually results in
> >>>> random data corruption.    
> >>>
> >>> It all stands and falls with the question whether a fence can drop
> >>> by accident in Rust, or if it will only ever drop when the hw-ring
> >>> is closed.
> >>>
> >>> What do you believe is the right thing to do when a driver
> >>> unloads? 
> >>
> >> Do a dma_fence_wait() to make sure that all HW operations have
> >> completed and all fences signaled.
> >>  
> >>> Ideally we could design it in a way that the driver closes its
> >>> rings, the pending fences drop and get signaled with ECANCELED.
> >>>  
> >>
> >> No, exactly that is a really bad idea.
> >>
> >> Just do it the other way around, use the dma_fence to wait for the
> >> HW operation to be completed.  
> > 
> > But in practice you don't just wait for the HW to finish most of the
> > time. You instruct the HW to stop processing stuff, and then wait
> > for it to acknowledge that it indeed stopped.  
> 
> And how does the HW acknowledged that it has indeed stopped? Maybe by
> sending an interrupt which signals a DMA-fence?

Yes, it's likely something like a _STATUS register update reflecting
the new HW state, plus an interrupt to wake the CPU up. The decision to
poll the status register or go the async-way is up to the driver.

> 
> The point here is that all acknowledgement from the HW that a DMA
> operation was indeed stopped, independent if it's the normal
> operation completed use case or if it's the I have aborted use case,
> *must* always take the same HW and SW path.
> 
> It is *not* sufficient that you do something like busy waiting for a
> bit in a register to flip in the abortion path and for a DMA memory
> write in the normal completion path.

I'm assuming the DMA_OP_COMPLETE is also a register update of some
sort. But let's assume it's not, then sure, we need to make sure the
operation is either complete (event received through the IRQ handler),
or the DMA engine is fully stopped. Doesn't really matter which path is
doing this check, as long as it's done.

> 
> That's why MMU/VM inside a device is usually not sufficient to
> prevent freed memory from being written to. You need an IOMMU for
> that, e.g. close to the CPU/memory and without caches behind the HW
> path.

Either that, or you need a way to preempt DMA engine operations and
have them cancelled before they make it to the bus, plus wait for the
non-cancellable ones. And it doesn't really matter how the HW works,
because my point is not that we need to enforce how the SW can ensure
the HW is done processing the stuff (that's very HW specific), just
that there needs to be a way to do this SW <-> HW synchronization, and
it's the driver responsibility to ensure that ultimately. My second
point was that, once the HW block is considered idle, there might be
operations that were never dequeued because they were cancelled before
the HW got to it, and for those, we'll never get HW events. We just have
to walk the list and manually signal fences. That's the step I was
suggesting to automate through the auto-signal-on-drop approach, but we
can automate it through an explicit DriverFenceTimeline::cancel_all()
method, I guess.

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/9/26 16:06, Boris Brezillon wrote:
>>>>
>>>> Just do it the other way around, use the dma_fence to wait for the
>>>> HW operation to be completed.  
>>>
>>> But in practice you don't just wait for the HW to finish most of the
>>> time. You instruct the HW to stop processing stuff, and then wait
>>> for it to acknowledge that it indeed stopped.  
>>
>> And how does the HW acknowledged that it has indeed stopped? Maybe by
>> sending an interrupt which signals a DMA-fence?
> 
> Yes, it's likely something like a _STATUS register update reflecting
> the new HW state, plus an interrupt to wake the CPU up. The decision to
> poll the status register or go the async-way is up to the driver.

Exactly that's the bad idea we have iterated over so many times.

Ideally such stuff should *not* be up to the driver but enforced by the kernel.

>>
>> The point here is that all acknowledgement from the HW that a DMA
>> operation was indeed stopped, independent if it's the normal
>> operation completed use case or if it's the I have aborted use case,
>> *must* always take the same HW and SW path.
>>
>> It is *not* sufficient that you do something like busy waiting for a
>> bit in a register to flip in the abortion path and for a DMA memory
>> write in the normal completion path.
> 
> I'm assuming the DMA_OP_COMPLETE is also a register update of some
> sort. But let's assume it's not, then sure, we need to make sure the
> operation is either complete (event received through the IRQ handler),
> or the DMA engine is fully stopped. Doesn't really matter which path is
> doing this check, as long as it's done.

Well, it is massively important to get that right or otherwise you end up with random memory corruption.

And drivers notoriously get that handling wrong resulting and much worse issues than a simple UAF.

>>
>> That's why MMU/VM inside a device is usually not sufficient to
>> prevent freed memory from being written to. You need an IOMMU for
>> that, e.g. close to the CPU/memory and without caches behind the HW
>> path.
> 
> Either that, or you need a way to preempt DMA engine operations and
> have them cancelled before they make it to the bus, plus wait for the
> non-cancellable ones. And it doesn't really matter how the HW works,
> because my point is not that we need to enforce how the SW can ensure
> the HW is done processing the stuff (that's very HW specific),

At least for PCIe devices that is pretty standardized. You need something which is ordered with respect to your DMA transactions, so you either end up with a write or an interrupt from the HW side.

How it is implemented in the end (32bit vs 64bit fences, writes vs interrupts etc...) is HW specific, but that is actually only a really minor part of the handling.

The problem is that what you describe above with "DMA_OP_COMPLETE is also a register update of some sort" is exactly what we have seen before as not working because MMIO register reads from the CPU side are not necessarily ordered with device writes.

> just
> that there needs to be a way to do this SW <-> HW synchronization, and
> it's the driver responsibility to ensure that ultimately. My second
> point was that, once the HW block is considered idle, there might be
> operations that were never dequeued because they were cancelled before
> the HW got to it, and for those, we'll never get HW events. We just have
> to walk the list and manually signal fences. That's the step I was
> suggesting to automate through the auto-signal-on-drop approach, but we
> can automate it through an explicit DriverFenceTimeline::cancel_all()
> method, I guess.

I strongly suggest to at least document what is known to work and what not.

Regards,
Christian.

Re: dma_fence: force users to take the lock manually

[Danilo Krummrich]

On Fri Mar 6, 2026 at 1:36 PM CET, Philipp Stanner wrote:
> On Fri, 2026-03-06 at 13:31 +0100, Christian König wrote:
>> All fences must always signal because the HW operation must always complete
>> or be terminated by a timeout.
>> 
>> If a fence signals only because it runs out of scope than that means that you
>> have a huge potential for data corruption and that is even worse than not
>> signaling a fence.

If that happens, it is a functional bug, the potential data corruption is only
within a separate memory object, e.g. GEM etc., no? I.e. it may fault the GPU,
but it does not fault the kernel.

>> In other words not signaling a fence can leave the system in a deadlock
>> state, but signaling it incorrectly usually results in random data
>> corruption.

Well, not signaling it results in a potential deadlock of the whole kernel,
whereas wrongly signaling it is "only" a functional bug.

> It all stands and falls with the question whether a fence can drop by
> accident in Rust, or if it will only ever drop when the hw-ring is
> closed.
>
> What do you believe is the right thing to do when a driver unloads?

The fence has to be signaled -- ideally after shutting down all queues, but it
has to be signaled.

> Ideally we could design it in a way that the driver closes its rings,
> the pending fences drop and get signaled with ECANCELED.
>
> Your concern seems to be a driver by accident droping a fence while the
> hardware is still processing the associated job.

I'm not concerned about the "driver drops fence by accident" case, as it is less
problematic than the "driver forgets to signal the fence" case. One is a logic
bug, whereas the other can deadlock the kernel, i.e. it is unsafe in terms of
Rust.

(Technically, there are subsequent problems to solve, as core::mem::forget() is
safe and would cause the same problem. However, this is not new, it applies to
lock guards in general. We can catch such things with klint though.)

Ultimately, a DMA fence (that has been exposed to the outside world) is
technically equivalent to a lock guard.

> (how's that dangerous, though? Shouldn't parties waiting for the fence
> detect the error? ECANCELED ⇒ you must not access the associated
> memory)

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 14:03, Danilo Krummrich wrote:
> On Fri Mar 6, 2026 at 1:36 PM CET, Philipp Stanner wrote:
>> On Fri, 2026-03-06 at 13:31 +0100, Christian König wrote:
>>> All fences must always signal because the HW operation must always complete
>>> or be terminated by a timeout.
>>>
>>> If a fence signals only because it runs out of scope than that means that you
>>> have a huge potential for data corruption and that is even worse than not
>>> signaling a fence.
> 
> If that happens, it is a functional bug, the potential data corruption is only
> within a separate memory object, e.g. GEM etc., no? I.e. it may fault the GPU,
> but it does not fault the kernel.

That makes assumption that DMA operations are protected by an MMU which provides virtual memory.

But the VM functionality of modern GPUs are the exception and not the norm for devices using DMA fences.

>>> In other words not signaling a fence can leave the system in a deadlock
>>> state, but signaling it incorrectly usually results in random data
>>> corruption.
> 
> Well, not signaling it results in a potential deadlock of the whole kernel,
> whereas wrongly signaling it is "only" a functional bug.

No, that results in random memory corruption. Which is easily a magnitude worse than just a kernel deadlock.

When have seen such bugs numerous times with suspend and resume on laptops in different subsystems, e.g. not only GPU. And I'm absolutely clearly rejecting any attempt to signal DMA fences when an object runs out of scope because of that experience.

>> It all stands and falls with the question whether a fence can drop by
>> accident in Rust, or if it will only ever drop when the hw-ring is
>> closed.
>>
>> What do you believe is the right thing to do when a driver unloads?
> 
> The fence has to be signaled -- ideally after shutting down all queues, but it
> has to be signaled.

Yeah well this shutting down all queues (and making sure that no write operation is pending in caches etc...) is what people usually don't get right.

What you can to is things like setting your timeout to zero and immediately causing terminating the HW operation and resetting the device.

This will then use the same code path as the mandatory timeout functionality for DMA operations and that usually works reliable.

>> Ideally we could design it in a way that the driver closes its rings,
>> the pending fences drop and get signaled with ECANCELED.
>>
>> Your concern seems to be a driver by accident droping a fence while the
>> hardware is still processing the associated job.
> 
> I'm not concerned about the "driver drops fence by accident" case, as it is less
> problematic than the "driver forgets to signal the fence" case. One is a logic
> bug, whereas the other can deadlock the kernel, i.e. it is unsafe in terms of
> Rust.
> 
> (Technically, there are subsequent problems to solve, as core::mem::forget() is
> safe and would cause the same problem. However, this is not new, it applies to
> lock guards in general. We can catch such things with klint though.)
> 
> Ultimately, a DMA fence (that has been exposed to the outside world) is
> technically equivalent to a lock guard.

+1

Yes, exactly that.

Regards,
Christian.

> 
>> (how's that dangerous, though? Shouldn't parties waiting for the fence
>> detect the error? ECANCELED ⇒ you must not access the associated
>> memory)

Re: dma_fence: force users to take the lock manually

[Philipp Stanner]

On Fri, 2026-03-06 at 14:15 +0100, Christian König wrote:
> On 3/6/26 14:03, Danilo Krummrich wrote:
> > On Fri Mar 6, 2026 at 1:36 PM CET, Philipp Stanner wrote:
> > > On Fri, 2026-03-06 at 13:31 +0100, Christian König wrote:
> > > > All fences must always signal because the HW operation must always complete
> > > > or be terminated by a timeout.
> > > > 
> > > > If a fence signals only because it runs out of scope than that means that you
> > > > have a huge potential for data corruption and that is even worse than not
> > > > signaling a fence.
> > 
> > If that happens, it is a functional bug, the potential data corruption is only
> > within a separate memory object, e.g. GEM etc., no? I.e. it may fault the GPU,
> > but it does not fault the kernel.
> 
> That makes assumption that DMA operations are protected by an MMU which provides virtual memory.
> 
> But the VM functionality of modern GPUs are the exception and not the norm for devices using DMA fences.
> 
> > > > In other words not signaling a fence can leave the system in a deadlock
> > > > state, but signaling it incorrectly usually results in random data
> > > > corruption.
> > 
> > Well, not signaling it results in a potential deadlock of the whole kernel,
> > whereas wrongly signaling it is "only" a functional bug.
> 
> No, that results in random memory corruption. Which is easily a magnitude worse than just a kernel deadlock.
> 
> When have seen such bugs numerous times with suspend and resume on laptops in different subsystems, e.g. not only GPU. And I'm absolutely clearly rejecting any attempt to signal DMA fences when an object runs out of scope because of that experience.

But you're aware that both in C and Rust you could experience UAF bugs
if fences drop unsignaled and the driver unloads?

Though I tentatively agree that memory corruptions on a large scale are
probably the worst error you can have on a computer.

> 
> > > It all stands and falls with the question whether a fence can drop by
> > > accident in Rust, or if it will only ever drop when the hw-ring is
> > > closed.
> > > 
> > > What do you believe is the right thing to do when a driver unloads?
> > 
> > The fence has to be signaled -- ideally after shutting down all queues, but it
> > has to be signaled.
> 
> Yeah well this shutting down all queues (and making sure that no write operation is pending in caches etc...) is what people usually don't get right.
> 
> What you can to is things like setting your timeout to zero and immediately causing terminating the HW operation and resetting the device.
> 
> This will then use the same code path as the mandatory timeout functionality for DMA operations and that usually works reliable.

Why is all that even an issue? The driver controls the hardware and
must "switch it off" before it unloads. Then the hardware will not
access any memory anymore for sure.


P.

> 
> > > Ideally we could design it in a way that the driver closes its rings,
> > > the pending fences drop and get signaled with ECANCELED.
> > > 
> > > Your concern seems to be a driver by accident droping a fence while the
> > > hardware is still processing the associated job.
> > 
> > I'm not concerned about the "driver drops fence by accident" case, as it is less
> > problematic than the "driver forgets to signal the fence" case. One is a logic
> > bug, whereas the other can deadlock the kernel, i.e. it is unsafe in terms of
> > Rust.
> > 
> > (Technically, there are subsequent problems to solve, as core::mem::forget() is
> > safe and would cause the same problem. However, this is not new, it applies to
> > lock guards in general. We can catch such things with klint though.)
> > 
> > Ultimately, a DMA fence (that has been exposed to the outside world) is
> > technically equivalent to a lock guard.
> 
> +1
> 
> Yes, exactly that.
> 
> Regards,
> Christian.
> 
> > 
> > > (how's that dangerous, though? Shouldn't parties waiting for the fence
> > > detect the error? ECANCELED ⇒ you must not access the associated
> > > memory)
> 

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/6/26 14:36, Philipp Stanner wrote:
>>>>> In other words not signaling a fence can leave the system in a deadlock
>>>>> state, but signaling it incorrectly usually results in random data
>>>>> corruption.
>>>
>>> Well, not signaling it results in a potential deadlock of the whole kernel,
>>> whereas wrongly signaling it is "only" a functional bug.
>>
>> No, that results in random memory corruption. Which is easily a magnitude worse than just a kernel deadlock.
>>
>> When have seen such bugs numerous times with suspend and resume on laptops in different subsystems, e.g. not only GPU. And I'm absolutely clearly rejecting any attempt to signal DMA fences when an object runs out of scope because of that experience.
> 
> But you're aware that both in C and Rust you could experience UAF bugs
> if fences drop unsignaled and the driver unloads?
> 
> Though I tentatively agree that memory corruptions on a large scale are
> probably the worst error you can have on a computer.

Yeah, of course I'm aware of the UAF issue we have.

But those are relatively harmless compared to the random memory corruption issues.

Linux has the unfortunate habit of re-using memory directly after freeing it because that means caches are usually hotter.

So rough DMA operations have the tendency to end up anywhere and tools like KASAN can't find anything wrong.

The only protection you sometimes have is IOMMU, but that is usually not able to catch everything either.

>>
>>>> It all stands and falls with the question whether a fence can drop by
>>>> accident in Rust, or if it will only ever drop when the hw-ring is
>>>> closed.
>>>>
>>>> What do you believe is the right thing to do when a driver unloads?
>>>
>>> The fence has to be signaled -- ideally after shutting down all queues, but it
>>> has to be signaled.
>>
>> Yeah well this shutting down all queues (and making sure that no write operation is pending in caches etc...) is what people usually don't get right.
>>
>> What you can to is things like setting your timeout to zero and immediately causing terminating the HW operation and resetting the device.
>>
>> This will then use the same code path as the mandatory timeout functionality for DMA operations and that usually works reliable.
> 
> Why is all that even an issue? The driver controls the hardware and
> must "switch it off" before it unloads. Then the hardware will not
> access any memory anymore for sure.

Well exactly that is usually really complicated. Let me try to explain that on a HW example. 

Between a shader and the actual system memory you usually have different IP blocks or stages where a memory access needs to go through:

Shader -> device VM -> device cache -> PCI bus -> CPU cache -> memory

Now when you want to terminate some shader or make some memory inaccessible because it is freed drivers update their page tables and issue the equivalent of TLB invalidation on a CPU.

The problem is now that this will only invalidate the translation in the device VM. It doesn't affect the device cache nor any ongoing memory transaction on the bus which waits to snoop the CPU cache.

To make sure that you don't corrupt system memory you actually need to wait for a cache flush event to be signaled and *not* just update the VM page tables and tell the HW to invalidate it's TLB.

So what is needed is usually a fence operation. In other words a memory value written over the PCIe bus into system memory. Background is that memory writes are ordered and this one comes after all previous PCIe memory writes of the device and so is in the correct order.

Only when the CPU sees this memory write you can be sure that your operation is completed.

This memory write is then often implemented by using an MSI interrupt which in turn signals the DMA fence.


So the right thing to do is to wait for the DMA fence to signal through its normal signaling path which includes both HW and SW functionality and *not* just tell the HW to stop some ring and then just assume that this is also sufficient to signal the DMA fence associated with the HW operation.

Regards,
Christian.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 6 Mar 2026 15:37:31 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 3/6/26 14:36, Philipp Stanner wrote:
> >>>>> In other words not signaling a fence can leave the system in a
> >>>>> deadlock state, but signaling it incorrectly usually results in
> >>>>> random data corruption.  
> >>>
> >>> Well, not signaling it results in a potential deadlock of the
> >>> whole kernel, whereas wrongly signaling it is "only" a functional
> >>> bug.  
> >>
> >> No, that results in random memory corruption. Which is easily a
> >> magnitude worse than just a kernel deadlock.
> >>
> >> When have seen such bugs numerous times with suspend and resume on
> >> laptops in different subsystems, e.g. not only GPU. And I'm
> >> absolutely clearly rejecting any attempt to signal DMA fences when
> >> an object runs out of scope because of that experience.  
> > 
> > But you're aware that both in C and Rust you could experience UAF
> > bugs if fences drop unsignaled and the driver unloads?
> > 
> > Though I tentatively agree that memory corruptions on a large scale
> > are probably the worst error you can have on a computer.  
> 
> Yeah, of course I'm aware of the UAF issue we have.
> 
> But those are relatively harmless compared to the random memory
> corruption issues.
> 
> Linux has the unfortunate habit of re-using memory directly after
> freeing it because that means caches are usually hotter.
> 
> So rough DMA operations have the tendency to end up anywhere and
> tools like KASAN can't find anything wrong.
> 
> The only protection you sometimes have is IOMMU, but that is usually
> not able to catch everything either.
> 
> >>  
> >>>> It all stands and falls with the question whether a fence can
> >>>> drop by accident in Rust, or if it will only ever drop when the
> >>>> hw-ring is closed.
> >>>>
> >>>> What do you believe is the right thing to do when a driver
> >>>> unloads?  
> >>>
> >>> The fence has to be signaled -- ideally after shutting down all
> >>> queues, but it has to be signaled.  
> >>
> >> Yeah well this shutting down all queues (and making sure that no
> >> write operation is pending in caches etc...) is what people
> >> usually don't get right.
> >>
> >> What you can to is things like setting your timeout to zero and
> >> immediately causing terminating the HW operation and resetting the
> >> device.
> >>
> >> This will then use the same code path as the mandatory timeout
> >> functionality for DMA operations and that usually works reliable.  
> > 
> > Why is all that even an issue? The driver controls the hardware and
> > must "switch it off" before it unloads. Then the hardware will not
> > access any memory anymore for sure.  
> 
> Well exactly that is usually really complicated. Let me try to
> explain that on a HW example. 
> 
> Between a shader and the actual system memory you usually have
> different IP blocks or stages where a memory access needs to go
> through:
> 
> Shader -> device VM -> device cache -> PCI bus -> CPU cache -> memory
> 
> Now when you want to terminate some shader or make some memory
> inaccessible because it is freed drivers update their page tables and
> issue the equivalent of TLB invalidation on a CPU.
> 
> The problem is now that this will only invalidate the translation in
> the device VM. It doesn't affect the device cache nor any ongoing
> memory transaction on the bus which waits to snoop the CPU cache.
> 
> To make sure that you don't corrupt system memory you actually need
> to wait for a cache flush event to be signaled and *not* just update
> the VM page tables and tell the HW to invalidate it's TLB.
> 
> So what is needed is usually a fence operation. In other words a
> memory value written over the PCIe bus into system memory. Background
> is that memory writes are ordered and this one comes after all
> previous PCIe memory writes of the device and so is in the correct
> order.
> 
> Only when the CPU sees this memory write you can be sure that your
> operation is completed.
> 
> This memory write is then often implemented by using an MSI interrupt
> which in turn signals the DMA fence.
> 
> 
> So the right thing to do is to wait for the DMA fence to signal
> through its normal signaling path which includes both HW and SW
> functionality and *not* just tell the HW to stop some ring and then
> just assume that this is also sufficient to signal the DMA fence
> associated with the HW operation.

Ultimately this
"stop-HW-and-make-sure-all-outcomes-are-visible-even-for-partially-executed-jobs"
is something you'll have to do, no matter what. But it leading to
having to wait for each pending fence, I'm not too sure. What about the
case where jobs/ops further away in the HWRing were not even considered
for execution by the HW, because the STOP operation prevented them from
being dequeued. I'd expect that the only event we'll get for those is
"HW queue is properly stopped now". So at this point it's a matter of
signalling everything that's left, no? I mean, that's basically what
Panthor does:

1. it stops
2. wait for all executing ops to land (with all the cache maintenance,
etc, you described)
3. signal(ECANCELED) what's left (things not picked by the HW by
the time the STOP was effective).

It's currently done manually, but does it have to?

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 6 Mar 2026 16:25:25 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Fri, 6 Mar 2026 15:37:31 +0100
> Christian König <christian.koenig@amd.com> wrote:
> 
> > On 3/6/26 14:36, Philipp Stanner wrote:  
> > >>>>> In other words not signaling a fence can leave the system in a
> > >>>>> deadlock state, but signaling it incorrectly usually results in
> > >>>>> random data corruption.    
> > >>>
> > >>> Well, not signaling it results in a potential deadlock of the
> > >>> whole kernel, whereas wrongly signaling it is "only" a functional
> > >>> bug.    
> > >>
> > >> No, that results in random memory corruption. Which is easily a
> > >> magnitude worse than just a kernel deadlock.
> > >>
> > >> When have seen such bugs numerous times with suspend and resume on
> > >> laptops in different subsystems, e.g. not only GPU. And I'm
> > >> absolutely clearly rejecting any attempt to signal DMA fences when
> > >> an object runs out of scope because of that experience.    
> > > 
> > > But you're aware that both in C and Rust you could experience UAF
> > > bugs if fences drop unsignaled and the driver unloads?
> > > 
> > > Though I tentatively agree that memory corruptions on a large scale
> > > are probably the worst error you can have on a computer.    
> > 
> > Yeah, of course I'm aware of the UAF issue we have.
> > 
> > But those are relatively harmless compared to the random memory
> > corruption issues.
> > 
> > Linux has the unfortunate habit of re-using memory directly after
> > freeing it because that means caches are usually hotter.
> > 
> > So rough DMA operations have the tendency to end up anywhere and
> > tools like KASAN can't find anything wrong.
> > 
> > The only protection you sometimes have is IOMMU, but that is usually
> > not able to catch everything either.
> >   
> > >>    
> > >>>> It all stands and falls with the question whether a fence can
> > >>>> drop by accident in Rust, or if it will only ever drop when the
> > >>>> hw-ring is closed.
> > >>>>
> > >>>> What do you believe is the right thing to do when a driver
> > >>>> unloads?    
> > >>>
> > >>> The fence has to be signaled -- ideally after shutting down all
> > >>> queues, but it has to be signaled.    
> > >>
> > >> Yeah well this shutting down all queues (and making sure that no
> > >> write operation is pending in caches etc...) is what people
> > >> usually don't get right.
> > >>
> > >> What you can to is things like setting your timeout to zero and
> > >> immediately causing terminating the HW operation and resetting the
> > >> device.
> > >>
> > >> This will then use the same code path as the mandatory timeout
> > >> functionality for DMA operations and that usually works reliable.    
> > > 
> > > Why is all that even an issue? The driver controls the hardware and
> > > must "switch it off" before it unloads. Then the hardware will not
> > > access any memory anymore for sure.    
> > 
> > Well exactly that is usually really complicated. Let me try to
> > explain that on a HW example. 
> > 
> > Between a shader and the actual system memory you usually have
> > different IP blocks or stages where a memory access needs to go
> > through:
> > 
> > Shader -> device VM -> device cache -> PCI bus -> CPU cache -> memory
> > 
> > Now when you want to terminate some shader or make some memory
> > inaccessible because it is freed drivers update their page tables and
> > issue the equivalent of TLB invalidation on a CPU.
> > 
> > The problem is now that this will only invalidate the translation in
> > the device VM. It doesn't affect the device cache nor any ongoing
> > memory transaction on the bus which waits to snoop the CPU cache.
> > 
> > To make sure that you don't corrupt system memory you actually need
> > to wait for a cache flush event to be signaled and *not* just update
> > the VM page tables and tell the HW to invalidate it's TLB.
> > 
> > So what is needed is usually a fence operation. In other words a
> > memory value written over the PCIe bus into system memory. Background
> > is that memory writes are ordered and this one comes after all
> > previous PCIe memory writes of the device and so is in the correct
> > order.
> > 
> > Only when the CPU sees this memory write you can be sure that your
> > operation is completed.
> > 
> > This memory write is then often implemented by using an MSI interrupt
> > which in turn signals the DMA fence.
> > 
> > 
> > So the right thing to do is to wait for the DMA fence to signal
> > through its normal signaling path which includes both HW and SW
> > functionality and *not* just tell the HW to stop some ring and then
> > just assume that this is also sufficient to signal the DMA fence
> > associated with the HW operation.  
> 
> Ultimately this
> "stop-HW-and-make-sure-all-outcomes-are-visible-even-for-partially-executed-jobs"
> is something you'll have to do, no matter what. But it leading to
> having to wait for each pending fence, I'm not too sure. What about the
> case where jobs/ops further away in the HWRing were not even considered
> for execution by the HW, because the STOP operation prevented them from
> being dequeued. I'd expect that the only event we'll get for those is
> "HW queue is properly stopped now". So at this point it's a matter of
> signalling everything that's left, no? I mean, that's basically what
> Panthor does:
> 
> 1. it stops
> 2. wait for all executing ops to land (with all the cache maintenance,
> etc, you described)
> 3. signal(ECANCELED) what's left (things not picked by the HW by
> the time the STOP was effective).
> 
> It's currently done manually, but does it have to?

All this being said, I'm also a pragmatic guy, so if you tell us "no
way!" even after these arguments, I'd rather give up on this
auto-signaling feature and have rust drivers be forced to manually
signal stuff than have the whole Fence abstraction blocked. We can add
a warn_on!(!is_signaled()) in the DriverFence::drop() path for the time
being, so we can at least catch cases where the driver didn't signal
the fence before dropping the signal-able object.

Re: dma_fence: force users to take the lock manually

[Philipp Stanner]

On Fri, 2026-03-06 at 16:43 +0100, Boris Brezillon wrote:
> On Fri, 6 Mar 2026 16:25:25 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Fri, 6 Mar 2026 15:37:31 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> > 
> > > On 3/6/26 14:36, Philipp Stanner wrote:  
> > > > > > > > In other words not signaling a fence can leave the system in a
> > > > > > > > deadlock state, but signaling it incorrectly usually results in
> > > > > > > > random data corruption.    
> > > > > > 
> > > > > > Well, not signaling it results in a potential deadlock of the
> > > > > > whole kernel, whereas wrongly signaling it is "only" a functional
> > > > > > bug.    
> > > > > 
> > > > > No, that results in random memory corruption. Which is easily a
> > > > > magnitude worse than just a kernel deadlock.
> > > > > 
> > > > > When have seen such bugs numerous times with suspend and resume on
> > > > > laptops in different subsystems, e.g. not only GPU. And I'm
> > > > > absolutely clearly rejecting any attempt to signal DMA fences when
> > > > > an object runs out of scope because of that experience.    
> > > > 
> > > > But you're aware that both in C and Rust you could experience UAF
> > > > bugs if fences drop unsignaled and the driver unloads?
> > > > 
> > > > Though I tentatively agree that memory corruptions on a large scale
> > > > are probably the worst error you can have on a computer.    
> > > 
> > > Yeah, of course I'm aware of the UAF issue we have.
> > > 
> > > But those are relatively harmless compared to the random memory
> > > corruption issues.
> > > 
> > > Linux has the unfortunate habit of re-using memory directly after
> > > freeing it because that means caches are usually hotter.
> > > 
> > > So rough DMA operations have the tendency to end up anywhere and
> > > tools like KASAN can't find anything wrong.
> > > 
> > > The only protection you sometimes have is IOMMU, but that is usually
> > > not able to catch everything either.
> > >   
> > > > >    
> > > > > > > It all stands and falls with the question whether a fence can
> > > > > > > drop by accident in Rust, or if it will only ever drop when the
> > > > > > > hw-ring is closed.
> > > > > > > 
> > > > > > > What do you believe is the right thing to do when a driver
> > > > > > > unloads?    
> > > > > > 
> > > > > > The fence has to be signaled -- ideally after shutting down all
> > > > > > queues, but it has to be signaled.    
> > > > > 
> > > > > Yeah well this shutting down all queues (and making sure that no
> > > > > write operation is pending in caches etc...) is what people
> > > > > usually don't get right.
> > > > > 
> > > > > What you can to is things like setting your timeout to zero and
> > > > > immediately causing terminating the HW operation and resetting the
> > > > > device.
> > > > > 
> > > > > This will then use the same code path as the mandatory timeout
> > > > > functionality for DMA operations and that usually works reliable.    
> > > > 
> > > > Why is all that even an issue? The driver controls the hardware and
> > > > must "switch it off" before it unloads. Then the hardware will not
> > > > access any memory anymore for sure.    
> > > 
> > > Well exactly that is usually really complicated. Let me try to
> > > explain that on a HW example. 
> > > 
> > > Between a shader and the actual system memory you usually have
> > > different IP blocks or stages where a memory access needs to go
> > > through:
> > > 
> > > Shader -> device VM -> device cache -> PCI bus -> CPU cache -> memory
> > > 
> > > Now when you want to terminate some shader or make some memory
> > > inaccessible because it is freed drivers update their page tables and
> > > issue the equivalent of TLB invalidation on a CPU.
> > > 
> > > The problem is now that this will only invalidate the translation in
> > > the device VM. It doesn't affect the device cache nor any ongoing
> > > memory transaction on the bus which waits to snoop the CPU cache.
> > > 
> > > To make sure that you don't corrupt system memory you actually need
> > > to wait for a cache flush event to be signaled and *not* just update
> > > the VM page tables and tell the HW to invalidate it's TLB.
> > > 
> > > So what is needed is usually a fence operation. In other words a
> > > memory value written over the PCIe bus into system memory. Background
> > > is that memory writes are ordered and this one comes after all
> > > previous PCIe memory writes of the device and so is in the correct
> > > order.
> > > 
> > > Only when the CPU sees this memory write you can be sure that your
> > > operation is completed.
> > > 
> > > This memory write is then often implemented by using an MSI interrupt
> > > which in turn signals the DMA fence.
> > > 
> > > 
> > > So the right thing to do is to wait for the DMA fence to signal
> > > through its normal signaling path which includes both HW and SW
> > > functionality and *not* just tell the HW to stop some ring and then
> > > just assume that this is also sufficient to signal the DMA fence
> > > associated with the HW operation.  
> > 
> > Ultimately this
> > "stop-HW-and-make-sure-all-outcomes-are-visible-even-for-partially-executed-jobs"
> > is something you'll have to do, no matter what. But it leading to
> > having to wait for each pending fence, I'm not too sure. What about the
> > case where jobs/ops further away in the HWRing were not even considered
> > for execution by the HW, because the STOP operation prevented them from
> > being dequeued. I'd expect that the only event we'll get for those is
> > "HW queue is properly stopped now". So at this point it's a matter of
> > signalling everything that's left, no? I mean, that's basically what
> > Panthor does:
> > 
> > 1. it stops
> > 2. wait for all executing ops to land (with all the cache maintenance,
> > etc, you described)
> > 3. signal(ECANCELED) what's left (things not picked by the HW by
> > the time the STOP was effective).
> > 
> > It's currently done manually, but does it have to?
> 
> All this being said, I'm also a pragmatic guy, so if you tell us "no
> way!" even after these arguments, I'd rather give up on this
> auto-signaling feature and have rust drivers be forced to manually
> signal stuff than have the whole Fence abstraction blocked. We can add
> a warn_on!(!is_signaled()) in the DriverFence::drop() path for the time
> being, so we can at least catch cases where the driver didn't signal
> the fence before dropping the signal-able object.


In past discussions with my team members we were also not very
determined whether we want autosignal or not.

The only thing I'm concerned about is the RCU vs. UAF feature. We
already invested a lot of time and pain into that feature, so we most
probably want it.


P.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 06 Mar 2026 20:02:58 +0100
Philipp Stanner <phasta@mailbox.org> wrote:

> On Fri, 2026-03-06 at 16:43 +0100, Boris Brezillon wrote:
> > On Fri, 6 Mar 2026 16:25:25 +0100
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > On Fri, 6 Mar 2026 15:37:31 +0100
> > > Christian König <christian.koenig@amd.com> wrote:
> > >   
> > > > On 3/6/26 14:36, Philipp Stanner wrote:    
> > > > > > > > > In other words not signaling a fence can leave the system in a
> > > > > > > > > deadlock state, but signaling it incorrectly usually results in
> > > > > > > > > random data corruption.      
> > > > > > > 
> > > > > > > Well, not signaling it results in a potential deadlock of the
> > > > > > > whole kernel, whereas wrongly signaling it is "only" a functional
> > > > > > > bug.      
> > > > > > 
> > > > > > No, that results in random memory corruption. Which is easily a
> > > > > > magnitude worse than just a kernel deadlock.
> > > > > > 
> > > > > > When have seen such bugs numerous times with suspend and resume on
> > > > > > laptops in different subsystems, e.g. not only GPU. And I'm
> > > > > > absolutely clearly rejecting any attempt to signal DMA fences when
> > > > > > an object runs out of scope because of that experience.      
> > > > > 
> > > > > But you're aware that both in C and Rust you could experience UAF
> > > > > bugs if fences drop unsignaled and the driver unloads?
> > > > > 
> > > > > Though I tentatively agree that memory corruptions on a large scale
> > > > > are probably the worst error you can have on a computer.      
> > > > 
> > > > Yeah, of course I'm aware of the UAF issue we have.
> > > > 
> > > > But those are relatively harmless compared to the random memory
> > > > corruption issues.
> > > > 
> > > > Linux has the unfortunate habit of re-using memory directly after
> > > > freeing it because that means caches are usually hotter.
> > > > 
> > > > So rough DMA operations have the tendency to end up anywhere and
> > > > tools like KASAN can't find anything wrong.
> > > > 
> > > > The only protection you sometimes have is IOMMU, but that is usually
> > > > not able to catch everything either.
> > > >     
> > > > > >      
> > > > > > > > It all stands and falls with the question whether a fence can
> > > > > > > > drop by accident in Rust, or if it will only ever drop when the
> > > > > > > > hw-ring is closed.
> > > > > > > > 
> > > > > > > > What do you believe is the right thing to do when a driver
> > > > > > > > unloads?      
> > > > > > > 
> > > > > > > The fence has to be signaled -- ideally after shutting down all
> > > > > > > queues, but it has to be signaled.      
> > > > > > 
> > > > > > Yeah well this shutting down all queues (and making sure that no
> > > > > > write operation is pending in caches etc...) is what people
> > > > > > usually don't get right.
> > > > > > 
> > > > > > What you can to is things like setting your timeout to zero and
> > > > > > immediately causing terminating the HW operation and resetting the
> > > > > > device.
> > > > > > 
> > > > > > This will then use the same code path as the mandatory timeout
> > > > > > functionality for DMA operations and that usually works reliable.      
> > > > > 
> > > > > Why is all that even an issue? The driver controls the hardware and
> > > > > must "switch it off" before it unloads. Then the hardware will not
> > > > > access any memory anymore for sure.      
> > > > 
> > > > Well exactly that is usually really complicated. Let me try to
> > > > explain that on a HW example. 
> > > > 
> > > > Between a shader and the actual system memory you usually have
> > > > different IP blocks or stages where a memory access needs to go
> > > > through:
> > > > 
> > > > Shader -> device VM -> device cache -> PCI bus -> CPU cache -> memory
> > > > 
> > > > Now when you want to terminate some shader or make some memory
> > > > inaccessible because it is freed drivers update their page tables and
> > > > issue the equivalent of TLB invalidation on a CPU.
> > > > 
> > > > The problem is now that this will only invalidate the translation in
> > > > the device VM. It doesn't affect the device cache nor any ongoing
> > > > memory transaction on the bus which waits to snoop the CPU cache.
> > > > 
> > > > To make sure that you don't corrupt system memory you actually need
> > > > to wait for a cache flush event to be signaled and *not* just update
> > > > the VM page tables and tell the HW to invalidate it's TLB.
> > > > 
> > > > So what is needed is usually a fence operation. In other words a
> > > > memory value written over the PCIe bus into system memory. Background
> > > > is that memory writes are ordered and this one comes after all
> > > > previous PCIe memory writes of the device and so is in the correct
> > > > order.
> > > > 
> > > > Only when the CPU sees this memory write you can be sure that your
> > > > operation is completed.
> > > > 
> > > > This memory write is then often implemented by using an MSI interrupt
> > > > which in turn signals the DMA fence.
> > > > 
> > > > 
> > > > So the right thing to do is to wait for the DMA fence to signal
> > > > through its normal signaling path which includes both HW and SW
> > > > functionality and *not* just tell the HW to stop some ring and then
> > > > just assume that this is also sufficient to signal the DMA fence
> > > > associated with the HW operation.    
> > > 
> > > Ultimately this
> > > "stop-HW-and-make-sure-all-outcomes-are-visible-even-for-partially-executed-jobs"
> > > is something you'll have to do, no matter what. But it leading to
> > > having to wait for each pending fence, I'm not too sure. What about the
> > > case where jobs/ops further away in the HWRing were not even considered
> > > for execution by the HW, because the STOP operation prevented them from
> > > being dequeued. I'd expect that the only event we'll get for those is
> > > "HW queue is properly stopped now". So at this point it's a matter of
> > > signalling everything that's left, no? I mean, that's basically what
> > > Panthor does:
> > > 
> > > 1. it stops
> > > 2. wait for all executing ops to land (with all the cache maintenance,
> > > etc, you described)
> > > 3. signal(ECANCELED) what's left (things not picked by the HW by
> > > the time the STOP was effective).
> > > 
> > > It's currently done manually, but does it have to?  
> > 
> > All this being said, I'm also a pragmatic guy, so if you tell us "no
> > way!" even after these arguments, I'd rather give up on this
> > auto-signaling feature and have rust drivers be forced to manually
> > signal stuff than have the whole Fence abstraction blocked. We can add
> > a warn_on!(!is_signaled()) in the DriverFence::drop() path for the time
> > being, so we can at least catch cases where the driver didn't signal
> > the fence before dropping the signal-able object.  
> 
> 
> In past discussions with my team members we were also not very
> determined whether we want autosignal or not.
> 
> The only thing I'm concerned about is the RCU vs. UAF feature. We
> already invested a lot of time and pain into that feature, so we most
> probably want it.

Right, there's this UAF situation too. I guess if auto-signal is not
an option, we could add an _ORPHAN_BIT (or _DEAD_BIT) flag, and have
it tested along the _SIGNALED_BIT one in paths where we check if
dma_fence::ops are usable.

Re: dma_fence: force users to take the lock manually

[Christian König]

On 3/9/26 09:16, Boris Brezillon wrote:
> On Fri, 06 Mar 2026 20:02:58 +0100
> Philipp Stanner <phasta@mailbox.org> wrote:
> 
>> On Fri, 2026-03-06 at 16:43 +0100, Boris Brezillon wrote:
>>> On Fri, 6 Mar 2026 16:25:25 +0100
>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>   
>>>> On Fri, 6 Mar 2026 15:37:31 +0100
>>>> Christian König <christian.koenig@amd.com> wrote:
>>>>   
>>>>> On 3/6/26 14:36, Philipp Stanner wrote:    
>>>>>>>>>> In other words not signaling a fence can leave the system in a
>>>>>>>>>> deadlock state, but signaling it incorrectly usually results in
>>>>>>>>>> random data corruption.      
>>>>>>>>
>>>>>>>> Well, not signaling it results in a potential deadlock of the
>>>>>>>> whole kernel, whereas wrongly signaling it is "only" a functional
>>>>>>>> bug.      
>>>>>>>
>>>>>>> No, that results in random memory corruption. Which is easily a
>>>>>>> magnitude worse than just a kernel deadlock.
>>>>>>>
>>>>>>> When have seen such bugs numerous times with suspend and resume on
>>>>>>> laptops in different subsystems, e.g. not only GPU. And I'm
>>>>>>> absolutely clearly rejecting any attempt to signal DMA fences when
>>>>>>> an object runs out of scope because of that experience.      
>>>>>>
>>>>>> But you're aware that both in C and Rust you could experience UAF
>>>>>> bugs if fences drop unsignaled and the driver unloads?
>>>>>>
>>>>>> Though I tentatively agree that memory corruptions on a large scale
>>>>>> are probably the worst error you can have on a computer.      
>>>>>
>>>>> Yeah, of course I'm aware of the UAF issue we have.
>>>>>
>>>>> But those are relatively harmless compared to the random memory
>>>>> corruption issues.
>>>>>
>>>>> Linux has the unfortunate habit of re-using memory directly after
>>>>> freeing it because that means caches are usually hotter.
>>>>>
>>>>> So rough DMA operations have the tendency to end up anywhere and
>>>>> tools like KASAN can't find anything wrong.
>>>>>
>>>>> The only protection you sometimes have is IOMMU, but that is usually
>>>>> not able to catch everything either.
>>>>>     
>>>>>>>      
>>>>>>>>> It all stands and falls with the question whether a fence can
>>>>>>>>> drop by accident in Rust, or if it will only ever drop when the
>>>>>>>>> hw-ring is closed.
>>>>>>>>>
>>>>>>>>> What do you believe is the right thing to do when a driver
>>>>>>>>> unloads?      
>>>>>>>>
>>>>>>>> The fence has to be signaled -- ideally after shutting down all
>>>>>>>> queues, but it has to be signaled.      
>>>>>>>
>>>>>>> Yeah well this shutting down all queues (and making sure that no
>>>>>>> write operation is pending in caches etc...) is what people
>>>>>>> usually don't get right.
>>>>>>>
>>>>>>> What you can to is things like setting your timeout to zero and
>>>>>>> immediately causing terminating the HW operation and resetting the
>>>>>>> device.
>>>>>>>
>>>>>>> This will then use the same code path as the mandatory timeout
>>>>>>> functionality for DMA operations and that usually works reliable.      
>>>>>>
>>>>>> Why is all that even an issue? The driver controls the hardware and
>>>>>> must "switch it off" before it unloads. Then the hardware will not
>>>>>> access any memory anymore for sure.      
>>>>>
>>>>> Well exactly that is usually really complicated. Let me try to
>>>>> explain that on a HW example. 
>>>>>
>>>>> Between a shader and the actual system memory you usually have
>>>>> different IP blocks or stages where a memory access needs to go
>>>>> through:
>>>>>
>>>>> Shader -> device VM -> device cache -> PCI bus -> CPU cache -> memory
>>>>>
>>>>> Now when you want to terminate some shader or make some memory
>>>>> inaccessible because it is freed drivers update their page tables and
>>>>> issue the equivalent of TLB invalidation on a CPU.
>>>>>
>>>>> The problem is now that this will only invalidate the translation in
>>>>> the device VM. It doesn't affect the device cache nor any ongoing
>>>>> memory transaction on the bus which waits to snoop the CPU cache.
>>>>>
>>>>> To make sure that you don't corrupt system memory you actually need
>>>>> to wait for a cache flush event to be signaled and *not* just update
>>>>> the VM page tables and tell the HW to invalidate it's TLB.
>>>>>
>>>>> So what is needed is usually a fence operation. In other words a
>>>>> memory value written over the PCIe bus into system memory. Background
>>>>> is that memory writes are ordered and this one comes after all
>>>>> previous PCIe memory writes of the device and so is in the correct
>>>>> order.
>>>>>
>>>>> Only when the CPU sees this memory write you can be sure that your
>>>>> operation is completed.
>>>>>
>>>>> This memory write is then often implemented by using an MSI interrupt
>>>>> which in turn signals the DMA fence.
>>>>>
>>>>>
>>>>> So the right thing to do is to wait for the DMA fence to signal
>>>>> through its normal signaling path which includes both HW and SW
>>>>> functionality and *not* just tell the HW to stop some ring and then
>>>>> just assume that this is also sufficient to signal the DMA fence
>>>>> associated with the HW operation.    
>>>>
>>>> Ultimately this
>>>> "stop-HW-and-make-sure-all-outcomes-are-visible-even-for-partially-executed-jobs"
>>>> is something you'll have to do, no matter what. But it leading to
>>>> having to wait for each pending fence, I'm not too sure. What about the
>>>> case where jobs/ops further away in the HWRing were not even considered
>>>> for execution by the HW, because the STOP operation prevented them from
>>>> being dequeued. I'd expect that the only event we'll get for those is
>>>> "HW queue is properly stopped now". So at this point it's a matter of
>>>> signalling everything that's left, no? I mean, that's basically what
>>>> Panthor does:
>>>>
>>>> 1. it stops
>>>> 2. wait for all executing ops to land (with all the cache maintenance,
>>>> etc, you described)
>>>> 3. signal(ECANCELED) what's left (things not picked by the HW by
>>>> the time the STOP was effective).
>>>>
>>>> It's currently done manually, but does it have to?  
>>>
>>> All this being said, I'm also a pragmatic guy, so if you tell us "no
>>> way!" even after these arguments, I'd rather give up on this
>>> auto-signaling feature and have rust drivers be forced to manually
>>> signal stuff than have the whole Fence abstraction blocked. We can add
>>> a warn_on!(!is_signaled()) in the DriverFence::drop() path for the time
>>> being, so we can at least catch cases where the driver didn't signal
>>> the fence before dropping the signal-able object.  
>>
>>
>> In past discussions with my team members we were also not very
>> determined whether we want autosignal or not.
>>
>> The only thing I'm concerned about is the RCU vs. UAF feature. We
>> already invested a lot of time and pain into that feature, so we most
>> probably want it.
> 
> Right, there's this UAF situation too. I guess if auto-signal is not
> an option, we could add an _ORPHAN_BIT (or _DEAD_BIT) flag, and have
> it tested along the _SIGNALED_BIT one in paths where we check if
> dma_fence::ops are usable.

You mean to protect driver unload? Yeah that's a pretty good point.

But I think I have an idea how to tackle that sanely even on the C side.

Give me a day to go over the rest of my mails and hack something together. I have another DMA-fence cleanup I wanted to run by Tvrtko and you anyway.

Regards,
Christian.

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 06 Mar 2026 13:36:48 +0100
Philipp Stanner <phasta@mailbox.org> wrote:

> On Fri, 2026-03-06 at 13:31 +0100, Christian König wrote:
> > On 3/6/26 12:57, Philipp Stanner wrote:  
> > > On Fri, 2026-03-06 at 12:24 +0100, Boris Brezillon wrote:  
> > > > On Fri, 6 Mar 2026 12:03:19 +0100
> > > > Christian König <christian.koenig@amd.com> wrote:
> > > >   
> > > > > On 3/6/26 11:37, Boris Brezillon wrote:  
> > > > > > On Fri, 6 Mar 2026 10:58:07 +0100
> > > > > > Christian König <christian.koenig@amd.com> wrote:
> > > > > >     
> > > > > > > On 3/6/26 10:46, Boris Brezillon wrote:    
> > > > > > > > On Fri, 6 Mar 2026 09:10:52 +0100
> > > > > > > > Christian König <christian.koenig@amd.com> wrote:      
> > > > > > > > > Well as I wrote above you either have super reliable locking in
> > > > > > > > > your signaling path or you will need that for error handling.      
> > > > > > > > 
> > > > > > > > Not really. With rust's ownership model, you can make it so only
> > > > > > > > one thread gets to own the DriverFence (the signal-able fence
> > > > > > > > object), and the DriverFence::signal() method consumes this
> > > > > > > > object. This implies that only one path gets to signal the
> > > > > > > > DriverFence, and after that it vanishes, so no one else can
> > > > > > > > signal it anymore. Just to clarify, by vanishes, I mean that the
> > > > > > > > signal-able view disappears, but the observable object (Fence)
> > > > > > > > can stay around, so it can be monitored (and only monitored) by
> > > > > > > > others. With this model, it doesn't matter that _set_error() is
> > > > > > > > set under a dma_fence locked section or not, because the
> > > > > > > > concurrency is addressed at a higher level.      
> > > > > > > 
> > > > > > > That whole approach won't work. You have at least the IRQ handler
> > > > > > > which signals completion and the timeout handler which signals
> > > > > > > completion with an error.    
> > > > > > 
> > > > > > From a pure rust standpoint, and assuming both path (IRQ handler and
> > > > > > timeout handler) are written in rust, the compiler won't let you
> > > > > > signal concurrently if we design the thing properly, that's what
> > > > > > I'm trying to say. Just to be clear, it doesn't mean you can't have
> > > > > > one worker (in a workqueue context) that can signal a fence and an
> > > > > > IRQ handler that can signal the same fence. It just means that rust
> > > > > > won't let you do that unless you have proper locking in place, and
> > > > > > rust will also guarantee you won't be able to signal a fence that
> > > > > > has already been signaled, because as soon as it's signaled, the
> > > > > > signal-able fence should be consumed.    
> > > > > 
> > > > > Ah got it! I've worked a lot with OCaml in the past which has some
> > > > > similarities, but doesn't push things that far.
> > > > >   
> > > > > > > 
> > > > > > > We have documented that this handling is mandatory for DMA-fences
> > > > > > > since so many driver implementations got it wrong.    
> > > > > > 
> > > > > > Again, I'm just talking about the rust implementation we're aiming
> > > > > > for. If you start mixing C and rust in the same driver, you're back
> > > > > > to the original problem you described.    
> > > > > 
> > > > > The key point is the Rust implementation should not repeat the
> > > > > mistakes we made in the C implementation.
> > > > > 
> > > > > For example blocking that multiple threads can't signal a DMA-fence
> > > > > is completely irrelevant.  
> > > > 
> > > > From a correctness standpoint, I think it's important to ensure no more
> > > > than one thread gets to signal the object.  
> > > 
> > > If you have two paths that can signal a fence, that will result
> > > effectively in you in Rust having to use yet another lock for a fence,
> > > and likely some mechanism for revoking the access.
> > > 
> > > I would at least consider whether it isn't much easier to have the
> > > signalling-function ignore multiple signal attempts.
> > > 
> > > AFAIU in Rust we originaly ended up at signal() consuming the fence
> > > because of the code UAF problem with data: T.  
> > 
> > +1
> >   
> > > > > 
> > > > > What we need to guarantee is correct timeout handling and that
> > > > > DMA-fence can only signal from something delivered from a HW event,
> > > > > e.g. a HW interrupt or interrupt worker or similar.  
> > > > 
> > > > We've mostly focused on coming up with a solution that would annotate
> > > > signaling paths in an automated way, and making sure dma_fence_signal()
> > > > is never called outside of a non-annotated path:
> > > > - creation of DmaFenceWorkqueue/DmaFence[Delayed]Work that guarantees
> > > >   all works are executed in a dma_fence_signalling_{begin,end}()
> > > >   section, so we can properly detect deadlocks (through lockdep)
> > > > - creation of a DmaFenceIrqHandler for the same reason
> > > > - we'll need variants for each new deferred mechanism drivers might
> > > >   want to use (kthread_worker?)
> > > > 
> > > > But there's currently no restriction on calling dma_fence_signal() in a
> > > > user thread context (IOCTL()). I guess that shouldn't be too hard to
> > > > add (is_user_task() to the rescue).
> > > >   
> > > > > 
> > > > > A DMA-fence should *never* signal because of an IOCTL  
> > > > 
> > > > Okay, that's understandable.
> > > >   
> > > > > or because some
> > > > > object runs out of scope. E.g. when you cleanup a HW ring buffer, FW
> > > > > queue, etc...  
> > > > 
> > > > We were actually going in the opposite direction:
> > > > auto-signal(ECANCELED) on DriverFenceTimeline object destruction  
> > 
> > Absolutely clear NAK to that, we have iterated that many times before on the C side as well.
> > 
> > See below for the explanation of the background.
> >   
> > > > (which
> > > > is the thing that would be attached to the HW ringbuf. The reason is:
> > > > we don't want to leave unsignalled fences behind,
> > > >   
> > > 
> > > Not only do we not "want to", we actually *cannot*. We have to make
> > > sure all fences are signaled because only this way the C backend plus
> > > RCU can protect also the Rust code against UAF.
> > >   
> > > >  and if the HW ring is
> > > > gone, there's nothing that can signal it. Mind explaining why you think
> > > > this shouldn't be done, because I originally interpreted your
> > > > suggestion as exactly the opposite.  
> > > 
> > > I also don't get it. All fences must always get signaled, that's one of
> > > the most fundamental fence rules. Thus, if the last accessor to a fence
> > > drops, you do want to signal it with -ECANCELED  
> > 
> > All fences must always signal because the HW operation must always complete or be terminated by a timeout.
> > 
> > If a fence signals only because it runs out of scope than that means that you have a huge potential for data corruption and that is even worse than not signaling a fence.
> > 
> > In other words not signaling a fence can leave the system in a deadlock state, but signaling it incorrectly usually results in random data corruption.

Forcing a manual signal doesn't really solve the problem, does it? I
mean, you can hand-roll your "signal(ECANCELED) all fences on this HW
ring on HW ring desctruction" (you'll have to if it's not managed by
something else), but how safer is it than providing a
DriverDmaFenceTimeline (or DriverDmaFenceContext, call it what you
want) that manages that for you, and then have this object attached to
your HW ring, and on HW ring drop, you get DriverDmaFenceContext
dropped too, and the associated auto-signal(ECANCELED) called.

Ultimately, the UAF you're referring to still exists, and I agree it's
worse than a deadlock, but it's also not something we're magically
immune to just because we don't signal on ::drop().

If you think that's preferable, we can have that done in some
::signal_all() method that has to be explicitly called when the HW
ring is destroyed, but that's basically the same problem: you have no
guarantee that no other paths will call that while the HW is still
active...

> 
> It all stands and falls with the question whether a fence can drop by
> accident in Rust, or if it will only ever drop when the hw-ring is
> closed.

Let's say it's less likely to happen, not impossible. And if it does
happen, it means the user really wanted that to happen (thinking of
ManuallyDrop for instance).

Re: dma_fence: force users to take the lock manually

[Boris Brezillon]

On Fri, 06 Mar 2026 12:57:00 +0100
Philipp Stanner <phasta@mailbox.org> wrote:

> On Fri, 2026-03-06 at 12:24 +0100, Boris Brezillon wrote:
> > On Fri, 6 Mar 2026 12:03:19 +0100
> > Christian König <christian.koenig@amd.com> wrote:
> >   
> > > On 3/6/26 11:37, Boris Brezillon wrote:  
> > > > On Fri, 6 Mar 2026 10:58:07 +0100
> > > > Christian König <christian.koenig@amd.com> wrote:
> > > >     
> > > > > On 3/6/26 10:46, Boris Brezillon wrote:    
> > > > > > On Fri, 6 Mar 2026 09:10:52 +0100
> > > > > > Christian König <christian.koenig@amd.com> wrote:      
> > > > > > > Well as I wrote above you either have super reliable locking in
> > > > > > > your signaling path or you will need that for error handling.      
> > > > > > 
> > > > > > Not really. With rust's ownership model, you can make it so only
> > > > > > one thread gets to own the DriverFence (the signal-able fence
> > > > > > object), and the DriverFence::signal() method consumes this
> > > > > > object. This implies that only one path gets to signal the
> > > > > > DriverFence, and after that it vanishes, so no one else can
> > > > > > signal it anymore. Just to clarify, by vanishes, I mean that the
> > > > > > signal-able view disappears, but the observable object (Fence)
> > > > > > can stay around, so it can be monitored (and only monitored) by
> > > > > > others. With this model, it doesn't matter that _set_error() is
> > > > > > set under a dma_fence locked section or not, because the
> > > > > > concurrency is addressed at a higher level.      
> > > > > 
> > > > > That whole approach won't work. You have at least the IRQ handler
> > > > > which signals completion and the timeout handler which signals
> > > > > completion with an error.    
> > > > 
> > > > From a pure rust standpoint, and assuming both path (IRQ handler and
> > > > timeout handler) are written in rust, the compiler won't let you
> > > > signal concurrently if we design the thing properly, that's what
> > > > I'm trying to say. Just to be clear, it doesn't mean you can't have
> > > > one worker (in a workqueue context) that can signal a fence and an
> > > > IRQ handler that can signal the same fence. It just means that rust
> > > > won't let you do that unless you have proper locking in place, and
> > > > rust will also guarantee you won't be able to signal a fence that
> > > > has already been signaled, because as soon as it's signaled, the
> > > > signal-able fence should be consumed.    
> > > 
> > > Ah got it! I've worked a lot with OCaml in the past which has some
> > > similarities, but doesn't push things that far.
> > >   
> > > > > 
> > > > > We have documented that this handling is mandatory for DMA-fences
> > > > > since so many driver implementations got it wrong.    
> > > > 
> > > > Again, I'm just talking about the rust implementation we're aiming
> > > > for. If you start mixing C and rust in the same driver, you're back
> > > > to the original problem you described.    
> > > 
> > > The key point is the Rust implementation should not repeat the
> > > mistakes we made in the C implementation.
> > > 
> > > For example blocking that multiple threads can't signal a DMA-fence
> > > is completely irrelevant.  
> > 
> > From a correctness standpoint, I think it's important to ensure no more
> > than one thread gets to signal the object.  
> 
> If you have two paths that can signal a fence, that will result
> effectively in you in Rust having to use yet another lock for a fence,
> and likely some mechanism for revoking the access.
> 
> I would at least consider whether it isn't much easier to have the
> signalling-function ignore multiple signal attempts.
> 
> AFAIU in Rust we originaly ended up at signal() consuming the fence
> because of the code UAF problem with data: T.

That's true, but it does prevent the multiple _signal() call on the same
fence at the same time. Anyway, we're running in circle here. Silently
ignoring when _signal() is called on an already signaled fence is
perfectly fine.

> 
> >   
> > > 
> > > What we need to guarantee is correct timeout handling and that
> > > DMA-fence can only signal from something delivered from a HW event,
> > > e.g. a HW interrupt or interrupt worker or similar.  
> > 
> > We've mostly focused on coming up with a solution that would annotate
> > signaling paths in an automated way, and making sure dma_fence_signal()
> > is never called outside of a non-annotated path:
> > - creation of DmaFenceWorkqueue/DmaFence[Delayed]Work that guarantees
> >   all works are executed in a dma_fence_signalling_{begin,end}()
> >   section, so we can properly detect deadlocks (through lockdep)
> > - creation of a DmaFenceIrqHandler for the same reason
> > - we'll need variants for each new deferred mechanism drivers might
> >   want to use (kthread_worker?)
> > 
> > But there's currently no restriction on calling dma_fence_signal() in a
> > user thread context (IOCTL()). I guess that shouldn't be too hard to
> > add (is_user_task() to the rescue).
> >   
> > > 
> > > A DMA-fence should *never* signal because of an IOCTL  
> > 
> > Okay, that's understandable.
> >   
> > > or because some
> > > object runs out of scope. E.g. when you cleanup a HW ring buffer, FW
> > > queue, etc...  
> > 
> > We were actually going in the opposite direction:
> > auto-signal(ECANCELED) on DriverFenceTimeline object destruction (which
> > is the thing that would be attached to the HW ringbuf. The reason is:
> > we don't want to leave unsignalled fences behind,
> >   
> 
> Not only do we not "want to", we actually *cannot*. We have to make
> sure all fences are signaled because only this way the C backend plus
> RCU can protect also the Rust code against UAF.

There's a bit of that, yes. But also, even if we were assuming the
dma_fence_ops and all data associated to the fence which are needed
for any of the fence ops to function correctly were staying around,
ensuring all DriverFences are signaled before being dropped is key if
we want to ensure deadlock-free implementations, otherwise you'll block
observers on a fence that will never be signaled.

> 
> >  and if the HW ring is
> > gone, there's nothing that can signal it. Mind explaining why you think
> > this shouldn't be done, because I originally interpreted your
> > suggestion as exactly the opposite.  
> 
> I also don't get it. All fences must always get signaled, that's one of
> the most fundamental fence rules. Thus, if the last accessor to a fence
> drops, you do want to signal it with -ECANCELED

Yep, I'm pretty much on the same page here.