On 6/22/26 10:57, Philipp Stanner wrote:
> 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.
Yeah that's exactly the reason why I have been rejecting the idea that drivers
install callbacks on their own fences.
Maybe an idea how to generally tackle this for all drivers: Never let the
dma-fence framework call dma_fence_signal(), that should only the driver do by
itself.
This way we don't run into the issue in the first place that the driver needs
to install some callbacks *and* the driver also knows when the installed
callbacks are finished because that is when dma_fence_signal() returns.
Does that sound valid to you guys? If yes I will be sending out patches to do
this.
Regards,
Christian.
>
> 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.
