Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 4948 bytes --]

On 9/4/26 21:06, Philipp Stanner wrote:
> On Fri, 2026-09-04 at 14:49 +0200, Christian König wrote:
>> On 9/4/26 10:31, Philipp Stanner wrote:
>>> On Fri, 2026-09-04 at 10:20 +0200, Christian König wrote:
>>>>
> 
> […]
> 
>>>>
>>>> That sounds like a bad idea as well.
>>>>
>>>> Dropping the fence->ops is to detach the fence from the module
>>>> which originally issued it and not solve lifetime problems
>>>> between the scheduler and the driver.
>>>
>>> It can be used to solve that problem though, can it not?
>>
>> Yes, but I think forcing dma_fence implementations to drop their
>> release callback to fix lifetime problems with the driver and
>> timeline name functions is a bad idea.
> 
> The issue we're facing is not so much related with life times, is it?
> 
> Even if the driver correctly tears down the scheduler (i.e., signal all
> hardware fences), this problem still exists.

Well no, before patch "035219a760ed dma-buf: dma-fence: Fix potential NULL pointer dereference" everything worked correctly as long as the driver waited for an RCU grace period before tearing down the scheduler.

After this patch we now messed up the lifetime for the scheduler fence get_timeline_name and get_driver_name callbacks.
>>
>> We should keep this fix simple and focused so that we can easily backport it.
> 
> My hope would be that the dma_fence backend solution can be used that
> *already exists*. That could then both be simple and maintainable.
> 
> 
> Correct me if I'm wrong, but it seems we have not found an alternative
> solution that can work yet?

I think we did. The problem was introduced with patch 035219a760ed and I think we should fix it there as well.

We just need to start checking for both the ops and signaled status in the dma_fence framework.
>>
>>>>
>>>> I think we should rather re-consider patch
>>>> 035219a760edb35ae9a9e96beba7f122e26a997b ("dma-buf: dma-fence:
>>>> Fix potential NULL pointer dereference"):
>>>>
>>>> Here we changed the check in dma_fence_driver_name() and
>>>> dma_fence_timeline_name():
>>>>
>>>> @@ -1167,7 +1167,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
>>>>  
>>>>         /* RCU protection is required for safe access to returned string */
>>>>         ops = rcu_dereference(fence->ops);
>>>> -       if (!dma_fence_test_signaled_flag(fence))
>>>> +       if (ops)
>>>>                 return (const char __rcu *)ops->get_driver_name(fence);
>>>>         else
>>>>                 return (const char __rcu *)"detached-driver";
>>>>
>>>> The problem is that we didn't considered that there a fence
>>>> implementations which still have a release or wait callbacks but
>>>> rely on not needing to return a string for a signaled fence.
>>>>
>>>
>>> Could we move the signaled check to amdgpu and pvr?
>>
>> Yes we could. I also considered that. But I would rather like to see
>> it handled in the common dma_fence code.
>>
>> If I remember correctly either Tvrko, you or somebody else was in
>> favor of doing "if (!dma_fence_test_signaled_flag(fence) && ops)"
>> here but I though that this was unnecessary and we would rather
>> remove the release callbacks. Maybe I was wrong with that.
> 
> I hope that wasn't me because that again looks very racy.

Why? Tvrtko added the RCU protection for that.
> I think that there is no way around using the spinlock. As I have
> pointed out many times, the fact that the signaled-bit is set with lock
> protection and read without it is fundamentally broken :(

As far as I can see the RCU approach works just fine, the problem is only that we dropped the check for the signaled bit from the common framework and didn't considered that scheduler fence and a few other weren't changed to not have a release callback yet.

>>> IOW, we keep the solution presented here (removing ops->release for
>>> finished-fence) and the few drivers that check whether a fence is
>>> their
>>> own first do a locked dma_fence_is_signaled() check?
>>
>> Works for me as well, but as I said I would rather like to keep it
>> simple and stupid for backporting.
> 
> If you can think of a stupid and simple solution, shoot. The only thing
> I can think of is moving the string into the dma_fence, as a hard copy
> :)

See attached. It doesn't fully solve the problem, but it gives us the status again we had after Tvrtko's RCU protection work.
> In the mean time, my proposal is to keep aiming for removing
> sched_fence->ops->release and fixing pvr and amdgpu.

Hui? For amdgpu that is completely irrelevant since that driver only destroys the scheduler on driver unload.

And there we always had the problem that when the module is unloaded we pretty much crash the kernel because the ops pointer is still around.

Regards,
Christian.
> I think that will have to entail a signaled-check. Question would then
> be whether we place it in the driver or the backend.
> 
> 
> P.

[-- Attachment #2: 0001-dma-buf-dma-fence-fix-checking-signaling-bit-for-tim.patch --]
[-- Type: text/x-patch, Size: 2301 bytes --]

From e70aebd9106f695179439273be985e589e63f3dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig@amd.com>
Date: Fri, 4 Sep 2026 10:26:13 +0200
Subject: [PATCH 1/2] dma-buf/dma-fence: fix checking signaling bit for
 timeline and driver name
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The patch "dma-buf: dma-fence: Fix potential NULL pointer dereference"
changed the check to test for the ops pointer instead of the signaled
bit to avoid a potential NULL dereference when the ops pointer has been
cleared.

The problem is now that the ops pointer is cleared only when neither the
release nor the wait callback is implemented and this isn't true for a lot
of dma_fence implementations yet. So those implementations lost the RCU
protection after signaling of the returned string resulting in potential
use after free.

Add the signaling check additional to the ops pointer check so that we
have both the protection against NULL dereference as well as the RCU
protection after signaling for the returned string.

Signed-off-by: Christian König <christian.koenig@amd.com>
Fixes: 035219a760ed ("dma-buf: dma-fence: Fix potential NULL pointer dereference")
CC: stable@vger.kernel.org # 7.2+
---
 drivers/dma-buf/dma-fence.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 05090fb0fd5a..e92f9df8d63c 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1170,7 +1170,7 @@ const char __rcu *dma_fence_driver_name(struct dma_fence *fence)
 
 	/* RCU protection is required for safe access to returned string */
 	ops = rcu_dereference(fence->ops);
-	if (ops)
+	if (!dma_fence_test_signaled_flag(fence) && ops)
 		return (const char __rcu *)ops->get_driver_name(fence);
 	else
 		return (const char __rcu *)"detached-driver";
@@ -1203,7 +1203,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
 
 	/* RCU protection is required for safe access to returned string */
 	ops = rcu_dereference(fence->ops);
-	if (ops)
+	if (!dma_fence_test_signaled_flag(fence) && ops)
 		return (const char __rcu *)ops->get_timeline_name(fence);
 	else
 		return (const char __rcu *)"signaled-timeline";
-- 
2.43.0


[-- Attachment #3: 0002-drm-sched-document-the-RCU-dependency.patch --]
[-- Type: text/x-patch, Size: 1719 bytes --]

From b93e52501e8920a881227011212920fa84247639 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig@amd.com>
Date: Mon, 7 Sep 2026 10:59:42 +0200
Subject: [PATCH 2/2] drm/sched: document the RCU dependency
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Tvrkos patches added RCU protection to the returned strings from
get_timeline_name()/get_driver_name() callbacks of the dma_fence
backends.

This fixed use after free problems for a couple of drivers, but we never
documented the consequences for the drm_sched_fence.

Add a few words on the function documentation to note that we need an
RCU grace period between signaling the last scheduler fence and
scheduler teardown.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 6cb6f9546493..22103cb07782 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1203,6 +1203,10 @@ static void drm_sched_cancel_remaining_jobs(struct drm_gpu_scheduler *sched)
  * is implemented, all jobs will be canceled through it and afterwards cleaned
  * up through &struct drm_sched_backend_ops.free_job. If cancel_job is not
  * implemented, memory could leak.
+ *
+ * The scheduler fences timeline name is returned protected by the signaled
+ * status and RCU, so an RCU grace period is necessary between signaling the
+ * last scheduler fence and tearing down the scheduler who originated it.
  */
 void drm_sched_fini(struct drm_gpu_scheduler *sched)
 {
-- 
2.43.0