[PATCH] drm/nouveau: Simplify nouveau_cli_work

[PATCH] drm/nouveau: Simplify nouveau_cli_work

[Tvrtko Ursulin]

I don't claim this fixes anything but the code certainly was a bit
roundabout and this at least cleans it up.

The only semantic difference is releasing the fence reference after
the work callback is called. Which kind of make sense anyway.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: nouveau@lists.freedesktop.org
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 42a81166f3a9..7de174537fc7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
 		return nouveau_platform_name(to_platform_device(dev->dev));
 }
 
-static inline bool
-nouveau_cli_work_ready(struct dma_fence *fence)
-{
-	unsigned long flags;
-	bool ret = true;
-
-	dma_fence_lock_irqsave(fence, flags);
-	if (!dma_fence_is_signaled_locked(fence))
-		ret = false;
-	dma_fence_unlock_irqrestore(fence, flags);
-
-	if (ret == true)
-		dma_fence_put(fence);
-	return ret;
-}
-
 static void
 nouveau_cli_work(struct work_struct *w)
 {
@@ -176,9 +160,10 @@ 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)) {
+		if (!work->fence || dma_fence_is_signaled(work->fence)) {
 			list_del(&work->head);
 			work->func(work);
+			dma_fence_put(work->fence);
 		}
 	}
 	mutex_unlock(&cli->lock);
-- 
2.54.0

Re: [PATCH] drm/nouveau: Simplify nouveau_cli_work

[Philipp Stanner]

On Fri, 2026-06-12 at 17:54 +0100, Tvrtko Ursulin wrote:
> I don't claim this fixes anything but the code certainly was a bit
> roundabout and this at least cleans it up.
> 
> The only semantic difference is releasing the fence reference after
> the work callback is called. Which kind of make sense anyway.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: nouveau@lists.freedesktop.org
> ---
>  drivers/gpu/drm/nouveau/nouveau_drm.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 42a81166f3a9..7de174537fc7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
>  		return nouveau_platform_name(to_platform_device(dev->dev));
>  }
>  
> -static inline bool
> -nouveau_cli_work_ready(struct dma_fence *fence)
> -{
> -	unsigned long flags;
> -	bool ret = true;
> -
> -	dma_fence_lock_irqsave(fence, flags);
> -	if (!dma_fence_is_signaled_locked(fence))
> -		ret = false;
> -	dma_fence_unlock_irqrestore(fence, flags);
> -
> -	if (ret == true)
> -		dma_fence_put(fence);
> -	return ret;
> -}
> -
>  static void
>  nouveau_cli_work(struct work_struct *w)
>  {
> @@ -176,9 +160,10 @@ 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)) {
> +		if (!work->fence || dma_fence_is_signaled(work->fence)) {
>  			list_del(&work->head);
>  			work->func(work);
> +			dma_fence_put(work->fence);

Dave had addressed a UAF with that patch.

So if this would fix it, then the claim would be that work->func(work)
was the user of that fence, and not some other party elsewhere.

I don't know whether that's the case.


P.

>  		}
>  	}
>  	mutex_unlock(&cli->lock);

Re: [PATCH] drm/nouveau: Simplify nouveau_cli_work

[Tvrtko Ursulin]

On 12/06/2026 18:14, Philipp Stanner wrote:
> On Fri, 2026-06-12 at 17:54 +0100, Tvrtko Ursulin wrote:
>> I don't claim this fixes anything but the code certainly was a bit
>> roundabout and this at least cleans it up.
>>
>> The only semantic difference is releasing the fence reference after
>> the work callback is called. Which kind of make sense anyway.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: nouveau@lists.freedesktop.org
>> ---
>>   drivers/gpu/drm/nouveau/nouveau_drm.c | 19 ++-----------------
>>   1 file changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
>> index 42a81166f3a9..7de174537fc7 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
>> @@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
>>   		return nouveau_platform_name(to_platform_device(dev->dev));
>>   }
>>   
>> -static inline bool
>> -nouveau_cli_work_ready(struct dma_fence *fence)
>> -{
>> -	unsigned long flags;
>> -	bool ret = true;
>> -
>> -	dma_fence_lock_irqsave(fence, flags);
>> -	if (!dma_fence_is_signaled_locked(fence))
>> -		ret = false;
>> -	dma_fence_unlock_irqrestore(fence, flags);
>> -
>> -	if (ret == true)
>> -		dma_fence_put(fence);
>> -	return ret;
>> -}
>> -
>>   static void
>>   nouveau_cli_work(struct work_struct *w)
>>   {
>> @@ -176,9 +160,10 @@ 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)) {
>> +		if (!work->fence || dma_fence_is_signaled(work->fence)) {
>>   			list_del(&work->head);
>>   			work->func(work);
>> +			dma_fence_put(work->fence);
> 
> Dave had addressed a UAF with that patch.
> 
> So if this would fix it, then the claim would be that work->func(work)
> was the user of that fence, and not some other party elsewhere.
> 
> I don't know whether that's the case.

Me neither. I say in the commit I don't claim to fix anything. But it is 
more logical and avoids the silly games with ret and all.

What I missed though is that work->func(work) frees the work.

So patch needs to cache the fence... v2 incoming..

Regards,

Tvrtko

> 
> 
> P.
> 
>>   		}
>>   	}
>>   	mutex_unlock(&cli->lock);

Re: [PATCH] drm/nouveau: Simplify nouveau_cli_work

[Tvrtko Ursulin]

On 12/06/2026 18:18, Tvrtko Ursulin wrote:
> 
> On 12/06/2026 18:14, Philipp Stanner wrote:
>> On Fri, 2026-06-12 at 17:54 +0100, Tvrtko Ursulin wrote:
>>> I don't claim this fixes anything but the code certainly was a bit
>>> roundabout and this at least cleans it up.
>>>
>>> The only semantic difference is releasing the fence reference after
>>> the work callback is called. Which kind of make sense anyway.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Philipp Stanner <phasta@kernel.org>
>>> Cc: Lyude Paul <lyude@redhat.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: nouveau@lists.freedesktop.org
>>> ---
>>>   drivers/gpu/drm/nouveau/nouveau_drm.c | 19 ++-----------------
>>>   1 file changed, 2 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/ 
>>> nouveau/nouveau_drm.c
>>> index 42a81166f3a9..7de174537fc7 100644
>>> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
>>> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
>>> @@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
>>>           return nouveau_platform_name(to_platform_device(dev->dev));
>>>   }
>>> -static inline bool
>>> -nouveau_cli_work_ready(struct dma_fence *fence)
>>> -{
>>> -    unsigned long flags;
>>> -    bool ret = true;
>>> -
>>> -    dma_fence_lock_irqsave(fence, flags);
>>> -    if (!dma_fence_is_signaled_locked(fence))
>>> -        ret = false;
>>> -    dma_fence_unlock_irqrestore(fence, flags);
>>> -
>>> -    if (ret == true)
>>> -        dma_fence_put(fence);
>>> -    return ret;
>>> -}
>>> -
>>>   static void
>>>   nouveau_cli_work(struct work_struct *w)
>>>   {
>>> @@ -176,9 +160,10 @@ 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)) {
>>> +        if (!work->fence || dma_fence_is_signaled(work->fence)) {
>>>               list_del(&work->head);
>>>               work->func(work);
>>> +            dma_fence_put(work->fence);
>>
>> Dave had addressed a UAF with that patch.
>>
>> So if this would fix it, then the claim would be that work->func(work)
>> was the user of that fence, and not some other party elsewhere.
>>
>> I don't know whether that's the case.
> 
> Me neither. I say in the commit I don't claim to fix anything. But it is 
> more logical and avoids the silly games with ret and all.
> 
> What I missed though is that work->func(work) frees the work.
> 
> So patch needs to cache the fence... v2 incoming..

Ah sorry and I forgot the lock cycling hack we discussed.. ie. the whole 
point of the race. Nevermind then, confusing.

Regards,

Tvrtko

[PATCH v2] drm/nouveau: Simplify nouveau_cli_work

[Tvrtko Ursulin]

I don't claim this fixes anything but the code certainly was a bit
roundabout and this at least cleans it up.

The only semantic difference is releasing the fence reference after
the work callback is called. Which kind of make sense anyway.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: nouveau@lists.freedesktop.org
---
v2:
 * Don't dereference work after work->func(work) frees it.
---
Not even compile tested!
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 42a81166f3a9..e81ec261689a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
 		return nouveau_platform_name(to_platform_device(dev->dev));
 }
 
-static inline bool
-nouveau_cli_work_ready(struct dma_fence *fence)
-{
-	unsigned long flags;
-	bool ret = true;
-
-	dma_fence_lock_irqsave(fence, flags);
-	if (!dma_fence_is_signaled_locked(fence))
-		ret = false;
-	dma_fence_unlock_irqrestore(fence, flags);
-
-	if (ret == true)
-		dma_fence_put(fence);
-	return ret;
-}
-
 static void
 nouveau_cli_work(struct work_struct *w)
 {
@@ -176,9 +160,12 @@ 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)) {
 			list_del(&work->head);
 			work->func(work);
+			dma_fence_put(fence);
 		}
 	}
 	mutex_unlock(&cli->lock);
-- 
2.54.0

[PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Tvrtko Ursulin]

Lets simplify and make clearer the fence lock cycling workaround in
nouveau_cli_work().

The reason for the workaround is that the worker processes the list of
pending work items (and so fence callbacks) opportunisticaly, while
dma_fence_is_signaled() returns true as soon as a the fence is signaled.
It is therefore not allowed for the opportunistic processing to free the
work item, since the fence callback processing can still be in the process
of dereferencing work->cb.

This wasn't very clear in the code so clarify it with a nice comment.

In the process we replace the somewhat interleaved and not very readable
nouveau_cli_work_ready() helper with a direct dma_fence_is_signaled, which
is even faster since it removes the lock cycling from the unsignaled fence
path.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: nouveau@lists.freedesktop.org
---
v2:
 * Don't dereference work after work->func(work) frees it.

v3:
 * Put back and explain lock cycling.
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 34 +++++++++++++--------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 42a81166f3a9..7ea95ab960a0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
 		return nouveau_platform_name(to_platform_device(dev->dev));
 }
 
-static inline bool
-nouveau_cli_work_ready(struct dma_fence *fence)
-{
-	unsigned long flags;
-	bool ret = true;
-
-	dma_fence_lock_irqsave(fence, flags);
-	if (!dma_fence_is_signaled_locked(fence))
-		ret = false;
-	dma_fence_unlock_irqrestore(fence, flags);
-
-	if (ret == true)
-		dma_fence_put(fence);
-	return ret;
-}
-
 static void
 nouveau_cli_work(struct work_struct *w)
 {
@@ -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) {
+				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);
+			}
 			list_del(&work->head);
 			work->func(work);
+			dma_fence_put(fence);
 		}
 	}
 	mutex_unlock(&cli->lock);
-- 
2.54.0

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Philipp Stanner]

On Mon, 2026-06-15 at 10:26 +0100, Tvrtko Ursulin wrote:
> Lets simplify and make clearer the fence lock cycling workaround in
> nouveau_cli_work().
> 
> The reason for the workaround is that the worker processes the list of
> pending work items (and so fence callbacks) opportunisticaly, while
> dma_fence_is_signaled() returns true as soon as a the fence is signaled.
> It is therefore not allowed for the opportunistic processing to free the
> work item, since the fence callback processing can still be in the process
> of dereferencing work->cb.
> 
> This wasn't very clear in the code so clarify it with a nice comment.
> 
> In the process we replace the somewhat interleaved and not very readable
> nouveau_cli_work_ready() helper with a direct dma_fence_is_signaled, which
> is even faster since it removes the lock cycling from the unsignaled fence
> path.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: nouveau@lists.freedesktop.org
> ---
> v2:
>  * Don't dereference work after work->func(work) frees it.
> 
> v3:
>  * Put back and explain lock cycling.
> ---
>  drivers/gpu/drm/nouveau/nouveau_drm.c | 34 +++++++++++++--------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 42a81166f3a9..7ea95ab960a0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -153,22 +153,6 @@ nouveau_name(struct drm_device *dev)
>  		return nouveau_platform_name(to_platform_device(dev->dev));
>  }
>  
> -static inline bool
> -nouveau_cli_work_ready(struct dma_fence *fence)
> -{
> -	unsigned long flags;
> -	bool ret = true;
> -
> -	dma_fence_lock_irqsave(fence, flags);
> -	if (!dma_fence_is_signaled_locked(fence))
> -		ret = false;
> -	dma_fence_unlock_irqrestore(fence, flags);
> -
> -	if (ret == true)
> -		dma_fence_put(fence);
> -	return ret;
> -}
> -
>  static void
>  nouveau_cli_work(struct work_struct *w)
>  {
> @@ -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) {
> +				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);
> +			}
>  			list_del(&work->head);
>  			work->func(work);
> +			dma_fence_put(fence);

This is semantically exactly the same fix that was introduced by Dave
in c8a5d5ea3ba6a and which you are reverting here again, basically.

It's no improvement IMO, just a differently looking work around the
fact that the dma_fence framework is not properly synchronized.

I suggest you participate here [1] and here [2] and share your opinion.
At least *this* problem would disappear of only dma_fence_is_signaled()
were locking properly.


P.


[1] https://lore.kernel.org/dri-devel/fa0dc9757bf8343516c4b156a2b70ec91b64ef8f.camel@mailbox.org/
[2] https://lore.kernel.org/dri-devel/20260612104251.2264707-2-phasta@kernel.org/

>  		}
>  	}
>  	mutex_unlock(&cli->lock);

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Danilo Krummrich]

On Mon, 15 Jun 2026 10:26:07 +0100, Tvrtko Ursulin wrote:
> [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

Applied, thanks!

  Branch: drm-misc-next
  Tree:   https://gitlab.freedesktop.org/drm/misc/kernel.git

[1/1] drm/nouveau: Simplify nouveau_cli_work
      commit: a80d12eafad1

The patch will appear in the next linux-next integration (typically within 24
hours on weekdays).

The patch is queued up for the upcoming merge window for the next major kernel
release.

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Danilo Krummrich]

(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) {
> +				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().

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!

Thanks,
Danilo

> +			}
>  			list_del(&work->head);
>  			work->func(work);
> +			dma_fence_put(fence);
>  		}
>  	}
>  	mutex_unlock(&cli->lock);
> -- 
> 2.54.0

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Philipp Stanner]

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.

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Tvrtko Ursulin]

On 22/06/2026 09: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.
> 
> 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.

FWIW for me the sandwiched sampling of the signaled status inside a lock 
was quite misleading, when the real problem is nouveau freeing a fence 
callback underneath the dma-fence APIs feet. Hence I think clearly 
splitting the steps is clearer.

Alternative could be having a dma_fence_remove_callback() variant which 
is safe to call for the nouveau use case. Then the sequence could be:

if (dma_fence_is_signaled(work->fence)) {
	dma_fence_remove_callback_safe(work->fence, work->cb);
	kfree(work);
}

(Signatures invented purely for the purpose of illustrating the logic 
flow and freeing the callback gotcha.)

And as an extra bonus the patch removed the pointless lock cycling of 
all the unsignaled fences on the list.

So in my view discussion about the semantics of the signaled status when 
observed from the outside is orthogonal. Ie. whether seeing a signaled 
fence status means fence has been signaled (what we have now), or 
whether also all listeners have been notified (what you argue for).

For the latter one could argue that it is not required to "block" all 
listeners until all have been notified. Let the first one go and carry 
one notifying the rest kind of makes sense.

Regards,

Tvrtko

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

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Christian König]

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.

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Philipp Stanner]

(+Cc Daniel)

On Mon, 2026-06-22 at 14:17 +0200, Christian König wrote:
> On 6/22/26 10:57, Philipp Stanner wrote:
> > 
> > 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.

I remember we had that discussion when I discovered that signaled
fences can temporarily remain on nouveau's "unsignaled fences" list and
wanted to do the removal automatically via callback.

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

About the framework I'm not sure, but what we do enforce strictly in
Rust is that only the creator of a fence can signal it. We achieve that
by two fence types, of which only one implements a signal() method

I remember seeing some parties signaling the GPU scheduler's fences, so
such things definitely happen. And since you likely can't enforce it in
C, you would still have to take the possibility of a third party
signalling your fence into account.

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

Hm. I suppose that would be an improvement?

Maybe let me ask differently, too: why is it that the framework is
signaling fences in the first place? If I understood you correctly last
time that is partially related to the locking problem in the callbacks.


P.

> 
> Regards,
> Christian.

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Danilo Krummrich]

On Mon Jun 22, 2026 at 2:17 PM CEST, Christian König wrote:
> 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.

I think that's a good idea, as it would get us rid of having to rely on API
internals to synchronize struct dma_fence_cb callbacks without doing the full
RCU dance, which can be quite cumbersome (as described in the nouveau case).

It still leaves us with the full RCU dance for struct dma_fence_ops callbacks,
but that's a different topic.

Thanks,
Danilo

Re: [PATCH v3] drm/nouveau: Simplify nouveau_cli_work

[Danilo Krummrich]

On Mon Jun 22, 2026 at 10:57 AM CEST, Philipp Stanner wrote:
> 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().

I described exactly that, with the goal to discuss a proper solution rather than
arguing about which hack is the better one.