On Sun, 2026-06-21 at 17:30 +0200, Danilo Krummrich wrote:
> (Cc: Alice, Boris, Christian, Dave, Gary)
>
> On Mon Jun 15, 2026 at 11:26 AM CEST, Tvrtko Ursulin wrote:
> > @@ -176,9 +160,25 @@ nouveau_cli_work(struct work_struct *w)
> > struct nouveau_cli_work *work, *wtmp;
> > mutex_lock(&cli->lock);
> > list_for_each_entry_safe(work, wtmp, &cli->worker, head) {
> > - if (!work->fence || nouveau_cli_work_ready(work->fence)) {
> > + struct dma_fence *fence = work->fence;
> > +
> > + if (!fence || dma_fence_is_signaled(fence)) {
> > + if (fence) {
btw I don't think that these two lines read more beautifully than the
removed nouveau_cli_work_ready().
> > + unsigned long flags;
> > +
> > + /*
> > + * Because fence can still be in the process of
> > + * processing the callback list, and the
> > + * callback references the work we are about to
> > + * free, we need to sync with the callback
> > + * processing before freeing the work.
> > + */
> > + dma_fence_lock_irqsave(fence, flags);
> > + dma_fence_unlock_irqrestore(fence, flags);
>
> This does not ensure that there are no more dma_fence_ops callbacks into the
> driver. For instance, a concurrent dma_fence_is_signaled() might read the
> signaled flag as false before we do this lock dance and then still enter
> ops->signaled().
The issue Dave addressed was a UAF within Nouveau itself, where the
driver put() its own fence while its own callbacks were still
executing.
Tvrtko's patch does not address the RCU issues you mention, nor does it
address any other fence related issue except for adding the comment
that should have been added back then with the fix in
nouveau_cli_work_ready().
It's literally the same. Take the lock manually to work around the fact
that the bit is protected inconsistently, setting is locked, reading
just randomly races around.
>
> Luckily, for this specific code here in nouveau it should be enough to ensure
> that all dma_fence_cb callbacks are finished, which this hack does achieve.
>
> The reason I call it hack is that it fundamentally relies on dma_fence
> implementation details and not a specified API contract.
>
> The closest thing to an API contract for this is the RCU protection that was
> recently added. So, the proper approach would be to run this work with
> queue_rcu_work() and then call flush_rcu_work().
>
> In this context it is worth noting that in this case it is even more
> complicated. Since the work struct is shared per CLI the single RCU barrier in
> flush_rcu_work() is not enough, i.e. there is no per fence callback grace
> period
> guarantee.
>
> To me this still seems to be a bit of a footgun (although we could probably
> improve the nouveau code to make it a bit simpler).
>
> That said, the patch does make things a bit clearer, so I picked it up --
> thanks!
I disagree that this is an improvement and am unhappy with you merging
it. It merely moves Dave's fix from one place to another and causes
unnecessary churn in the git log.
Adding the comment into Dave's fix until someone can address the
(literally) fundemantal problem in dma_fence would have been my
preferred approach.
P.
