[RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Tvrtko Ursulin]

Hi all,

Recently I mentioned to Danilo about some fence lifetime issues so here is a
rough series, more than anything intended to start the discussion.

Most of the problem statement can be found in the first patch but to briefly
summarise - because sched fence can outlive the scheduler, we can trivially
engineer an use after free with xe and possibly other drivers. All that is
needed is to convert a syncobj into a sync file behind drivers back, and I don't
see what the driver can do about it.

IGT that exploits the problem:
https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2

Different flavour of the problem space is if we had a close(drm_fd) in that test
before the sleep. In that case we can even unload xe.ko and gpu-sched.ko for
even more fun. Last two patches in the series close that gap.

But first two patches are just shrinking the race window. They are not proper
fixes. This is what I want to discuss since I understand reference counting all
the involved objects has been rejected in the past. And since the problem
probably expands to all dma fences it certainly isn't easy.

To be clear once more - lets not focus on how this does not fix it fully - I am
primarily trying to start the conversation.

Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

Tvrtko Ursulin (4):
  sync_file: Weakly paper over one use-after-free resulting race
  dma-fence: Slightly safer dma_fence_set_deadline
  drm/sched: Keep module reference while there are active fences
  drm/xe: Keep module reference while there are active fences

 drivers/dma-buf/dma-fence.c             |  2 +-
 drivers/dma-buf/sync_file.c             | 29 ++++++++++++++++++++-----
 drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++--
 drivers/gpu/drm/xe/xe_hw_fence.c        | 13 ++++++++++-
 4 files changed, 47 insertions(+), 9 deletions(-)

-- 
2.48.0

[RFC 1/4] sync_file: Weakly paper over one use-after-free resulting race

[Tvrtko Ursulin]

Xe and probably some other drivers can tear down the internal state
referenced by exported sync file fence which then causes a null pointer
derefences on accessing said fence.

This is somewhat related to DRM scheduler design where sched fence is
supposed to be allowed to outlive the scheduler instance itself, in which
case either the fence->ops, or just the timeline name may go away and the
fact driver has no real visibility if someone had converted the syncobj
into sync file in the meantime.

Bug can be triggered easily from IGT:

 [IGT] xe_sync_file: starting subtest sync_file_race
 ==================================================================
 BUG: KASAN: slab-use-after-free in drm_sched_fence_get_timeline_name+0xa1/0xb0 [gpu_sched]
 Read of size 8 at addr ffff888126726020 by task xe_sync_file/2931
 ...
 Call Trace:
  <TASK>
  kasan_report+0xeb/0x130
  drm_sched_fence_get_timeline_name+0xa1/0xb0 [gpu_sched]
  sync_file_ioctl+0x3cb/0xb00
 ...
 Allocated by task 2931:
  __kmalloc_cache_noprof+0x1c2/0x410
  guc_exec_queue_init+0x1a8/0x1240 [xe]
  xe_exec_queue_create+0xe72/0x13b0 [xe]
  xe_exec_queue_create_ioctl+0x10d9/0x1770 [xe]
  drm_ioctl_kernel+0x179/0x300
  drm_ioctl+0x58f/0xcf0
  xe_drm_ioctl+0xe8/0x140 [xe]
 ...
 Freed by task 1689:
  kfree+0x106/0x3e0
  __guc_exec_queue_fini_async+0x144/0x2d0 [xe]
  process_one_work+0x610/0xdf0
  worker_thread+0x7c8/0x14b0

This patch papers over it weakly by guarding one entry points with the
signaled check. Race is still there just the window is smaller.

As an alternative we could remove the timeline description from sched
fence altogether, but the name is unfortunately not the only route to
disaster. There is also the dma fence deadline setting ioctl.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/dma-buf/sync_file.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index d9b1c1b2a72b..cfaa7b5d325e 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -116,6 +116,9 @@ struct dma_fence *sync_file_get_fence(int fd)
 }
 EXPORT_SYMBOL(sync_file_get_fence);
 
+const char *sync_fence_signaled_obj_name = "signaled-timeline";
+const char *sync_fence_signaled_driver_name = "signaled-driver";
+
 /**
  * sync_file_get_name - get the name of the sync_file
  * @sync_file:		sync_file to get the fence from
@@ -136,11 +139,18 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
 	} else {
 		struct dma_fence *fence = sync_file->fence;
 
-		snprintf(buf, len, "%s-%s%llu-%lld",
-			 fence->ops->get_driver_name(fence),
-			 fence->ops->get_timeline_name(fence),
-			 fence->context,
-			 fence->seqno);
+		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+			snprintf(buf, len, "%s-%s%llu-%lld",
+				 sync_fence_signaled_driver_name,
+				 sync_fence_signaled_obj_name,
+				 fence->context,
+				 fence->seqno);
+		else
+			snprintf(buf, len, "%s-%s%llu-%lld",
+				 fence->ops->get_driver_name(fence),
+				 fence->ops->get_timeline_name(fence),
+				 fence->context,
+				 fence->seqno);
 	}
 
 	return buf;
@@ -262,6 +272,15 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
 static int sync_fill_fence_info(struct dma_fence *fence,
 				 struct sync_fence_info *info)
 {
+	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+		info->status = fence->error ?: 1;
+		info->timestamp_ns = ktime_to_ns(dma_fence_timestamp(fence));
+		strscpy(info->obj_name, sync_fence_signaled_obj_name);
+		strscpy(info->driver_name, sync_fence_signaled_driver_name);
+
+		return info->status;
+	}
+
 	strscpy(info->obj_name, fence->ops->get_timeline_name(fence),
 		sizeof(info->obj_name));
 	strscpy(info->driver_name, fence->ops->get_driver_name(fence),
-- 
2.48.0

[RFC 2/4] dma-fence: Slightly safer dma_fence_set_deadline

[Tvrtko Ursulin]

Similar to the previous patch lets precede the fence->ops dereference with
the signaled check. This reduces the window to hit the race where sync
file can get into a signaled DRM scheduler fence which was left "dangling"
after the driver had potentially torn down the corresponding internal
state.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/dma-buf/dma-fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index f0cdd3e99d36..b3f085a65910 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -968,7 +968,7 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
  */
 void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
 {
-	if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+	if (!dma_fence_is_signaled(fence) && fence->ops->set_deadline)
 		fence->ops->set_deadline(fence, deadline);
 }
 EXPORT_SYMBOL(dma_fence_set_deadline);
-- 
2.48.0

[RFC 3/4] drm/sched: Keep module reference while there are active fences

[Tvrtko Ursulin]

Continuing the theme from previous patches. This time round we deal with
the problem that it is possible to unbind the driver from the PCI device
with an active sync file fence.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index e971528504a5..178077f03e44 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -115,8 +115,10 @@ static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
 void drm_sched_fence_free(struct drm_sched_fence *fence)
 {
 	/* This function should not be called if the fence has been initialized. */
-	if (!WARN_ON_ONCE(fence->sched))
+	if (!WARN_ON_ONCE(fence->sched)) {
 		kmem_cache_free(sched_fence_slab, fence);
+		module_put(THIS_MODULE);
+	}
 }
 
 /**
@@ -133,6 +135,7 @@ static void drm_sched_fence_release_scheduled(struct dma_fence *f)
 
 	dma_fence_put(fence->parent);
 	call_rcu(&fence->finished.rcu, drm_sched_fence_free_rcu);
+	module_put(THIS_MODULE);
 }
 
 /**
@@ -210,9 +213,14 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity,
 {
 	struct drm_sched_fence *fence = NULL;
 
+	if (!try_module_get(THIS_MODULE))
+		return NULL;
+
 	fence = kmem_cache_zalloc(sched_fence_slab, GFP_KERNEL);
-	if (fence == NULL)
+	if (!fence) {
+		module_put(THIS_MODULE);
 		return NULL;
+	}
 
 	fence->owner = owner;
 	spin_lock_init(&fence->lock);
-- 
2.48.0

[RFC 4/4] drm/xe: Keep module reference while there are active fences

[Tvrtko Ursulin]

Continuing the theme from previous patches. This time round we deal with
the problem that it is possible to unbind the driver from the PCI device
with an active sync file fence.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_fence.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
index 0b4f12be3692..209ed6ca78d9 100644
--- a/drivers/gpu/drm/xe/xe_hw_fence.c
+++ b/drivers/gpu/drm/xe/xe_hw_fence.c
@@ -37,7 +37,16 @@ void xe_hw_fence_module_exit(void)
 
 static struct xe_hw_fence *fence_alloc(void)
 {
-	return kmem_cache_zalloc(xe_hw_fence_slab, GFP_KERNEL);
+	struct xe_hw_fence *fence;
+
+	if (!try_module_get(THIS_MODULE))
+		return NULL;
+
+	fence = kmem_cache_zalloc(xe_hw_fence_slab, GFP_KERNEL);
+	if (!fence)
+		module_put(THIS_MODULE);
+
+	return fence;
 }
 
 static void fence_free(struct rcu_head *rcu)
@@ -189,6 +198,7 @@ static void xe_hw_fence_release(struct dma_fence *dma_fence)
 
 	XE_WARN_ON(!list_empty(&fence->irq_link));
 	call_rcu(&dma_fence->rcu, fence_free);
+	module_put(THIS_MODULE);
 }
 
 static const struct dma_fence_ops xe_hw_fence_ops = {
@@ -235,6 +245,7 @@ struct dma_fence *xe_hw_fence_alloc(void)
 void xe_hw_fence_free(struct dma_fence *fence)
 {
 	fence_free(&fence->rcu);
+	module_put(THIS_MODULE);
 }
 
 /**
-- 
2.48.0

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Christian König]

On 4/18/25 18:42, Tvrtko Ursulin wrote:
> Hi all,
> 
> Recently I mentioned to Danilo about some fence lifetime issues so here is a
> rough series, more than anything intended to start the discussion.
> 
> Most of the problem statement can be found in the first patch but to briefly
> summarise - because sched fence can outlive the scheduler, we can trivially
> engineer an use after free with xe and possibly other drivers. All that is
> needed is to convert a syncobj into a sync file behind drivers back, and I don't
> see what the driver can do about it.


Yeah that topic again :) The problem here is that this is not a bug, it is a feature!

IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.

The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.

The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)

I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.

I then came up with the following steps to allow this:
1. Decouple the lock used for protecting the dma_fence callback list from the caller.
2. Stop calling enable_signaling with the lock held.
3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().
4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.

I already send patches out for #1 and #2, but don't have enough time to actually finish the work.

If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.

Regards,
Christian.


> 
> IGT that exploits the problem:
> https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2
> 
> Different flavour of the problem space is if we had a close(drm_fd) in that test
> before the sleep. In that case we can even unload xe.ko and gpu-sched.ko for
> even more fun. Last two patches in the series close that gap.
> 
> But first two patches are just shrinking the race window. They are not proper
> fixes. This is what I want to discuss since I understand reference counting all
> the involved objects has been rejected in the past. And since the problem
> probably expands to all dma fences it certainly isn't easy.
> 
> To be clear once more - lets not focus on how this does not fix it fully - I am
> primarily trying to start the conversation.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Tvrtko Ursulin (4):
>   sync_file: Weakly paper over one use-after-free resulting race
>   dma-fence: Slightly safer dma_fence_set_deadline
>   drm/sched: Keep module reference while there are active fences
>   drm/xe: Keep module reference while there are active fences
> 
>  drivers/dma-buf/dma-fence.c             |  2 +-
>  drivers/dma-buf/sync_file.c             | 29 ++++++++++++++++++++-----
>  drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++--
>  drivers/gpu/drm/xe/xe_hw_fence.c        | 13 ++++++++++-
>  4 files changed, 47 insertions(+), 9 deletions(-)
> 

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Matthew Brost]

On Wed, Apr 23, 2025 at 03:12:27PM +0200, Christian König wrote:
> On 4/18/25 18:42, Tvrtko Ursulin wrote:
> > Hi all,
> > 
> > Recently I mentioned to Danilo about some fence lifetime issues so here is a
> > rough series, more than anything intended to start the discussion.
> > 
> > Most of the problem statement can be found in the first patch but to briefly
> > summarise - because sched fence can outlive the scheduler, we can trivially
> > engineer an use after free with xe and possibly other drivers. All that is
> > needed is to convert a syncobj into a sync file behind drivers back, and I don't
> > see what the driver can do about it.
> 
> 
> Yeah that topic again :) The problem here is that this is not a bug, it is a feature!
>

Really more unforeseen design flaw IMO - it happens.

> IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.
> 
> The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.
> 

Yea, the Xe patch holding module ref is a no-go.

> The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)
> 
> I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.
> 
> I then came up with the following steps to allow this:
> 1. Decouple the lock used for protecting the dma_fence callback list from the caller.
> 2. Stop calling enable_signaling with the lock held.
> 3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().

Let's document this too.

Xe is an offender, I'll post a fix tomorrow.

> 4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.
> 
> I already send patches out for #1 and #2, but don't have enough time to actually finish the work.
>

Link? This has been lingering for a while perhaps the community can pick this up.

Matt

> If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.
> 
> Regards,
> Christian.
> 
> 
> > 
> > IGT that exploits the problem:
> > https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2
> > 
> > Different flavour of the problem space is if we had a close(drm_fd) in that test
> > before the sleep. In that case we can even unload xe.ko and gpu-sched.ko for
> > even more fun. Last two patches in the series close that gap.
> > 
> > But first two patches are just shrinking the race window. They are not proper
> > fixes. This is what I want to discuss since I understand reference counting all
> > the involved objects has been rejected in the past. And since the problem
> > probably expands to all dma fences it certainly isn't easy.
> > 
> > To be clear once more - lets not focus on how this does not fix it fully - I am
> > primarily trying to start the conversation.
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Philipp Stanner <phasta@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Tvrtko Ursulin (4):
> >   sync_file: Weakly paper over one use-after-free resulting race
> >   dma-fence: Slightly safer dma_fence_set_deadline
> >   drm/sched: Keep module reference while there are active fences
> >   drm/xe: Keep module reference while there are active fences
> > 
> >  drivers/dma-buf/dma-fence.c             |  2 +-
> >  drivers/dma-buf/sync_file.c             | 29 ++++++++++++++++++++-----
> >  drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++--
> >  drivers/gpu/drm/xe/xe_hw_fence.c        | 13 ++++++++++-
> >  4 files changed, 47 insertions(+), 9 deletions(-)
> > 
> 

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Tvrtko Ursulin]

On 23/04/2025 14:12, Christian König wrote:
> On 4/18/25 18:42, Tvrtko Ursulin wrote:
>> Hi all,
>>
>> Recently I mentioned to Danilo about some fence lifetime issues so here is a
>> rough series, more than anything intended to start the discussion.
>>
>> Most of the problem statement can be found in the first patch but to briefly
>> summarise - because sched fence can outlive the scheduler, we can trivially
>> engineer an use after free with xe and possibly other drivers. All that is
>> needed is to convert a syncobj into a sync file behind drivers back, and I don't
>> see what the driver can do about it.
> 
> 
> Yeah that topic again :) The problem here is that this is not a bug, it is a feature!
> 
> IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.
> 
> The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.

Where "always" is only "while there are active objects from that 
module", no?

> The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)
> 
> I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.
> 
> I then came up with the following steps to allow this:
> 1. Decouple the lock used for protecting the dma_fence callback list from the caller.
> 2. Stop calling enable_signaling with the lock held.
> 3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().
> 4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.
> 
> I already send patches out for #1 and #2, but don't have enough time to actually finish the work.
> 
> If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.

So enabling dma fence "revoke" so to say.

Just to check we are on the same page, it is not just about the module 
references, but also use after frees which can happen even if module is 
still loaded but any memory reachable via dma fence entry points has 
been freed.

In that case, as Matt has already asked, if you could dig up your 
unfinished work it would be interesting to see.

Regards,

Tvrtko


>> IGT that exploits the problem:
>> https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2
>>
>> Different flavour of the problem space is if we had a close(drm_fd) in that test
>> before the sleep. In that case we can even unload xe.ko and gpu-sched.ko for
>> even more fun. Last two patches in the series close that gap.
>>
>> But first two patches are just shrinking the race window. They are not proper
>> fixes. This is what I want to discuss since I understand reference counting all
>> the involved objects has been rejected in the past. And since the problem
>> probably expands to all dma fences it certainly isn't easy.
>>
>> To be clear once more - lets not focus on how this does not fix it fully - I am
>> primarily trying to start the conversation.
>>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>> Tvrtko Ursulin (4):
>>    sync_file: Weakly paper over one use-after-free resulting race
>>    dma-fence: Slightly safer dma_fence_set_deadline
>>    drm/sched: Keep module reference while there are active fences
>>    drm/xe: Keep module reference while there are active fences
>>
>>   drivers/dma-buf/dma-fence.c             |  2 +-
>>   drivers/dma-buf/sync_file.c             | 29 ++++++++++++++++++++-----
>>   drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++--
>>   drivers/gpu/drm/xe/xe_hw_fence.c        | 13 ++++++++++-
>>   4 files changed, 47 insertions(+), 9 deletions(-)
>>
> 

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Christian König]

On 4/24/25 09:07, Tvrtko Ursulin wrote:
> 
> On 23/04/2025 14:12, Christian König wrote:
>> On 4/18/25 18:42, Tvrtko Ursulin wrote:
>>> Hi all,
>>>
>>> Recently I mentioned to Danilo about some fence lifetime issues so here is a
>>> rough series, more than anything intended to start the discussion.
>>>
>>> Most of the problem statement can be found in the first patch but to briefly
>>> summarise - because sched fence can outlive the scheduler, we can trivially
>>> engineer an use after free with xe and possibly other drivers. All that is
>>> needed is to convert a syncobj into a sync file behind drivers back, and I don't
>>> see what the driver can do about it.
>>
>>
>> Yeah that topic again :) The problem here is that this is not a bug, it is a feature!
>>
>> IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.
>>
>> The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.
> 
> Where "always" is only "while there are active objects from that module", no?


The problem is that dma_fences stay around after they are signaled. And basically all drivers keep some dma_fence around for their resource management. E.g. amdgpu for the VMIDs.

This means that some dma_fence is referenced by the module and the module referenced by some dma_fence. E.g. you are never able to unload the module.


> 
>> The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)
>>
>> I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.
>>
>> I then came up with the following steps to allow this:
>> 1. Decouple the lock used for protecting the dma_fence callback list from the caller.
>> 2. Stop calling enable_signaling with the lock held.
>> 3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().
>> 4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.
>>
>> I already send patches out for #1 and #2, but don't have enough time to actually finish the work.
>>
>> If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.
> 
> So enabling dma fence "revoke" so to say.
> 
> Just to check we are on the same page, it is not just about the module references, but also use after frees which can happen even if module is still loaded but any memory reachable via dma fence entry points has been freed.


Yeah, that came much later when people started to use the scheduler dynamically. Basically the sched pointer in the drm_sched_fence implementation becomes invalid as soon as the fence signals.

> 
> In that case, as Matt has already asked, if you could dig up your unfinished work it would be interesting to see.


This is what I already send out: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-enable-signaling

A bunch of the cleanup patches in that branch have already been applied, only the last one is missing IIRC.

And here is a WIP patch to decouple the lock I wrote halve a year ago or so: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-locking

Regards,
Christian.


> 
> Regards,
> 
> Tvrtko
> 
> 
>>> IGT that exploits the problem:
>>> https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2
>>>
>>> Different flavour of the problem space is if we had a close(drm_fd) in that test
>>> before the sleep. In that case we can even unload xe.ko and gpu-sched.ko for
>>> even more fun. Last two patches in the series close that gap.
>>>
>>> But first two patches are just shrinking the race window. They are not proper
>>> fixes. This is what I want to discuss since I understand reference counting all
>>> the involved objects has been rejected in the past. And since the problem
>>> probably expands to all dma fences it certainly isn't easy.
>>>
>>> To be clear once more - lets not focus on how this does not fix it fully - I am
>>> primarily trying to start the conversation.
>>>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: Philipp Stanner <phasta@kernel.org>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>
>>> Tvrtko Ursulin (4):
>>>    sync_file: Weakly paper over one use-after-free resulting race
>>>    dma-fence: Slightly safer dma_fence_set_deadline
>>>    drm/sched: Keep module reference while there are active fences
>>>    drm/xe: Keep module reference while there are active fences
>>>
>>>   drivers/dma-buf/dma-fence.c             |  2 +-
>>>   drivers/dma-buf/sync_file.c             | 29 ++++++++++++++++++++-----
>>>   drivers/gpu/drm/scheduler/sched_fence.c | 12 ++++++++--
>>>   drivers/gpu/drm/xe/xe_hw_fence.c        | 13 ++++++++++-
>>>   4 files changed, 47 insertions(+), 9 deletions(-)
>>>
>>
> 

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Tvrtko Ursulin]

On 28/04/2025 14:15, Christian König wrote:
> On 4/24/25 09:07, Tvrtko Ursulin wrote:
>>
>> On 23/04/2025 14:12, Christian König wrote:
>>> On 4/18/25 18:42, Tvrtko Ursulin wrote:
>>>> Hi all,
>>>>
>>>> Recently I mentioned to Danilo about some fence lifetime issues so here is a
>>>> rough series, more than anything intended to start the discussion.
>>>>
>>>> Most of the problem statement can be found in the first patch but to briefly
>>>> summarise - because sched fence can outlive the scheduler, we can trivially
>>>> engineer an use after free with xe and possibly other drivers. All that is
>>>> needed is to convert a syncobj into a sync file behind drivers back, and I don't
>>>> see what the driver can do about it.
>>>
>>>
>>> Yeah that topic again :) The problem here is that this is not a bug, it is a feature!
>>>
>>> IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.
>>>
>>> The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.
>>
>> Where "always" is only "while there are active objects from that module", no?
> 
> 
> The problem is that dma_fences stay around after they are signaled. And basically all drivers keep some dma_fence around for their resource management. E.g. amdgpu for the VMIDs.
> 
> This means that some dma_fence is referenced by the module and the module referenced by some dma_fence. E.g. you are never able to unload the module.

Are you thinking truly never or for as long someone has a reference?

For example while userspace has a reference to dma_fence via sync_file 
fence owning module would not unloadable. One would have to terminate 
the process, which granted wouldn't be easy to see which process 
prevents the unload, before driver could be unloaded.

For the foreign fences kept around in kernel space, that would be 
solvable by some periodic house keeping at worst.

Also, about the use cases for module unload. Since you and Brost 
especially seem to be expressing a hard no to module references, what 
are the use cases you are concerned about?

>>> The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)
>>>
>>> I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.
>>>
>>> I then came up with the following steps to allow this:
>>> 1. Decouple the lock used for protecting the dma_fence callback list from the caller.
>>> 2. Stop calling enable_signaling with the lock held.
>>> 3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().
>>> 4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.
>>>
>>> I already send patches out for #1 and #2, but don't have enough time to actually finish the work.
>>>
>>> If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.
>>
>> So enabling dma fence "revoke" so to say.
>>
>> Just to check we are on the same page, it is not just about the module references, but also use after frees which can happen even if module is still loaded but any memory reachable via dma fence entry points has been freed.
> 
> 
> Yeah, that came much later when people started to use the scheduler dynamically. Basically the sched pointer in the drm_sched_fence implementation becomes invalid as soon as the fence signals.
> 
>>
>> In that case, as Matt has already asked, if you could dig up your unfinished work it would be interesting to see.
> 
> 
> This is what I already send out: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-enable-signaling
> 
> A bunch of the cleanup patches in that branch have already been applied, only the last one is missing IIRC.
> 
> And here is a WIP patch to decouple the lock I wrote halve a year ago or so: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-locking

Thanks!

My concern here is that to me it appears the whole premise is to leave 
fences dangling in memory and somehow make them safe to be accessed by 
importers.

For starters this can create permanent memory leaks. Or at least for the 
same window of duration as would the exporters be not unloadable with 
the reference counting alternative. So we would not a strong argument 
for why poorly bound memory leaks are better than poorly bound 
unloadable modules.

It is also a question how to "revoke" fences safely (race free). It 
sounds hard to me. It does not seem you got to this last problem in the 
above branches so I don't know if you had some elegant ideas for that.

Maybe first to ask if anyone is aware of a precedent where something in 
the kernel already uses this design pattern?

Regards,

Tvrtko

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Christian König]

On 5/7/25 14:28, Tvrtko Ursulin wrote:
> 
> On 28/04/2025 14:15, Christian König wrote:
>> On 4/24/25 09:07, Tvrtko Ursulin wrote:
>>>
>>> On 23/04/2025 14:12, Christian König wrote:
>>>> On 4/18/25 18:42, Tvrtko Ursulin wrote:
>>>>> Hi all,
>>>>>
>>>>> Recently I mentioned to Danilo about some fence lifetime issues so here is a
>>>>> rough series, more than anything intended to start the discussion.
>>>>>
>>>>> Most of the problem statement can be found in the first patch but to briefly
>>>>> summarise - because sched fence can outlive the scheduler, we can trivially
>>>>> engineer an use after free with xe and possibly other drivers. All that is
>>>>> needed is to convert a syncobj into a sync file behind drivers back, and I don't
>>>>> see what the driver can do about it.
>>>>
>>>>
>>>> Yeah that topic again :) The problem here is that this is not a bug, it is a feature!
>>>>
>>>> IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.
>>>>
>>>> The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.
>>>
>>> Where "always" is only "while there are active objects from that module", no?
>>
>>
>> The problem is that dma_fences stay around after they are signaled. And basically all drivers keep some dma_fence around for their resource management. E.g. amdgpu for the VMIDs.
>>
>> This means that some dma_fence is referenced by the module and the module referenced by some dma_fence. E.g. you are never able to unload the module.
> 
> Are you thinking truly never or for as long someone has a reference?

Truly never. It's simply a circle dependency you can never break up.

In other words the module references the fence and the fence references the module.

> 
> For example while userspace has a reference to dma_fence via sync_file fence owning module would not unloadable. One would have to terminate the process, which granted wouldn't be easy to see which process prevents the unload, before driver could be unloaded.
> 
> For the foreign fences kept around in kernel space, that would be solvable by some periodic house keeping at worst.
> 
> Also, about the use cases for module unload. Since you and Brost especially seem to be expressing a hard no to module references, what are the use cases you are concerned about?
> 
>>>> The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)
>>>>
>>>> I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.
>>>>
>>>> I then came up with the following steps to allow this:
>>>> 1. Decouple the lock used for protecting the dma_fence callback list from the caller.
>>>> 2. Stop calling enable_signaling with the lock held.
>>>> 3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().
>>>> 4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.
>>>>
>>>> I already send patches out for #1 and #2, but don't have enough time to actually finish the work.
>>>>
>>>> If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.
>>>
>>> So enabling dma fence "revoke" so to say.
>>>
>>> Just to check we are on the same page, it is not just about the module references, but also use after frees which can happen even if module is still loaded but any memory reachable via dma fence entry points has been freed.
>>
>>
>> Yeah, that came much later when people started to use the scheduler dynamically. Basically the sched pointer in the drm_sched_fence implementation becomes invalid as soon as the fence signals.
>>
>>>
>>> In that case, as Matt has already asked, if you could dig up your unfinished work it would be interesting to see.
>>
>>
>> This is what I already send out: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-enable-signaling
>>
>> A bunch of the cleanup patches in that branch have already been applied, only the last one is missing IIRC.
>>
>> And here is a WIP patch to decouple the lock I wrote halve a year ago or so: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-locking
> 
> Thanks!
> 
> My concern here is that to me it appears the whole premise is to leave fences dangling in memory and somehow make them safe to be accessed by importers.

As soon as you unload the last module using it the fences will automatically be released. So I don't see the problem.

> For starters this can create permanent memory leaks. Or at least for the same window of duration as would the exporters be not unloadable with the reference counting alternative. So we would not a strong argument for why poorly bound memory leaks are better than poorly bound unloadable modules.

When the module unloads it drops the reference to the fences ultimately freeing them.

The only issue is that modules can both reference their own as well a foreign fences. So what can happen is that you have module A which references fences A1, A2 and B1 and module B which references B1, B2 and A2.

Now you can't unload either module first because they cross reference their fences and unloading one would leave the other module with fences which can't be released without crashing.

So what we need to have is that the dma_fence framework guarantees that you don't need the fence->ops nor the fence->lock pointer any more after the fence signaled.

> It is also a question how to "revoke" fences safely (race free). It sounds hard to me. It does not seem you got to this last problem in the above branches so I don't know if you had some elegant ideas for that.
> 
> Maybe first to ask if anyone is aware of a precedent where something in the kernel already uses this design pattern?

Of hand I don't know of any, but the problem sounds rather common to me.

Regards,
Christian.

> 
> Regards,
> 
> Tvrtko
> 

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Tvrtko Ursulin]

On 07/05/2025 13:54, Christian König wrote:
> On 5/7/25 14:28, Tvrtko Ursulin wrote:
>>
>> On 28/04/2025 14:15, Christian König wrote:
>>> On 4/24/25 09:07, Tvrtko Ursulin wrote:
>>>>
>>>> On 23/04/2025 14:12, Christian König wrote:
>>>>> On 4/18/25 18:42, Tvrtko Ursulin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Recently I mentioned to Danilo about some fence lifetime issues so here is a
>>>>>> rough series, more than anything intended to start the discussion.
>>>>>>
>>>>>> Most of the problem statement can be found in the first patch but to briefly
>>>>>> summarise - because sched fence can outlive the scheduler, we can trivially
>>>>>> engineer an use after free with xe and possibly other drivers. All that is
>>>>>> needed is to convert a syncobj into a sync file behind drivers back, and I don't
>>>>>> see what the driver can do about it.
>>>>>
>>>>>
>>>>> Yeah that topic again :) The problem here is that this is not a bug, it is a feature!
>>>>>
>>>>> IIRC it was Alex who pointed that issue out on the very first fence patch set, and we already discussed what to do back then.
>>>>>
>>>>> The problem with grabbing module references for fences is that you get trivially into circle references and so basically always preventing the module from unloading.
>>>>
>>>> Where "always" is only "while there are active objects from that module", no?
>>>
>>>
>>> The problem is that dma_fences stay around after they are signaled. And basically all drivers keep some dma_fence around for their resource management. E.g. amdgpu for the VMIDs.
>>>
>>> This means that some dma_fence is referenced by the module and the module referenced by some dma_fence. E.g. you are never able to unload the module.
>>
>> Are you thinking truly never or for as long someone has a reference?
> 
> Truly never. It's simply a circle dependency you can never break up.
> 
> In other words the module references the fence and the fence references the module.

Past fences being signaled? How?

>> For example while userspace has a reference to dma_fence via sync_file fence owning module would not unloadable. One would have to terminate the process, which granted wouldn't be easy to see which process prevents the unload, before driver could be unloaded.
>>
>> For the foreign fences kept around in kernel space, that would be solvable by some periodic house keeping at worst.
>>
>> Also, about the use cases for module unload. Since you and Brost especially seem to be expressing a hard no to module references, what are the use cases you are concerned about?
>>
>>>>> The decision was made to postpone this and live with the potential use after free on module unload until somebody has time to fix it. Well that was +10 years ago :)
>>>>>
>>>>> I discussed this with Sima again last year and we came to the conclusion that the easiest way forward would be to decouple the dma_fence implementation from the driver or component issuing the fence.
>>>>>
>>>>> I then came up with the following steps to allow this:
>>>>> 1. Decouple the lock used for protecting the dma_fence callback list from the caller.
>>>>> 2. Stop calling enable_signaling with the lock held.
>>>>> 3. Nuke all those kmem_cache implementations and force drivers to always allocate fences using kvmalloc().
>>>>> 4. Nuke the release callback (or maybe move it directly after signaling) and set fence->ops to NULL after signaling the fence.
>>>>>
>>>>> I already send patches out for #1 and #2, but don't have enough time to actually finish the work.
>>>>>
>>>>> If you want take a look at nuking all those kmem_cache implementations for allocating the fence memory. I think that can be completed completely separate to everything else.
>>>>
>>>> So enabling dma fence "revoke" so to say.
>>>>
>>>> Just to check we are on the same page, it is not just about the module references, but also use after frees which can happen even if module is still loaded but any memory reachable via dma fence entry points has been freed.
>>>
>>>
>>> Yeah, that came much later when people started to use the scheduler dynamically. Basically the sched pointer in the drm_sched_fence implementation becomes invalid as soon as the fence signals.
>>>
>>>>
>>>> In that case, as Matt has already asked, if you could dig up your unfinished work it would be interesting to see.
>>>
>>>
>>> This is what I already send out: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-enable-signaling
>>>
>>> A bunch of the cleanup patches in that branch have already been applied, only the last one is missing IIRC.
>>>
>>> And here is a WIP patch to decouple the lock I wrote halve a year ago or so: https://gitlab.freedesktop.org/ckoenig/linux-drm/-/commits/dma-fence-rework-locking
>>
>> Thanks!
>>
>> My concern here is that to me it appears the whole premise is to leave fences dangling in memory and somehow make them safe to be accessed by importers.
> 
> As soon as you unload the last module using it the fences will automatically be released. So I don't see the problem.
> 
>> For starters this can create permanent memory leaks. Or at least for the same window of duration as would the exporters be not unloadable with the reference counting alternative. So we would not a strong argument for why poorly bound memory leaks are better than poorly bound unloadable modules.
> 
> When the module unloads it drops the reference to the fences ultimately freeing them.
> 
> The only issue is that modules can both reference their own as well a foreign fences. So what can happen is that you have module A which references fences A1, A2 and B1 and module B which references B1, B2 and A2.
> 
> Now you can't unload either module first because they cross reference their fences and unloading one would leave the other module with fences which can't be released without crashing.
> 
> So what we need to have is that the dma_fence framework guarantees that you don't need the fence->ops nor the fence->lock pointer any more after the fence signaled.

With this option it would mean guarding all entry points with the 
embedded lock or you had in mind something different? Going simply by 
the signaled bit looks impossible to be safe.

>> It is also a question how to "revoke" fences safely (race free). It sounds hard to me. It does not seem you got to this last problem in the above branches so I don't know if you had some elegant ideas for that.
>>
>> Maybe first to ask if anyone is aware of a precedent where something in the kernel already uses this design pattern?
> 
> Of hand I don't know of any, but the problem sounds rather common to me.

Uf I don't know. Feels very atypical to me but I would be very glad to 
be told otherwise.

Regards,

Tvrtko

Re: [RFC 0/4] Some (drm_sched_|dma_)fence lifetime issues

[Christian König]

On 5/7/25 16:07, Tvrtko Ursulin wrote:
>>> Are you thinking truly never or for as long someone has a reference?
>>
>> Truly never. It's simply a circle dependency you can never break up.
>>
>> In other words the module references the fence and the fence references the module.
> 
> Past fences being signaled? How?

See how for example amdgpu manages it's VMIDs. Basically the driver keeps an array of all the fence which every used the VMID.

When a VMID is needed the driver checks those fences and eventually frees the signaled ones until an idle VMID is found.

The problem is that freeing the old signaled fences is a lazy operation and only done when a new request comes in.

As far as I know we have tons of those use cases spread all around in different drivers.

>> When the module unloads it drops the reference to the fences ultimately freeing them.
>>
>> The only issue is that modules can both reference their own as well a foreign fences. So what can happen is that you have module A which references fences A1, A2 and B1 and module B which references B1, B2 and A2.
>>
>> Now you can't unload either module first because they cross reference their fences and unloading one would leave the other module with fences which can't be released without crashing.
>>
>> So what we need to have is that the dma_fence framework guarantees that you don't need the fence->ops nor the fence->lock pointer any more after the fence signaled.
> 
> With this option it would mean guarding all entry points with the embedded lock or you had in mind something different? Going simply by the signaled bit looks impossible to be safe.

The module can't unload until all it's fences are signaled, that's obvious.

I think it's save to assume that module unload doesn't happens right after signaling a fence, so we should be save to assume that nobody is inside the callbacks any more after some grace period.

We could use some SRCU or similar to enforce that but my gut feeling is that this would hurt more than help, especially since the code is really performance critical. 


>>> It is also a question how to "revoke" fences safely (race free). It sounds hard to me. It does not seem you got to this last problem in the above branches so I don't know if you had some elegant ideas for that.
>>>
>>> Maybe first to ask if anyone is aware of a precedent where something in the kernel already uses this design pattern?
>>
>> Of hand I don't know of any, but the problem sounds rather common to me.
> 
> Uf I don't know. Feels very atypical to me but I would be very glad to be told otherwise.

I briefly remember that Greg once kicked me because I accidentally violated some rule in the FS layer which implemented something similar for signal/polling.

E.g. you can unmount and unload a file system even if userspace is still sleeping and waiting on something.

I need to look that up again, maybe it provides a good pattern on how to solve this.

Regards,
Christian.



> 
> Regards,
> 
> Tvrtko
>