[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

[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The patch introduces a use-after-free vulnerability by reading `work->fence` after calling `work->func(work)`, which frees the `work` structure.
--

commit 5f500d0384ad49806cf724c1cd7f76a580d6bc42
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/nouveau: Simplify nouveau_cli_work

This patch simplifies the nouveau_cli_work function by removing the
nouveau_cli_work_ready wrapper. As part of this cleanup, the
dma_fence_put() call is moved to execute after the work callback
has been invoked.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index e16f59b00f6fa..a3e38a30fbdab 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -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);

[Severity: Critical]
Does moving dma_fence_put() after work->func() introduce a use-after-free
here?

Looking at the callback implementation in nouveau_gem_object_delete_work()
(in drivers/gpu/drm/nouveau/nouveau_gem.c), the work structure is freed:

nouveau_gem_object_delete_work(...)
{
	...
	nouveau_gem_object_delete(work->vma);
	kfree(work);
}

If the callback frees the work structure, accessing work->fence immediately
afterwards in dma_fence_put() would read from freed memory.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612165409.54447-1-tvrtko.ursulin@igalia.com?part=1

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

[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: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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Replacing the locked fence check (`nouveau_cli_work_ready`) with the lockless `dma_fence_is_signaled()` removes critical synchronization with `dma_fence_signal()`, introducing a race condition that leads to a Use-After-Free of `struct nouveau_cli_work`.
--

commit f14e589461298a55cf7c0a8d50dd778a913dd4fb
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/nouveau: Simplify nouveau_cli_work

This commit simplifies nouveau_cli_work by removing the
nouveau_cli_work_ready helper and replacing it with a direct call to
dma_fence_is_signaled. It also changes the fence reference release to happen
after the work callback is invoked.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index e16f59b00f6fa..a6432eede6137 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -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)) {

[Severity: Critical]
Can this lockless check introduce a race condition leading to a
use-after-free of the work structure?

The original nouveau_cli_work_ready() acquired the fence spinlock before
checking if the fence was signaled, synchronizing with dma_fence_signal().

By using the lockless dma_fence_is_signaled(), this code can observe the
signaled bit while dma_fence_signal() is still iterating over its local
callback list.

If work->func(work) then deallocates the work structure (for instance, via
kfree in nouveau_gem_object_delete_work()), could the signaling CPU
subsequently trigger a use-after-free when it invokes the callback
(nouveau_cli_work_fence)?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612172227.54893-1-tvrtko.ursulin@igalia.com?part=1

[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

[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

[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]

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

[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