* [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:06 [PATCH v4 0/3] drm/sched: fix use-after-free of the fence timeline name Jonghyuk Kim(MalHyuk)
@ 2026-09-04 8:06 ` Jonghyuk Kim(MalHyuk)
2026-09-04 8:20 ` Christian König
2026-09-04 9:57 ` Danilo Krummrich
2026-09-04 8:06 ` [PATCH v4 2/3] drm/sched: add the fence ops-detach cleanup to the TODO list Jonghyuk Kim(MalHyuk)
2026-09-04 8:06 ` [PATCH v4 3/3] drm/sched/tests: add a UAF regression test for the timeline name Jonghyuk Kim(MalHyuk)
2 siblings, 2 replies; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-04 8:06 UTC (permalink / raw)
To: phasta, christian.koenig, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, Jonghyuk Kim(MalHyuk),
stable
drm_sched_fence_get_timeline_name() returns fence->sched->name, and the
drm_sched_fence ops keep a .release callback, so the fence is not
ops-detached on signalling (dma_fence_signal_timestamp_locked() only
clears ->ops for fences without .release/.wait). The callback therefore
stays reachable on a long-signalled, userspace-held finished fence and
unconditionally dereferences fence->sched.
A driver that allocates a drm_gpu_scheduler at per-context/per-queue/per-VM
granularity and frees it on an unprivileged context/fd close, while
exporting the resulting finished fence to userspace (drm_syncobj /
sync_file / dma_resv), leaves fence->sched dangling after the free. A
subsequent SYNC_IOC_FILE_INFO ioctl (which calls get_timeline_name()) then
reads the freed scheduler:
BUG: KASAN: slab-use-after-free in drm_sched_fence_get_timeline_name
This is the same class as CVE-2025-38703 (drm/xe) and CVE-2025-71302
(drm/panthor), which were fixed per-driver. amdxdna, nouveau and msm
(VM_BIND) are still affected in mainline, so fix it in the core to cover
any per-context-scheduler driver at once.
Cache the scheduler's name pointer in the fence at init time, while the
scheduler is guaranteed alive, and return the cached value from
get_timeline_name() without dereferencing fence->sched. The timeline name
is not guaranteed by the contract to outlive the scheduler, so document in
struct drm_sched_init_args that the @name passed to drm_sched_init() must
follow the dma-fence safe access rules and outlive any exported fence.
Every in-tree driver passes a string literal, which satisfies this;
commit 299bc6d50b1b ("drm/xe/guc: Keep scheduler timeline name alive")
keeps drm/xe's dynamically-allocated name alive across the RCU grace and
can be simplified on top of this.
Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the rules")
Cc: stable@vger.kernel.org # we don't know since when
Signed-off-by: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>
---
drivers/gpu/drm/scheduler/sched_fence.c | 24 +++++++++++++++++++++++-
include/drm/gpu_scheduler.h | 18 +++++++++++++++++-
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 096fe28aa9c9..b2a842a1c9ba 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -92,7 +92,13 @@ static const char *drm_sched_fence_get_driver_name(struct dma_fence *fence)
static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
{
struct drm_sched_fence *fence = to_drm_sched_fence(f);
- return (const char *)fence->sched->name;
+
+ /*
+ * Do not dereference fence->sched here: a userspace-held finished
+ * fence can outlive a per-context scheduler. Return the name cached
+ * in drm_sched_fence_init() instead.
+ */
+ return fence->sched_name;
}
static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
@@ -180,6 +186,14 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
dma_fence_set_deadline(parent, deadline);
}
+/*
+ * TODO: Both fences implement .release, so dma_fence keeps their ops attached
+ * after signalling. Dropping the callbacks would let dma_fence detach the ops,
+ * after which neither get_timeline_name() nor get_driver_name() can run against
+ * a freed scheduler or an unloaded module - the complete fix. It first requires
+ * auditing every to_drm_sched_fence() caller, since ops-detach makes the helper
+ * return NULL for a signalled fence. See Documentation/gpu/todo.rst.
+ */
static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
.get_driver_name = drm_sched_fence_get_driver_name,
.get_timeline_name = drm_sched_fence_get_timeline_name,
@@ -228,6 +242,14 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
unsigned seq;
fence->sched = entity->rq->sched;
+ /*
+ * Cache the scheduler's timeline name. The finished fence may be
+ * exported to userspace and outlive @sched (per-context schedulers are
+ * freed on context teardown), so get_timeline_name() must not
+ * dereference @sched. The name is required to outlive any exported
+ * fence (see @name in struct drm_sched_init_args).
+ */
+ fence->sched_name = fence->sched->name;
seq = atomic_inc_return(&entity->fence_seq);
dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
&fence->lock, entity->fence_context, seq);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 7a64cc11de08..412b8c4643f1 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -322,6 +322,17 @@ struct drm_sched_fence {
* belongs to.
*/
struct drm_gpu_scheduler *sched;
+ /**
+ * @sched_name: the timeline name of @sched, cached at init time.
+ *
+ * &drm_sched_fence.finished may be exported to userspace (via a
+ * sync_file or drm_syncobj) and can outlive @sched: a driver using a
+ * per-context scheduler frees it on context teardown while a
+ * userspace-held finished fence still references it. The
+ * get_timeline_name() callback must therefore not dereference @sched;
+ * it returns this cached name instead.
+ */
+ const char *sched_name;
/**
* @lock: the lock used by the scheduled and the finished fences.
*/
@@ -646,7 +657,12 @@ struct drm_gpu_scheduler {
* @timeout: timeout value in jiffies for submitted jobs.
* @timeout_wq: workqueue to use for timeout work. If NULL, the system_wq is used.
* @score: score atomic shared with other schedulers. May be NULL.
- * @name: name (typically the driver's name). Used for debugging
+ * @name: name (typically the driver's name). Used for debugging, and as the
+ * dma-fence timeline name of the scheduler's fences. It must follow the
+ * dma-fence safe access rules: a &drm_sched_fence.finished exported to
+ * userspace can outlive the scheduler, so @name has to outlive any such
+ * fence - use a string literal, or free it only after an RCU grace period
+ * past the last exported fence. See drm_sched_fence_get_timeline_name().
* @dev: associated device. Used for debugging
*/
struct drm_sched_init_args {
--
2.43.0
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:06 ` [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free Jonghyuk Kim(MalHyuk)
@ 2026-09-04 8:20 ` Christian König
2026-09-04 8:31 ` Philipp Stanner
2026-09-04 8:31 ` Jonghyuk Kim(MalHyuk)
2026-09-04 9:57 ` Danilo Krummrich
1 sibling, 2 replies; 34+ messages in thread
From: Christian König @ 2026-09-04 8:20 UTC (permalink / raw)
To: Jonghyuk Kim(MalHyuk), phasta, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 9/4/26 10:06, Jonghyuk Kim(MalHyuk) wrote:
> drm_sched_fence_get_timeline_name() returns fence->sched->name, and the
> drm_sched_fence ops keep a .release callback, so the fence is not
> ops-detached on signalling (dma_fence_signal_timestamp_locked() only
> clears ->ops for fences without .release/.wait). The callback therefore
> stays reachable on a long-signalled, userspace-held finished fence and
> unconditionally dereferences fence->sched.
>
> A driver that allocates a drm_gpu_scheduler at per-context/per-queue/per-VM
> granularity and frees it on an unprivileged context/fd close, while
> exporting the resulting finished fence to userspace (drm_syncobj /
> sync_file / dma_resv), leaves fence->sched dangling after the free. A
> subsequent SYNC_IOC_FILE_INFO ioctl (which calls get_timeline_name()) then
> reads the freed scheduler:
>
> BUG: KASAN: slab-use-after-free in drm_sched_fence_get_timeline_name
>
> This is the same class as CVE-2025-38703 (drm/xe) and CVE-2025-71302
> (drm/panthor), which were fixed per-driver. amdxdna, nouveau and msm
> (VM_BIND) are still affected in mainline, so fix it in the core to cover
> any per-context-scheduler driver at once.
>
> Cache the scheduler's name pointer in the fence at init time, while the
> scheduler is guaranteed alive, and return the cached value from
> get_timeline_name() without dereferencing fence->sched. The timeline name
> is not guaranteed by the contract to outlive the scheduler, so document in
> struct drm_sched_init_args that the @name passed to drm_sched_init() must
> follow the dma-fence safe access rules and outlive any exported fence.
> Every in-tree driver passes a string literal, which satisfies this;
> commit 299bc6d50b1b ("drm/xe/guc: Keep scheduler timeline name alive")
> keeps drm/xe's dynamically-allocated name alive across the RCU grace and
> can be simplified on top of this.
>
> Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the rules")
> Cc: stable@vger.kernel.org # we don't know since when
> Signed-off-by: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>
> ---
> drivers/gpu/drm/scheduler/sched_fence.c | 24 +++++++++++++++++++++++-
> include/drm/gpu_scheduler.h | 18 +++++++++++++++++-
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
> index 096fe28aa9c9..b2a842a1c9ba 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -92,7 +92,13 @@ static const char *drm_sched_fence_get_driver_name(struct dma_fence *fence)
> static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
> {
> struct drm_sched_fence *fence = to_drm_sched_fence(f);
> - return (const char *)fence->sched->name;
> +
> + /*
> + * Do not dereference fence->sched here: a userspace-held finished
> + * fence can outlive a per-context scheduler. Return the name cached
> + * in drm_sched_fence_init() instead.
> + */
> + return fence->sched_name;
I don't think that this actually solves the problem, the sched_name still needs to be kept alive until all fences are destroyed and that is something drivers don't want/can do.
> }
>
> static void drm_sched_fence_free_rcu(struct rcu_head *rcu)
> @@ -180,6 +186,14 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
> dma_fence_set_deadline(parent, deadline);
> }
>
> +/*
> + * TODO: Both fences implement .release, so dma_fence keeps their ops attached
> + * after signalling. Dropping the callbacks would let dma_fence detach the ops,
> + * after which neither get_timeline_name() nor get_driver_name() can run against
> + * a freed scheduler or an unloaded module - the complete fix. It first requires
> + * auditing every to_drm_sched_fence() caller, since ops-detach makes the helper
> + * return NULL for a signalled fence. See Documentation/gpu/todo.rst.
> + */
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.
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.
Regards,
Christian.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:20 ` Christian König
@ 2026-09-04 8:31 ` Philipp Stanner
2026-09-04 12:49 ` Christian König
2026-09-04 8:31 ` Jonghyuk Kim(MalHyuk)
1 sibling, 1 reply; 34+ messages in thread
From: Philipp Stanner @ 2026-09-04 8:31 UTC (permalink / raw)
To: Christian König, Jonghyuk Kim(MalHyuk),
phasta, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Fri, 2026-09-04 at 10:20 +0200, Christian König wrote:
>
[…]
> >
> > +/*
> > + * TODO: Both fences implement .release, so dma_fence keeps their ops attached
> > + * after signalling. Dropping the callbacks would let dma_fence detach the ops,
> > + * after which neither get_timeline_name() nor get_driver_name() can run against
> > + * a freed scheduler or an unloaded module - the complete fix. It first requires
> > + * auditing every to_drm_sched_fence() caller, since ops-detach makes the helper
> > + * return NULL for a signalled fence. See Documentation/gpu/todo.rst.
> > + */
>
> 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?
The underlying problem is that the driver has no chance to figure out
when the scheduler is actually done with all the sched_fences.
Remember our lengthy discussions about drm_sched_fini(). Maybe we want
to reconsider providing a function with which the driver can wait until
the scheduler is done with all finished_fences?
>
> 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?
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?
P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:31 ` Philipp Stanner
@ 2026-09-04 12:49 ` Christian König
2026-09-04 19:06 ` Philipp Stanner
0 siblings, 1 reply; 34+ messages in thread
From: Christian König @ 2026-09-04 12:49 UTC (permalink / raw)
To: phasta, Jonghyuk Kim(MalHyuk), tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 9/4/26 10:31, Philipp Stanner wrote:
> On Fri, 2026-09-04 at 10:20 +0200, Christian König wrote:
>>
>
> […]
>
>>>
>>> +/*
>>> + * TODO: Both fences implement .release, so dma_fence keeps their ops attached
>>> + * after signalling. Dropping the callbacks would let dma_fence detach the ops,
>>> + * after which neither get_timeline_name() nor get_driver_name() can run against
>>> + * a freed scheduler or an unloaded module - the complete fix. It first requires
>>> + * auditing every to_drm_sched_fence() caller, since ops-detach makes the helper
>>> + * return NULL for a signalled fence. See Documentation/gpu/todo.rst.
>>> + */
>>
>> 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.
We should keep this fix simple and focused so that we can easily backport it.
Fixing all dma_fence implementations to not need the release callback is something I really like to have as well, but not to fix this issue here.
>
> The underlying problem is that the driver has no chance to figure out
> when the scheduler is actually done with all the sched_fences.
>
> Remember our lengthy discussions about drm_sched_fini(). Maybe we want
> to reconsider providing a function with which the driver can wait until
> the scheduler is done with all finished_fences?
The problem is that won't help unless we either add more checks or fix the checks in dma_fence_driver_name()/dma_fence_timeline_name().
The dma_fence object can trivially outlive both the driver and the scheduler instance it originally issued.
>>
>> 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.
> 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.
Regards,
Christian.
>
>
> P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 12:49 ` Christian König
@ 2026-09-04 19:06 ` Philipp Stanner
2026-09-07 9:15 ` Tvrtko Ursulin
2026-09-07 11:42 ` Christian König
0 siblings, 2 replies; 34+ messages in thread
From: Philipp Stanner @ 2026-09-04 19:06 UTC (permalink / raw)
To: Christian König, phasta, Jonghyuk Kim(MalHyuk),
tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
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.
>
> 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?
>
> Fixing all dma_fence implementations to not need the release callback
> is something I really like to have as well, but not to fix this issue
> here.
>
> >
> > The underlying problem is that the driver has no chance to figure out
> > when the scheduler is actually done with all the sched_fences.
> >
> > Remember our lengthy discussions about drm_sched_fini(). Maybe we want
> > to reconsider providing a function with which the driver can wait until
> > the scheduler is done with all finished_fences?
>
> The problem is that won't help unless we either add more checks or
> fix the checks in dma_fence_driver_name()/dma_fence_timeline_name().
>
> The dma_fence object can trivially outlive both the driver and the
> scheduler instance it originally issued.
Right.
>
> > >
> > > 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.
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 :(
> > 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
:)
In the mean time, my proposal is to keep aiming for removing
sched_fence->ops->release and fixing pvr and amdgpu.
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.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 19:06 ` Philipp Stanner
@ 2026-09-07 9:15 ` Tvrtko Ursulin
2026-09-07 9:42 ` Philipp Stanner
2026-09-07 11:42 ` Christian König
1 sibling, 1 reply; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-07 9:15 UTC (permalink / raw)
To: phasta, Christian König, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 04/09/2026 20:06, Philipp Stanner wrote:
8><
> 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
> :)
>
>
> In the mean time, my proposal is to keep aiming for removing
> sched_fence->ops->release and fixing pvr and amdgpu.
Fixing the drivers sounds like an obvious thing to try indeed. Along the
same lines as it was done for xe and panthor. It is an already
established and well understood approach so shouldn't be controversial.
After that we can discuss in leisurely pace if something better is
possible in the scheduler core.
I understand its amdxdna, nouveau, and msm. Was it attempted so far? Is
it significantly more complicated than it was for panthor and xe?
As for regarding the 035219a760ed ("dma-buf: dma-fence: Fix potential
NULL pointer dereference") sub-thread - I did not manage to penetrate
the consensus there - whether it was established that it needs adding
the is signaled check back (with additional memory barriers, like v1 of
that patch) or not? Regardless of fixing the drivers or what?
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 9:15 ` Tvrtko Ursulin
@ 2026-09-07 9:42 ` Philipp Stanner
2026-09-07 9:49 ` Philipp Stanner
` (2 more replies)
0 siblings, 3 replies; 34+ messages in thread
From: Philipp Stanner @ 2026-09-07 9:42 UTC (permalink / raw)
To: Tvrtko Ursulin, phasta, Christian König,
Jonghyuk Kim(MalHyuk),
matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Mon, 2026-09-07 at 10:15 +0100, Tvrtko Ursulin wrote:
>
>
> On 04/09/2026 20:06, Philipp Stanner wrote:
>
> 8><
>
> > 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
> > :)
> >
> >
> > In the mean time, my proposal is to keep aiming for removing
> > sched_fence->ops->release and fixing pvr and amdgpu.
>
> Fixing the drivers sounds like an obvious thing to try indeed. Along the
> same lines as it was done for xe and panthor. It is an already
> established and well understood approach so shouldn't be controversial.
> After that we can discuss in leisurely pace if something better is
> possible in the scheduler core.
>
> I understand its amdxdna, nouveau, and msm. Was it attempted so far? Is
> it significantly more complicated than it was for panthor and xe?
How did the others fix that?
If we look at nouveau:
static void
nouveau_sched_fini(struct nouveau_sched *sched)
{
struct drm_gpu_scheduler *drm_sched = &sched->base;
struct drm_sched_entity *entity = &sched->entity;
wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
drm_sched_entity_fini(entity);
drm_sched_fini(drm_sched);
/* Destroy workqueue after scheduler tear down, otherwise it might still
* be in use.
*/
if (sched->wq)
destroy_workqueue(sched->wq);
}
We see that it
1. stops accepting jobs from userspace (not visible here)
2. waits until all hardware fences in this ring are signaled
3. only then tears down drm_sched
Then nouveau might unload or free up resources.
The nasty thing is that I don't see how nouveau misbehaves here and how
the stuff might be fixed.
The problem is that the sched_fence implements ops->release, so the
check doesn't take effect.
Moreover, even if we did remove ops->release in drm_sched, it would
still be a race: a driver's contract is the hardware_fence, the rule
being that you have to signal those. So after signaling the last
hardware_fence, you could actually start releasing resources, but it
might be that finished_fences are still in-flight and are unsignaled.
So we have some sort of fence -> fence race here, too.
>
> As for regarding the 035219a760ed ("dma-buf: dma-fence: Fix potential
> NULL pointer dereference") sub-thread - I did not manage to penetrate
> the consensus there - whether it was established that it needs adding
> the is signaled check back (with additional memory barriers, like v1 of
> that patch) or not? Regardless of fixing the drivers or what?
As far as my understanding goes this is the only way to get this right
for everyone, i.e. also users who implement ops->release(). Then at
least the driver could unload after signalling all its fences (with the
exception of those who have a shared spinlock maybe).
However, I suppose then we would then have two mechanisms, one dancing
with RCU around the ops pointer, the other checking whether the fence
is signaled, presumably with manual ordering through barriers.
My first guess would be that maybe we should only rely on the signaled-
state and leave the ops-pointer untouched? This should also work for
pvr, notably.
P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 9:42 ` Philipp Stanner
@ 2026-09-07 9:49 ` Philipp Stanner
2026-09-07 10:28 ` Tvrtko Ursulin
2026-09-07 12:18 ` Alessio Belle
2 siblings, 0 replies; 34+ messages in thread
From: Philipp Stanner @ 2026-09-07 9:49 UTC (permalink / raw)
To: phasta, Tvrtko Ursulin, Christian König,
Jonghyuk Kim(MalHyuk),
matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Mon, 2026-09-07 at 11:42 +0200, Philipp Stanner wrote:
> Moreover, even if we did remove ops->release in drm_sched, it would
> still be a race: a driver's contract is the hardware_fence, the rule
> being that you have to signal those. So after signaling the last
> hardware_fence, you could actually start releasing resources, but it
> might be that finished_fences are still in-flight and are unsignaled.
>
> So we have some sort of fence -> fence race here, too.
Forget about that, signaling the hw_fence should always result in
undelayed signaling of the finished-fence.
So it would seem that making the signaled-state instead of ops == NULL
the decoupling point in dma_fence is the solution to go for?
P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 9:42 ` Philipp Stanner
2026-09-07 9:49 ` Philipp Stanner
@ 2026-09-07 10:28 ` Tvrtko Ursulin
2026-09-07 10:34 ` Tvrtko Ursulin
2026-09-07 10:47 ` Philipp Stanner
2026-09-07 12:18 ` Alessio Belle
2 siblings, 2 replies; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-07 10:28 UTC (permalink / raw)
To: phasta, Christian König, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 07/09/2026 10:42, Philipp Stanner wrote:
> On Mon, 2026-09-07 at 10:15 +0100, Tvrtko Ursulin wrote:
>>
>>
>> On 04/09/2026 20:06, Philipp Stanner wrote:
>>
>> 8><
>>
>>> 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
>>> :)
>>>
>>>
>>> In the mean time, my proposal is to keep aiming for removing
>>> sched_fence->ops->release and fixing pvr and amdgpu.
>>
>> Fixing the drivers sounds like an obvious thing to try indeed. Along the
>> same lines as it was done for xe and panthor. It is an already
>> established and well understood approach so shouldn't be controversial.
>> After that we can discuss in leisurely pace if something better is
>> possible in the scheduler core.
>>
>> I understand its amdxdna, nouveau, and msm. Was it attempted so far? Is
>> it significantly more complicated than it was for panthor and xe?
>
> How did the others fix that?
Combination of kfree_rcu, synchronize_rcu and storing the name in an
object protected by those:
6bd90e700b42 ("drm/xe: Make dma-fences compliant with the safe access
rules")
299bc6d50b1b ("drm/xe/guc: Keep scheduler timeline name alive")
efe24898485c ("drm/panthor: fix for dma-fence safe access rules")
Not too complicated on the overall. Simply ensure RCU grace period
between signaling the hw fence and freeing the ops, scheduler, name, all
that it is in the externally accessible dereference chain.
> If we look at nouveau:
>
> static void
> nouveau_sched_fini(struct nouveau_sched *sched)
> {
> struct drm_gpu_scheduler *drm_sched = &sched->base;
> struct drm_sched_entity *entity = &sched->entity;
>
> wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
>
> drm_sched_entity_fini(entity);
> drm_sched_fini(drm_sched);
>
> /* Destroy workqueue after scheduler tear down, otherwise it might still
> * be in use.
> */
> if (sched->wq)
> destroy_workqueue(sched->wq);
> }
>
>
> We see that it
> 1. stops accepting jobs from userspace (not visible here)
> 2. waits until all hardware fences in this ring are signaled
> 3. only then tears down drm_sched
I cannot do a very deep dive into nouveau at the moment. I see fence
itself is already freed with kfree_rcu so that's good. What is reachable
via the timeline name callback:
struct nouveau_fence *fence = to_nouveau_fence(f);
struct nouveau_fence_chan *fctx = nouveau_fctx(fence);
return !fctx->dead ? fctx->name : "dead channel";
Fence is presumably the fence so channel. Chagning to kfree_rcu in
nouveau_fence_context_put() there might be enough for that one.
For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
That makes scheduler timeline name vfunc safe:
static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
{
struct drm_sched_fence *fence = to_drm_sched_fence(f);
return (const char *)fence->sched->name;
sched is then RCU protected. sched->name is already static so not a concern.
As you say nouveau_sched_fini() only tears down the scheduler after
fences have been signaled it seems adding two new kfree_rcu make is safe.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 10:28 ` Tvrtko Ursulin
@ 2026-09-07 10:34 ` Tvrtko Ursulin
2026-09-07 10:47 ` Philipp Stanner
1 sibling, 0 replies; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-07 10:34 UTC (permalink / raw)
To: phasta, Christian König, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 07/09/2026 11:28, Tvrtko Ursulin wrote:
>
> On 07/09/2026 10:42, Philipp Stanner wrote:
>> On Mon, 2026-09-07 at 10:15 +0100, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 04/09/2026 20:06, Philipp Stanner wrote:
>>>
>>> 8><
>>>
>>>> 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
>>>> :)
>>>>
>>>>
>>>> In the mean time, my proposal is to keep aiming for removing
>>>> sched_fence->ops->release and fixing pvr and amdgpu.
>>>
>>> Fixing the drivers sounds like an obvious thing to try indeed. Along the
>>> same lines as it was done for xe and panthor. It is an already
>>> established and well understood approach so shouldn't be controversial.
>>> After that we can discuss in leisurely pace if something better is
>>> possible in the scheduler core.
>>>
>>> I understand its amdxdna, nouveau, and msm. Was it attempted so far? Is
>>> it significantly more complicated than it was for panthor and xe?
>>
>> How did the others fix that?
>
> Combination of kfree_rcu, synchronize_rcu and storing the name in an
> object protected by those:
>
> 6bd90e700b42 ("drm/xe: Make dma-fences compliant with the safe access
> rules")
> 299bc6d50b1b ("drm/xe/guc: Keep scheduler timeline name alive")
> efe24898485c ("drm/panthor: fix for dma-fence safe access rules")
>
> Not too complicated on the overall. Simply ensure RCU grace period
> between signaling the hw fence and freeing the ops, scheduler, name, all
> that it is in the externally accessible dereference chain.
>
>> If we look at nouveau:
>>
>> static void
>> nouveau_sched_fini(struct nouveau_sched *sched)
>> {
>> struct drm_gpu_scheduler *drm_sched = &sched->base;
>> struct drm_sched_entity *entity = &sched->entity;
>>
>> wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
>>
>> drm_sched_entity_fini(entity);
>> drm_sched_fini(drm_sched);
>>
>> /* Destroy workqueue after scheduler tear down, otherwise it might
>> still
>> * be in use.
>> */
>> if (sched->wq)
>> destroy_workqueue(sched->wq);
>> }
>>
>>
>> We see that it
>> 1. stops accepting jobs from userspace (not visible here)
>> 2. waits until all hardware fences in this ring are signaled
>> 3. only then tears down drm_sched
>
> I cannot do a very deep dive into nouveau at the moment. I see fence
> itself is already freed with kfree_rcu so that's good. What is reachable
> via the timeline name callback:
>
> struct nouveau_fence *fence = to_nouveau_fence(f);
> struct nouveau_fence_chan *fctx = nouveau_fctx(fence);
>
> return !fctx->dead ? fctx->name : "dead channel";
>
> Fence is presumably the fence so channel. Chagning to kfree_rcu in
> nouveau_fence_context_put() there might be enough for that one.
>
> For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
>
> That makes scheduler timeline name vfunc safe:
>
> static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
> {
> struct drm_sched_fence *fence = to_drm_sched_fence(f);
> return (const char *)fence->sched->name;
>
> sched is then RCU protected. sched->name is already static so not a
> concern.
>
> As you say nouveau_sched_fini() only tears down the scheduler after
> fences have been signaled it seems adding two new kfree_rcu make is safe.
P.S. Idea on how to test it from userspace:
https://patchwork.freedesktop.org/patch/642709/?series=146211&rev=2
But as nouveau does not export via sync_file you would need to adapt to
export sync_file from syncobj.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 10:28 ` Tvrtko Ursulin
2026-09-07 10:34 ` Tvrtko Ursulin
@ 2026-09-07 10:47 ` Philipp Stanner
2026-09-07 11:06 ` Tvrtko Ursulin
1 sibling, 1 reply; 34+ messages in thread
From: Philipp Stanner @ 2026-09-07 10:47 UTC (permalink / raw)
To: Tvrtko Ursulin, phasta, Christian König,
Jonghyuk Kim(MalHyuk),
matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Mon, 2026-09-07 at 11:28 +0100, Tvrtko Ursulin wrote:
> >
[…]
> > How did the others fix that?
>
> Combination of kfree_rcu, synchronize_rcu and storing the name in an
> object protected by those:
[…]
> For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
>
> That makes scheduler timeline name vfunc safe:
>
> static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
> {
> struct drm_sched_fence *fence = to_drm_sched_fence(f);
> return (const char *)fence->sched->name;
>
> sched is then RCU protected. sched->name is already static so not a concern.
>
> As you say nouveau_sched_fini() only tears down the scheduler after
> fences have been signaled it seems adding two new kfree_rcu make is safe.
I don't see how any RCU mechanism would make anything here safe.
As long as ops->release is implemented, ops will never be set to NULL,
(with or without RCU) – so everyone who holds a reference to finished-
fence can still run into drm_sched_fence_get_timeline_name() and cause
UAF on the sched pointer *and* the name pointer.
Moreover, all of this falls apart should the driver unload.
So I maintain the position that we only get it right by having the
signaled-bit be the decoupling point.
Or am I missing something?
P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 10:47 ` Philipp Stanner
@ 2026-09-07 11:06 ` Tvrtko Ursulin
2026-09-07 11:15 ` Philipp Stanner
2026-09-07 12:28 ` Tvrtko Ursulin
0 siblings, 2 replies; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-07 11:06 UTC (permalink / raw)
To: phasta, Christian König, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 07/09/2026 11:47, Philipp Stanner wrote:
> On Mon, 2026-09-07 at 11:28 +0100, Tvrtko Ursulin wrote:
>>>
>
> […]
>
>>> How did the others fix that?
>>
>> Combination of kfree_rcu, synchronize_rcu and storing the name in an
>> object protected by those:
>
> […]
>
>> For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
>>
>> That makes scheduler timeline name vfunc safe:
>>
>> static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)
>> {
>> struct drm_sched_fence *fence = to_drm_sched_fence(f);
>> return (const char *)fence->sched->name;
>>
>> sched is then RCU protected. sched->name is already static so not a concern.
>>
>> As you say nouveau_sched_fini() only tears down the scheduler after
>> fences have been signaled it seems adding two new kfree_rcu make is safe.
>
>
> I don't see how any RCU mechanism would make anything here safe.
>
> As long as ops->release is implemented, ops will never be set to NULL,
> (with or without RCU) – so everyone who holds a reference to finished-
> fence can still run into drm_sched_fence_get_timeline_name() and cause
> UAF on the sched pointer *and* the name pointer.
>
> Moreover, all of this falls apart should the driver unload.
>
> So I maintain the position that we only get it right by having the
> signaled-bit be the decoupling point.
>
> Or am I missing something?
The driver fixes you asked about and I listed work(ed) in the context of
506aa8b02a8d ("dma-fence: Add safe access helpers and document the
rules"). In that "world" it would have been safe. Did 035219a760ed
("dma-buf: dma-fence: Fix potential NULL pointer dereference") break
those fixes a bit?
I can try my IGT and see..
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 11:06 ` Tvrtko Ursulin
@ 2026-09-07 11:15 ` Philipp Stanner
2026-09-07 12:59 ` Christian König
2026-09-07 12:28 ` Tvrtko Ursulin
1 sibling, 1 reply; 34+ messages in thread
From: Philipp Stanner @ 2026-09-07 11:15 UTC (permalink / raw)
To: Tvrtko Ursulin, phasta, Christian König,
Jonghyuk Kim(MalHyuk),
matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Mon, 2026-09-07 at 12:06 +0100, Tvrtko Ursulin wrote:
> On 07/09/2026 11:47, Philipp Stanner wrote:
[…]
> >
> > So I maintain the position that we only get it right by having the
> > signaled-bit be the decoupling point.
> >
> > Or am I missing something?
>
> The driver fixes you asked about and I listed work(ed) in the context of
> 506aa8b02a8d ("dma-fence: Add safe access helpers and document the
> rules"). In that "world" it would have been safe. Did 035219a760ed
> ("dma-buf: dma-fence: Fix potential NULL pointer dereference") break
> those fixes a bit?
I think we all agree that 035219a760ed should not have been implemented
in that way because it does not work for drivers that provide a ops-
>release / ops->wait.
So I think the only thing we can do right now is add the signaled-check
back (like Christian suggested) and ensure correct memory ordering –
since we also cannot use the spinlock consistently, as that can still
be extern…
P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 11:15 ` Philipp Stanner
@ 2026-09-07 12:59 ` Christian König
2026-09-07 13:38 ` Philipp Stanner
0 siblings, 1 reply; 34+ messages in thread
From: Christian König @ 2026-09-07 12:59 UTC (permalink / raw)
To: phasta, Tvrtko Ursulin, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 9/7/26 13:15, Philipp Stanner wrote:
> On Mon, 2026-09-07 at 12:06 +0100, Tvrtko Ursulin wrote:
>> On 07/09/2026 11:47, Philipp Stanner wrote:
>
> […]
>
>>>
>>> So I maintain the position that we only get it right by having the
>>> signaled-bit be the decoupling point.
>>>
>>> Or am I missing something?
>>
>> The driver fixes you asked about and I listed work(ed) in the context of
>> 506aa8b02a8d ("dma-fence: Add safe access helpers and document the
>> rules"). In that "world" it would have been safe. Did 035219a760ed
>> ("dma-buf: dma-fence: Fix potential NULL pointer dereference") break
>> those fixes a bit?
>
> I think we all agree that 035219a760ed should not have been implemented
> in that way because it does not work for drivers that provide a ops-
>> release / ops->wait.
>
> So I think the only thing we can do right now is add the signaled-check
> back (like Christian suggested) and ensure correct memory ordering –
> since we also cannot use the spinlock consistently, as that can still
> be extern…
I think the memory ordering isn't really a problem.
See if the ops pointer or the signaled bit is loaded first doesn't matter if you check both.
You only need to make sure that the ops pointer is loaded once cause that one is used multiple times, but that is already the case by using rcu_dereference().
Or what exactly is your concern?
Thanks,
Christian.
>
>
> P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 12:59 ` Christian König
@ 2026-09-07 13:38 ` Philipp Stanner
2026-09-07 15:21 ` Christian König
0 siblings, 1 reply; 34+ messages in thread
From: Philipp Stanner @ 2026-09-07 13:38 UTC (permalink / raw)
To: Christian König, phasta, Tvrtko Ursulin,
Jonghyuk Kim(MalHyuk),
matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Mon, 2026-09-07 at 14:59 +0200, Christian König wrote:
> I think the memory ordering isn't really a problem.
>
> See if the ops pointer or the signaled bit is loaded first doesn't
> matter if you check both.
>
> You only need to make sure that the ops pointer is loaded once cause
> that one is used multiple times, but that is already the case by
> using rcu_dereference().
That.. sounds correct.
Although I still don't get why we then can't use the signaled state
everywhere instead of the ops pointer. That would be simpler.
Anyways.
Maybe Jonghyuk can give your patch a test run and then we could use it
as a hot-fix to backport and discuss the wider future of dma-fence
separately?
P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 13:38 ` Philipp Stanner
@ 2026-09-07 15:21 ` Christian König
2026-09-08 10:49 ` Jonghyuk Kim(MalHyuk)
0 siblings, 1 reply; 34+ messages in thread
From: Christian König @ 2026-09-07 15:21 UTC (permalink / raw)
To: phasta, Tvrtko Ursulin, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 9/7/26 15:38, Philipp Stanner wrote:
> On Mon, 2026-09-07 at 14:59 +0200, Christian König wrote:
>> I think the memory ordering isn't really a problem.
>>
>> See if the ops pointer or the signaled bit is loaded first doesn't
>> matter if you check both.
>>
>> You only need to make sure that the ops pointer is loaded once cause
>> that one is used multiple times, but that is already the case by
>> using rcu_dereference().
>
> That.. sounds correct.
>
> Although I still don't get why we then can't use the signaled state
> everywhere instead of the ops pointer. That would be simpler.
Yeah I have considered that as well.
But I wanted a) to let trace_dma_fence_signaled() be able to trace timeline and driver name for the last time and b) not keep an invalid ops pointer around.
Setting ops to NULL caused some trouble but I think we have solved most of that now.
> Anyways.
> Maybe Jonghyuk can give your patch a test run and then we could use it
> as a hot-fix to backport and discuss the wider future of dma-fence
> separately?
Completely agree.
Regards,
Christian.
>
>
> P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 15:21 ` Christian König
@ 2026-09-08 10:49 ` Jonghyuk Kim(MalHyuk)
2026-09-08 11:07 ` Philipp Stanner
0 siblings, 1 reply; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-08 10:49 UTC (permalink / raw)
To: christian.koenig, phasta, tursulin, matthew.brost, dakr
Cc: Jonghyuk Kim, dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 07/09/2026 17:21, Christian König wrote:
>> Maybe Jonghyuk can give your patch a test run and then we could use
>> it as a hot-fix to backport
>
> Completely agree.
Ran 0001 on v7.3-rc1-99-g89a312991dc6 with the KUnit regression test from my
v4 3/3 (mock scheduler, KASAN, no hardware). Without it the test fails with
BUG: KASAN: slab-use-after-free in drm_sched_fence_get_timeline_name+0x9c/0xb0
and with it applied it passes. Reverted and re-applied twice, same both times.
Tested-by: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>
Two things it doesn't cover: it's x86 only, so nothing about the load ordering
you discussed; and only the signaled case - for a fence exported before it
signals, get_timeline_name() is still reached and reads fence->sched->name.
I haven't tried to build that case.
Could you add a Reported-by for me when you post it? I'm happy to drop my
v4 1/3 in favour of this, and can respin the KUnit test standalone so the
fix lands with a regression test.
Thanks,
Jonghyuk
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-08 10:49 ` Jonghyuk Kim(MalHyuk)
@ 2026-09-08 11:07 ` Philipp Stanner
2026-09-09 0:37 ` Jonghyuk Kim(MalHyuk)
0 siblings, 1 reply; 34+ messages in thread
From: Philipp Stanner @ 2026-09-08 11:07 UTC (permalink / raw)
To: Jonghyuk Kim(MalHyuk),
christian.koenig, phasta, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Tue, 2026-09-08 at 19:49 +0900, Jonghyuk Kim(MalHyuk) wrote:
> Tested-by: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>
>
> Two things it doesn't cover: it's x86 only, so nothing about the load ordering
> you discussed;
Christian and I think that it shouldn't be an issue anymore. Both
values are loaded and the ops pointer cannot become invalid until an
RCU grace period has passed. Similarly, sched must not be cleaned up
before that.
> and only the signaled case - for a fence exported before it
> signals, get_timeline_name() is still reached and reads fence->sched->name.
Is this an issue? A fence can only be exported if the scheduler exists.
The hard rule with dma_fence is that all drivers must signal all of
them before they tear down the scheduler and clean up driver resources.
This then does the decoupling.
I think that rule is really the only chance we have to get things
right.
> I haven't tried to build that case.
>
> Could you add a Reported-by for me when you post it? I'm happy to drop my
> v4 1/3 in favour of this, and can respin the KUnit test standalone so the
> fix lands with a regression test.
+1
I think since it's @Christian's patch he'll take care of it
Thanks
P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-08 11:07 ` Philipp Stanner
@ 2026-09-09 0:37 ` Jonghyuk Kim(MalHyuk)
2026-09-09 7:44 ` Philipp Stanner
0 siblings, 1 reply; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-09 0:37 UTC (permalink / raw)
To: phasta, christian.koenig, tursulin, matthew.brost, dakr
Cc: Jonghyuk Kim, dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 08/09/2026 13:07, Philipp Stanner wrote:
> Is this an issue? A fence can only be exported if the scheduler exists.
> The hard rule with dma_fence is that all drivers must signal all of
> them before they tear down the scheduler
Agreed for the bug at hand - that fence is signaled, so 0001 covers it.
My point was narrower: nothing enforces the rule. drm_sched_fini() only does
if (!list_empty(&sched->pending_list))
dev_warn(sched->dev, "Tearing down scheduler while jobs are pending!\n");
and the one path that would drain the list, drm_sched_cancel_remaining_jobs(),
needs ops->cancel_job, which no driver in current mainline implements -
the only user is the mock scheduler in the KUnit tests. So a driver that
gets it wrong gets a warning, not a stopped teardown. Not an argument
against the hot-fix.
Thanks,
Jonghyuk
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-09 0:37 ` Jonghyuk Kim(MalHyuk)
@ 2026-09-09 7:44 ` Philipp Stanner
0 siblings, 0 replies; 34+ messages in thread
From: Philipp Stanner @ 2026-09-09 7:44 UTC (permalink / raw)
To: Jonghyuk Kim(MalHyuk),
phasta, christian.koenig, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Wed, 2026-09-09 at 09:37 +0900, Jonghyuk Kim(MalHyuk) wrote:
> On 08/09/2026 13:07, Philipp Stanner wrote:
> > Is this an issue? A fence can only be exported if the scheduler exists.
> > The hard rule with dma_fence is that all drivers must signal all of
> > them before they tear down the scheduler
>
> Agreed for the bug at hand - that fence is signaled, so 0001 covers it.
>
> My point was narrower: nothing enforces the rule. drm_sched_fini() only does
>
> if (!list_empty(&sched->pending_list))
> dev_warn(sched->dev, "Tearing down scheduler while jobs are pending!\n");
>
> and the one path that would drain the list, drm_sched_cancel_remaining_jobs(),
> needs ops->cancel_job, which no driver in current mainline implements -
> the only user is the mock scheduler in the KUnit tests. So a driver that
> gets it wrong gets a warning, not a stopped teardown. Not an argument
> against the hot-fix.
True. The cancel_job() cb is currently the recommended solution
(although there was disagreement back then) that Tvrtko and I came up
with a while ago. The issue was that drm_sched was designed with no
idiomatic solution for handling remaining jobs in sched->pending_list
on teardown, which is why all drivers presumably have different
solutions. cancel_job() was an attempt at providing an idiomatic
solution.
It's afaik currently being used by Asahi downstream and was used in
Nouveau, but Nouveau doesn't need it / cannot use it for other reasons
that have to do with page table cleanup AFAIR. So Nouveau covers it
with a waitqueue. We could add it to Nouveau again, but then it would
never be called because the waitqueue comes first and needs to stay…
So should you see a driver that could make good use of it, I would
appreciate if you'd try to add it :)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 11:06 ` Tvrtko Ursulin
2026-09-07 11:15 ` Philipp Stanner
@ 2026-09-07 12:28 ` Tvrtko Ursulin
2026-09-08 15:20 ` Tvrtko Ursulin
1 sibling, 1 reply; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-07 12:28 UTC (permalink / raw)
To: phasta, Christian König, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 07/09/2026 12:06, Tvrtko Ursulin wrote:
>
> On 07/09/2026 11:47, Philipp Stanner wrote:
>> On Mon, 2026-09-07 at 11:28 +0100, Tvrtko Ursulin wrote:
>>>>
>>
>> […]
>>
>>>> How did the others fix that?
>>>
>>> Combination of kfree_rcu, synchronize_rcu and storing the name in an
>>> object protected by those:
>>
>> […]
>>
>>> For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
>>>
>>> That makes scheduler timeline name vfunc safe:
>>>
>>> static const char *drm_sched_fence_get_timeline_name(struct dma_fence
>>> *f)
>>> {
>>> struct drm_sched_fence *fence = to_drm_sched_fence(f);
>>> return (const char *)fence->sched->name;
>>>
>>> sched is then RCU protected. sched->name is already static so not a
>>> concern.
>>>
>>> As you say nouveau_sched_fini() only tears down the scheduler after
>>> fences have been signaled it seems adding two new kfree_rcu make is
>>> safe.
>>
>>
>> I don't see how any RCU mechanism would make anything here safe.
>>
>> As long as ops->release is implemented, ops will never be set to NULL,
>> (with or without RCU) – so everyone who holds a reference to finished-
>> fence can still run into drm_sched_fence_get_timeline_name() and cause
>> UAF on the sched pointer *and* the name pointer.
>>
>> Moreover, all of this falls apart should the driver unload.
>>
>> So I maintain the position that we only get it right by having the
>> signaled-bit be the decoupling point.
>>
>> Or am I missing something?
>
> The driver fixes you asked about and I listed work(ed) in the context of
> 506aa8b02a8d ("dma-fence: Add safe access helpers and document the
> rules"). In that "world" it would have been safe. Did 035219a760ed
> ("dma-buf: dma-fence: Fix potential NULL pointer dereference") break
> those fixes a bit?
>
> I can try my IGT and see..
With RCU frees in xe it is harder to hit it reliably for KASAN to
notice. Guess I would need to have a way to flush the RCU callbacks from
IGT but I did not bother with that, it seemed easier to check the
returned name. Since it is not returning "detached-driver" /
"detached-timeline" post signalling that proves things are indeed broken:
$ sudo tests/xe_sync_file
IGT-Version: 2.5-ge37a85b91 (x86_64) (Linux: 7.3.0-rc1+ x86_64)
Using IGT_SRANDOM=1788784104 for randomisation
Opened device: /dev/dri/card0
Starting subtest: sync_file_race
[2178.517756] (xe_sync_file:10970) CRITICAL: Test assertion failure
function test_race, file ../tests/intel/xe_sync_file.c:130:
[2178.517790] (xe_sync_file:10970) CRITICAL: Failed assertion:
!strcmp(driver_name, "detached-driver")
Stack trace:
#0 ../lib/igt_core.c:2089 __igt_fail_assert()
#1 ../tests/intel/xe_sync_file.c:155 __igt_unique____real_main136()
#2 ../tests/intel/xe_sync_file.c:136 main()
#3 ../sysdeps/nptl/libc_start_call_main.h:83 __libc_start_call_main()
#4 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
#5 [_start+0x25]
Subtest sync_file_race failed.
**** DEBUG ****
[2177.516393] (xe_sync_file:10970) DEBUG: 'drm_sched'/'rcs16' = 1
[2178.517745] (xe_sync_file:10970) DEBUG: 'drm_sched'/'rcs16' = 1
[2178.517756] (xe_sync_file:10970) CRITICAL: Test assertion failure
function test_race, file ../tests/intel/xe_sync_file.c:130:
[2178.517790] (xe_sync_file:10970) CRITICAL: Failed assertion:
!strcmp(driver_name, "detached-driver")
[2178.518762] (xe_sync_file:10970) igt_core-INFO: Stack trace:
[2178.523313] (xe_sync_file:10970) igt_core-INFO: #0
../lib/igt_core.c:2089 __igt_fail_assert()
[2178.523428] (xe_sync_file:10970) igt_core-INFO: #1
../tests/intel/xe_sync_file.c:155 __igt_unique____real_main136()
[2178.523436] (xe_sync_file:10970) igt_core-INFO: #2
../tests/intel/xe_sync_file.c:136 main()
[2178.564635] (xe_sync_file:10970) igt_core-INFO: #3
../sysdeps/nptl/libc_start_call_main.h:83 __libc_start_call_main()
[2178.564690] (xe_sync_file:10970) igt_core-INFO: #4
../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
[2178.564892] (xe_sync_file:10970) igt_core-INFO: #5 [_start+0x25]
**** END ****
Subtest sync_file_race: FAIL (1.054s)
I'll copy you on the updated IGT for reference.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 12:28 ` Tvrtko Ursulin
@ 2026-09-08 15:20 ` Tvrtko Ursulin
0 siblings, 0 replies; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-08 15:20 UTC (permalink / raw)
To: phasta, Christian König, Jonghyuk Kim(MalHyuk), matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On 07/09/2026 13:28, Tvrtko Ursulin wrote:
>
> On 07/09/2026 12:06, Tvrtko Ursulin wrote:
>>
>> On 07/09/2026 11:47, Philipp Stanner wrote:
>>> On Mon, 2026-09-07 at 11:28 +0100, Tvrtko Ursulin wrote:
>>>>>
>>>
>>> […]
>>>
>>>>> How did the others fix that?
>>>>
>>>> Combination of kfree_rcu, synchronize_rcu and storing the name in an
>>>> object protected by those:
>>>
>>> […]
>>>
>>>> For the scheduler (nouveau_sched_destroy()) the same, kfree_rcu.
>>>>
>>>> That makes scheduler timeline name vfunc safe:
>>>>
>>>> static const char *drm_sched_fence_get_timeline_name(struct
>>>> dma_fence *f)
>>>> {
>>>> struct drm_sched_fence *fence = to_drm_sched_fence(f);
>>>> return (const char *)fence->sched->name;
>>>>
>>>> sched is then RCU protected. sched->name is already static so not a
>>>> concern.
>>>>
>>>> As you say nouveau_sched_fini() only tears down the scheduler after
>>>> fences have been signaled it seems adding two new kfree_rcu make is
>>>> safe.
>>>
>>>
>>> I don't see how any RCU mechanism would make anything here safe.
>>>
>>> As long as ops->release is implemented, ops will never be set to NULL,
>>> (with or without RCU) – so everyone who holds a reference to finished-
>>> fence can still run into drm_sched_fence_get_timeline_name() and cause
>>> UAF on the sched pointer *and* the name pointer.
>>>
>>> Moreover, all of this falls apart should the driver unload.
>>>
>>> So I maintain the position that we only get it right by having the
>>> signaled-bit be the decoupling point.
>>>
>>> Or am I missing something?
>>
>> The driver fixes you asked about and I listed work(ed) in the context
>> of 506aa8b02a8d ("dma-fence: Add safe access helpers and document the
>> rules"). In that "world" it would have been safe. Did 035219a760ed
>> ("dma-buf: dma-fence: Fix potential NULL pointer dereference") break
>> those fixes a bit?
>>
>> I can try my IGT and see..
>
> With RCU frees in xe it is harder to hit it reliably for KASAN to
> notice. Guess I would need to have a way to flush the RCU callbacks from
> IGT but I did not bother with that, it seemed easier to check the
> returned name. Since it is not returning "detached-driver" / "detached-
> timeline" post signalling that proves things are indeed broken:
>
> $ sudo tests/xe_sync_file
> IGT-Version: 2.5-ge37a85b91 (x86_64) (Linux: 7.3.0-rc1+ x86_64)
> Using IGT_SRANDOM=1788784104 for randomisation
> Opened device: /dev/dri/card0
> Starting subtest: sync_file_race
> [2178.517756] (xe_sync_file:10970) CRITICAL: Test assertion failure
> function test_race, file ../tests/intel/xe_sync_file.c:130:
> [2178.517790] (xe_sync_file:10970) CRITICAL: Failed assertion: !
> strcmp(driver_name, "detached-driver")
> Stack trace:
> #0 ../lib/igt_core.c:2089 __igt_fail_assert()
> #1 ../tests/intel/xe_sync_file.c:155 __igt_unique____real_main136()
> #2 ../tests/intel/xe_sync_file.c:136 main()
> #3 ../sysdeps/nptl/libc_start_call_main.h:83 __libc_start_call_main()
> #4 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
> #5 [_start+0x25]
> Subtest sync_file_race failed.
> **** DEBUG ****
> [2177.516393] (xe_sync_file:10970) DEBUG: 'drm_sched'/'rcs16' = 1
> [2178.517745] (xe_sync_file:10970) DEBUG: 'drm_sched'/'rcs16' = 1
> [2178.517756] (xe_sync_file:10970) CRITICAL: Test assertion failure
> function test_race, file ../tests/intel/xe_sync_file.c:130:
> [2178.517790] (xe_sync_file:10970) CRITICAL: Failed assertion: !
> strcmp(driver_name, "detached-driver")
> [2178.518762] (xe_sync_file:10970) igt_core-INFO: Stack trace:
> [2178.523313] (xe_sync_file:10970) igt_core-INFO: #0 ../lib/
> igt_core.c:2089 __igt_fail_assert()
> [2178.523428] (xe_sync_file:10970) igt_core-INFO: #1 ../tests/intel/
> xe_sync_file.c:155 __igt_unique____real_main136()
> [2178.523436] (xe_sync_file:10970) igt_core-INFO: #2 ../tests/intel/
> xe_sync_file.c:136 main()
> [2178.564635] (xe_sync_file:10970) igt_core-INFO: #3 ../sysdeps/nptl/
> libc_start_call_main.h:83 __libc_start_call_main()
> [2178.564690] (xe_sync_file:10970) igt_core-INFO: #4 ../csu/libc-
> start.c:128 __libc_start_main@@GLIBC_2.34()
> [2178.564892] (xe_sync_file:10970) igt_core-INFO: #5 [_start+0x25]
> **** END ****
> Subtest sync_file_race: FAIL (1.054s)
>
> I'll copy you on the updated IGT for reference.
For reference Intel's CI managed to catch the UAF using the test I
re-posted yesterday:
https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_15796/shard-bmg-9/igt@xe_sync_file@sync_file_race.html
<6> [107.688696] [IGT] xe_sync_file: executing
<6> [107.711053] [IGT] xe_sync_file: starting subtest sync_file_race
<4> [108.719407] Oops: general protection fault, probably for
non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP NOPTI
<4> [108.730119] CPU: 11 UID: 0 PID: 4227 Comm: xe_sync_file Tainted: G
S U 7.3.0-rc2-lgci-xe-xe-5698-547f981c369d4b8ad-debug+ #1
PREEMPT(lazy)
<4> [108.743915] Tainted: [S]=CPU_OUT_OF_SPEC, [U]=USER
<4> [108.748699] Hardware name: ASUS System Product Name/PRIME Z790-P
WIFI, BIOS 1645 03/15/2024
<4> [108.757038] RIP: 0010:strnlen+0x17/0x50
<4> [108.760873] Code: 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90
90 90 90 48 8d 14 37 48 89 f8 48 85 f6 75 0b eb 2d 48 83 c0 01 48 39 c2
74 13 <80> 38 00 75 f2 48 29 f8 31 d2 31 f6 31 ff c3 cc cc cc cc 48 89 d0
<4> [108.779603] RSP: 0018:ffffc9000414fcc8 EFLAGS: 00010202
<4> [108.784827] RAX: 6b6b6b6b6b6b6b6b RBX: 0000000000000050 RCX:
0000000000000000
<4> [108.791954] RDX: 6b6b6b6b6b6b6b8b RSI: 0000000000000020 RDI:
6b6b6b6b6b6b6b6b
<4> [108.799079] RBP: ffffc9000414fd90 R08: 0000000000000000 R09:
0000000000000000
<4> [108.806201] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8881098231a0
<4> [108.813329] R13: 0000000000000020 R14: ffff88816a833ab8 R15:
6b6b6b6b6b6b6b6b
<4> [108.820454] FS: 00007de5028bebc0(0000) GS:ffff8888d5fdb000(0000)
knlGS:0000000000000000
<4> [108.828530] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4> [108.834268] CR2: 00007de5028bc000 CR3: 000000012c9b7001 CR4:
0000000000f72ef0
<4> [108.841396] PKRU: 55555554
<4> [108.844104] Call Trace:
<4> [108.846554] <TASK>
<4> [108.848656] ? sync_file_ioctl+0x1ed/0x750
<4> [108.852754] ? sync_file_ioctl+0x153/0x750
<4> [108.856850] __x64_sys_ioctl+0xa5/0x100
<4> [108.860691] x64_sys_call+0x2b5/0x27e0
<4> [108.864439] do_syscall_64+0xea/0x670
<4> [108.868101] ? do_syscall_64+0x224/0x670
<4> [108.872024] ? trace_hardirqs_on_prepare+0xcb/0xf0
<4> [108.876812] ? do_syscall_64+0x224/0x670
<4> [108.880736] ? trace_hardirqs_on_prepare+0xcb/0xf0
<4> [108.885522] ? trace_hardirqs_off_finish+0xc7/0xe0
<4> [108.890307] ? do_syscall_64+0x58/0x670
<4> [108.894143] entry_SYSCALL_64_after_hwframe+0x76/0x7e
<4> [108.899190] RIP: 0033:0x7de504724f1d
<4> [108.902764] Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10
c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 00
0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 00 00 00
<4> [108.921497] RSP: 002b:00007ffdc9fd5e50 EFLAGS: 00000246 ORIG_RAX:
0000000000000010
<4> [108.929059] RAX: ffffffffffffffda RBX: 00007ffdc9fd5fd8 RCX:
00007de504724f1d
<4> [108.936182] RDX: 00007ffdc9fd5ee0 RSI: 00000000c0383e04 RDI:
0000000000000006
<4> [108.943310] RBP: 00007ffdc9fd5ea0 R08: 0000000000000000 R09:
0000000000000000
<4> [108.950435] R10: 00007ffdc9fd5f60 R11: 0000000000000246 R12:
00007ffdc9fd5ee0
<4> [108.957558] R13: 00000000c0383e04 R14: 0000000000000006 R15:
0000000000000005
<4> [108.964686] </TASK>
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 9:42 ` Philipp Stanner
2026-09-07 9:49 ` Philipp Stanner
2026-09-07 10:28 ` Tvrtko Ursulin
@ 2026-09-07 12:18 ` Alessio Belle
2 siblings, 0 replies; 34+ messages in thread
From: Alessio Belle @ 2026-09-07 12:18 UTC (permalink / raw)
To: phasta, tursulin, christian.koenig, malhyuk97
Cc: dri-devel, matthew.brost, stable, dakr, linux-kernel, mdaenzer,
Luigi Santivetti
Hi,
On Mon, 2026-09-07 at 11:42 +0200, Philipp Stanner wrote:
> On Mon, 2026-09-07 at 10:15 +0100, Tvrtko Ursulin wrote:
> >
> >
> > On 04/09/2026 20:06, Philipp Stanner wrote:
> >
> > 8><
> >
> > > 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
> > > :)
> > >
> > >
> > > In the mean time, my proposal is to keep aiming for removing
> > > sched_fence->ops->release and fixing pvr and amdgpu.
> >
> > Fixing the drivers sounds like an obvious thing to try indeed. Along the
> > same lines as it was done for xe and panthor. It is an already
> > established and well understood approach so shouldn't be controversial.
> > After that we can discuss in leisurely pace if something better is
> > possible in the scheduler core.
> >
> > I understand its amdxdna, nouveau, and msm. Was it attempted so far? Is
> > it significantly more complicated than it was for panthor and xe?
>
> How did the others fix that?
>
> If we look at nouveau:
>
> static void
> nouveau_sched_fini(struct nouveau_sched *sched)
> {
> struct drm_gpu_scheduler *drm_sched = &sched->base;
> struct drm_sched_entity *entity = &sched->entity;
>
> wait_event(sched->job.wq, nouveau_sched_job_list_empty(sched));
>
> drm_sched_entity_fini(entity);
> drm_sched_fini(drm_sched);
>
> /* Destroy workqueue after scheduler tear down, otherwise it might still
> * be in use.
> */
> if (sched->wq)
> destroy_workqueue(sched->wq);
> }
>
>
> We see that it
> 1. stops accepting jobs from userspace (not visible here)
> 2. waits until all hardware fences in this ring are signaled
> 3. only then tears down drm_sched
>
> Then nouveau might unload or free up resources.
>
> The nasty thing is that I don't see how nouveau misbehaves here and how
> the stuff might be fixed.
>
> The problem is that the sched_fence implements ops->release, so the
> check doesn't take effect.
>
> Moreover, even if we did remove ops->release in drm_sched, it would
> still be a race: a driver's contract is the hardware_fence, the rule
> being that you have to signal those. So after signaling the last
> hardware_fence, you could actually start releasing resources, but it
> might be that finished_fences are still in-flight and are unsignaled.
>
> So we have some sort of fence -> fence race here, too.
>
> >
> > As for regarding the 035219a760ed ("dma-buf: dma-fence: Fix potential
> > NULL pointer dereference") sub-thread - I did not manage to penetrate
> > the consensus there - whether it was established that it needs adding
> > the is signaled check back (with additional memory barriers, like v1 of
> > that patch) or not? Regardless of fixing the drivers or what?
>
>
> As far as my understanding goes this is the only way to get this right
> for everyone, i.e. also users who implement ops->release(). Then at
> least the driver could unload after signalling all its fences (with the
> exception of those who have a shared spinlock maybe).
>
>
> However, I suppose then we would then have two mechanisms, one dancing
> with RCU around the ops pointer, the other checking whether the fence
> is signaled, presumably with manual ordering through barriers.
>
> My first guess would be that maybe we should only rely on the signaled-
> state and leave the ops-pointer untouched? This should also work for
> pvr, notably.
It seems in the meantime the discussion moved towards fixing the common code, at
least in the short term (correct?), but pointing out anyway that if needed,
having a similar fix on the powervr side would also be fine for us especially
for backporting purposes.
We are looking at alternatives to avoid accessing sched_fence->sched->ops, but
can't tell yet if it can be done without loss of functionality and/or
performance and how safe it is to backport.
Thanks,
Alessio
>
>
> P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 19:06 ` Philipp Stanner
2026-09-07 9:15 ` Tvrtko Ursulin
@ 2026-09-07 11:42 ` Christian König
2026-09-07 11:54 ` Philipp Stanner
1 sibling, 1 reply; 34+ messages in thread
From: Christian König @ 2026-09-07 11:42 UTC (permalink / raw)
To: phasta, Jonghyuk Kim(MalHyuk), tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
[-- 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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-07 11:42 ` Christian König
@ 2026-09-07 11:54 ` Philipp Stanner
0 siblings, 0 replies; 34+ messages in thread
From: Philipp Stanner @ 2026-09-07 11:54 UTC (permalink / raw)
To: Christian König, phasta, Jonghyuk Kim(MalHyuk),
tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, stable
On Mon, 2026-09-07 at 13:42 +0200, Christian König wrote:
> 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.
035219a760ed literally fixed a race condition for weakly ordered
platforms, so I wouldn't say that "everything worked correctly" :)
>
[…]
> >
> >
> > 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.
Agreed, we should address the problem there.
But see my other answer to Tvrtko. I think if the decoupling point is
the signaled bit anyways, we can and should stop setting ops to NULL in
the first place.
Because now we'd have two decoupling points.
> > > >
> >
> > I hope that wasn't me because that again looks very racy.
>
> Why? Tvrtko added the RCU protection for that.
RCU does not address ordering between signaled bit and ops pointer.
> > 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.
>
Patch 0001 seems to reintroduce the race condition. No one guarantees
that the CPU will load the signaled flag before the ops.
P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:20 ` Christian König
2026-09-04 8:31 ` Philipp Stanner
@ 2026-09-04 8:31 ` Jonghyuk Kim(MalHyuk)
2026-09-04 8:39 ` Philipp Stanner
2026-09-04 9:07 ` Tvrtko Ursulin
1 sibling, 2 replies; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-04 8:31 UTC (permalink / raw)
To: christian.koenig, phasta, tursulin, matthew.brost, dakr
Cc: Jonghyuk Kim(MalHyuk),
dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti
On 9/4/26 10:20, Christian König wrote:
>> + return fence->sched_name;
>
> I don't think that this actually solves the problem, the sched_name still
> needs to be kept alive until all fences are destroyed and that is something
> drivers don't want/can do.
Agreed, and that is the same objection Tvrtko raised against v1. Caching the
pointer only moves the lifetime requirement from the scheduler to the string,
and the documentation hunk I added just pushes that requirement onto drivers.
I will drop that patch.
>> +/*
>> + * TODO: Both fences implement .release, so dma_fence keeps their ops attached
>> + * after signalling. Dropping the callbacks would let dma_fence detach the ops,
>
> 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.
Understood - ops-detach is about producer/module decoupling, not about the
scheduler's lifetime relative to the driver, so framing it as "the complete
fix" for this bug was wrong. I will drop the TODO patch as well.
> I think we should rather re-consider patch 035219a760edb35ae9a9e96beba7f122e26a997b
> ("dma-buf: dma-fence: Fix potential NULL pointer dereference"):
> [...]
> 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.
That matches what I see in the code, thanks - this is the actual root cause and
it is not drm/sched specific.
dma_fence_signal_timestamp_locked() only clears the ops pointer for fences that
carry neither .release nor .wait:
ops = rcu_dereference_protected(fence->ops, true);
if (!ops->release && !ops->wait)
RCU_INIT_POINTER(fence->ops, NULL);
drm_sched_fence implements .release, so its ops survive signalling. Before
035219a760ed the helpers gated on the signaled bit, so such a fence returned
the static string and the producer callback was never reached. Since that
commit they gate on the ops pointer alone, so get_timeline_name() /
get_driver_name() are called on a long-signalled fence - which is exactly the
window my report hits, with ->sched already freed.
To be clear, I am not suggesting a revert: 035219a760ed fixes a real problem,
namely that "set signaled bit, then NULL the ops" and "load ops, then check the
signaled bit" can be reordered on weakly ordered platforms, and using the ops
pointer as the synchronization point solves that elegantly. That property
should stay.
What seems to be missing is that the ops check answers "may I dereference the
pointer", not "may I call into the producer". The dma-fence rules say the
latter is not allowed once the fence is signalled, so I think both conditions
are needed:
ops = rcu_dereference(fence->ops);
if (ops && !dma_fence_test_signaled_flag(fence))
return (const char __rcu *)ops->get_timeline_name(fence);
else
return (const char __rcu *)"signaled-timeline";
The ops load keeps the RCU/ordering guarantee from 035219a760ed, and the
signaled check restores the contract. That fixes every implementation which
keeps .release or .wait and assumes it is not called after signalling, rather
than just drm/sched, and it puts no lifetime burden on drivers.
Philipp, since 035219a760ed is yours - do you agree with adding the signaled
check back on top of the ops check? I would rather have your ack on that before
I respin.
One thing I noticed while checking the callers: the tracepoints in
include/trace/events/dma_fence.h and drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
call fence->ops->get_driver_name() / get_timeline_name() directly instead of
going through the helpers, so they are not covered by the above. That looks
like a pre-existing and much narrower exposure (tracing only), but let me know
if you want it addressed in the same series or separately.
So for v5 I plan:
1. dma-buf/dma-fence: add the signaled check back to dma_fence_driver_name()
and dma_fence_timeline_name(), Fixes: 035219a760ed, Cc: stable.
2. Keep the KUnit regression test - it exercises exactly this path through
dma_fence_timeline_name() and needs no change; it also picked up the
teardown issue the review bot flagged, which I have fixed locally by
using kunit_add_action_or_reset() + kunit_release_action().
and drop the drm/sched caching and TODO patches. I will wait for your and
Philipp's input before sending it.
Thanks,
Jonghyuk
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:31 ` Jonghyuk Kim(MalHyuk)
@ 2026-09-04 8:39 ` Philipp Stanner
2026-09-04 9:11 ` Jonghyuk Kim(MalHyuk)
2026-09-04 9:07 ` Tvrtko Ursulin
1 sibling, 1 reply; 34+ messages in thread
From: Philipp Stanner @ 2026-09-04 8:39 UTC (permalink / raw)
To: Jonghyuk Kim(MalHyuk),
christian.koenig, phasta, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle, luigi.santivetti
On Fri, 2026-09-04 at 17:31 +0900, Jonghyuk Kim(MalHyuk) wrote:
> On 9/4/26 10:20, Christian König wrote:
> Understood - ops-detach is about producer/module decoupling, not about the
> scheduler's lifetime relative to the driver, so framing it as "the complete
> fix" for this bug was wrong. I will drop the TODO patch as well.
Please wait a bit until we discussed it for a bit.
>
> > I think we should rather re-consider patch 035219a760edb35ae9a9e96beba7f122e26a997b
> > ("dma-buf: dma-fence: Fix potential NULL pointer dereference"):
> > [...]
> > 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.
>
> That matches what I see in the code, thanks - this is the actual root cause and
> it is not drm/sched specific.
>
> dma_fence_signal_timestamp_locked() only clears the ops pointer for fences that
> carry neither .release nor .wait:
>
> ops = rcu_dereference_protected(fence->ops, true);
> if (!ops->release && !ops->wait)
> RCU_INIT_POINTER(fence->ops, NULL);
>
> drm_sched_fence implements .release, so its ops survive signalling. Before
> 035219a760ed the helpers gated on the signaled bit, so such a fence returned
> the static string and the producer callback was never reached. Since that
> commit they gate on the ops pointer alone, so get_timeline_name() /
> get_driver_name() are called on a long-signalled fence - which is exactly the
> window my report hits, with ->sched already freed.
>
> To be clear, I am not suggesting a revert: 035219a760ed fixes a real problem,
> namely that "set signaled bit, then NULL the ops" and "load ops, then check the
> signaled bit" can be reordered on weakly ordered platforms, and using the ops
> pointer as the synchronization point solves that elegantly. That property
> should stay.
>
> What seems to be missing is that the ops check answers "may I dereference the
> pointer", not "may I call into the producer". The dma-fence rules say the
> latter is not allowed once the fence is signalled, so I think both conditions
> are needed:
>
> ops = rcu_dereference(fence->ops);
> if (ops && !dma_fence_test_signaled_flag(fence))
> return (const char __rcu *)ops->get_timeline_name(fence);
> else
> return (const char __rcu *)"signaled-timeline";
>
> The ops load keeps the RCU/ordering guarantee from 035219a760ed, and the
> signaled check restores the contract. That fixes every implementation which
> keeps .release or .wait and assumes it is not called after signalling, rather
> than just drm/sched, and it puts no lifetime burden on drivers.
>
> Philipp, since 035219a760ed is yours - do you agree with adding the signaled
> check back on top of the ops check? I would rather have your ack on that before
> I respin.
That commit has to stay because it solves an ordering problem on some
architectures.
I do agree to having a conversation with Christian in my other mail,
where I wonder whether it would be better if a driver does the is-
signaled check, holding the lock.
These things are extremely complicated and cannot be rushed. We
discussed dma_fence synchronization for weeks this summer and concluded
that there is no easy solution at hands for some problems, notably
because of callbacks and legacy callbacks, and also because Christian
and I disagree on to what degree a fence should be locked.
>
> One thing I noticed while checking the callers: the tracepoints in
> include/trace/events/dma_fence.h and drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> call fence->ops->get_driver_name() / get_timeline_name() directly instead of
> going through the helpers, so they are not covered by the above. That looks
> like a pre-existing and much narrower exposure (tracing only), but let me know
> if you want it addressed in the same series or separately.
Do you generate part of your mails with an LLM? It seems to be a huge
corpus for 11min of time between receiving and answer
P.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:39 ` Philipp Stanner
@ 2026-09-04 9:11 ` Jonghyuk Kim(MalHyuk)
0 siblings, 0 replies; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-04 9:11 UTC (permalink / raw)
To: phasta, christian.koenig, tursulin, matthew.brost, dakr
Cc: Jonghyuk Kim(MalHyuk),
dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti
On Fri, 2026-09-04 at 10:39 +0200, Philipp Stanner wrote:
> Please wait a bit until we discussed it for a bit.
Understood. No respin from me until you and Christian have settled the
direction.
> Do you generate part of your mails with an LLM? It seems to be a huge
> corpus for 11min of time between receiving and answer
Yes, I draft them with LLM assistance. I should have said so up front,
sorry. What I do not delegate is the checking: I verify the claims against
the tree before sending, and I am answerable for whatever goes out under my
name. The bug report itself came out of my own KASAN testing.
Your point about the volume is fair, and there is a concrete example of why
you are right to push back. In the v4 commit message I asserted that "every
in-tree driver passes a string literal". That is wrong: panthor kasprintf()s
the name (panthor_sched.c:3591) and kfree()s it in group_free_queue(). So
Christian's objection to caching holds for an in-tree driver today, not just
in theory, and I should have caught that before sending rather than after.
I will keep the mails shorter and slower.
Tvrtko, thanks for the quick answer on the tracepoints - the comment makes it
clear those call sites are fine, so I will drop that point.
Jonghyuk
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:31 ` Jonghyuk Kim(MalHyuk)
2026-09-04 8:39 ` Philipp Stanner
@ 2026-09-04 9:07 ` Tvrtko Ursulin
1 sibling, 0 replies; 34+ messages in thread
From: Tvrtko Ursulin @ 2026-09-04 9:07 UTC (permalink / raw)
To: Jonghyuk Kim(MalHyuk), christian.koenig, phasta, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle, luigi.santivetti
To address only one narrow point quickly, I haven't had the time to
catch up with the thread yet:
On 04/09/2026 09:31, Jonghyuk Kim(MalHyuk) wrote:
8><
> One thing I noticed while checking the callers: the tracepoints in
> include/trace/events/dma_fence.h and drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
> call fence->ops->get_driver_name() / get_timeline_name() directly instead of
> going through the helpers, so they are not covered by the above. That looks
> like a pre-existing and much narrower exposure (tracing only), but let me know
> if you want it addressed in the same series or separately.
For the dma_fence.h see the comment:
/*
* Safe only for call sites which are guaranteed to not race with fence
* signaling, holding the fence->lock and having checked for not
signaled, or
* the signaling path itself.
*
* TODO: Remove the need for this event class when drivers switch to
independent
* fences.
*/
These tracepoints call the name vfuncs directly: dma_fence_init,
dma_fence_enable_signal, dma_fence_signaled. So those are supposed to be
safe as per comment and changing them to use the helper would create a
different issue (can't remember from the top of my head which one - but
the thread is moving fast so I wanted to reply quickly).
amdgpu_trace.h also look safe on a quick look because they are called
before the job which can signal the fence is submitted.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 8:06 ` [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free Jonghyuk Kim(MalHyuk)
2026-09-04 8:20 ` Christian König
@ 2026-09-04 9:57 ` Danilo Krummrich
2026-09-04 10:51 ` Philipp Stanner
1 sibling, 1 reply; 34+ messages in thread
From: Danilo Krummrich @ 2026-09-04 9:57 UTC (permalink / raw)
To: Jonghyuk Kim(MalHyuk)
Cc: phasta, christian.koenig, tursulin, matthew.brost, dri-devel,
linux-kernel, mdaenzer, alessio.belle, luigi.santivetti, stable
On Fri Sep 4, 2026 at 10:06 AM CEST, Jonghyuk Kim(MalHyuk) wrote:
> Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the rules")
> Cc: stable@vger.kernel.org # we don't know since when
How is that? If there's a Fixes: tag it expresses that this commit introduced
the bug, so it should be known exactly since when it is present.
However, I don't think this is the correct Fixes: tag, the bug was present
before. I think it was present all the way back to commit f556cb0caeec
("drm/amd: add scheduler fence implementation (v2)").
It is also not the case that it wasn't a bug before we had per-context
schedulers, it's just that it became much more likely to trigger with
per-context schedulers; nothing prevents the kernel from having a reference
count of a DMA fence after the driver was unbound and hence the
per-device/per-ring scheduler was torn down.
More in general, putting "we don't know since when" for the stable folks is
rough. If neither the author of the patch nor the maintainers of the subsystem
can figure it out, how are the stable folks supposed to?
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free
2026-09-04 9:57 ` Danilo Krummrich
@ 2026-09-04 10:51 ` Philipp Stanner
0 siblings, 0 replies; 34+ messages in thread
From: Philipp Stanner @ 2026-09-04 10:51 UTC (permalink / raw)
To: Danilo Krummrich, Jonghyuk Kim(MalHyuk)
Cc: phasta, christian.koenig, tursulin, matthew.brost, dri-devel,
linux-kernel, mdaenzer, alessio.belle, luigi.santivetti, stable
On Fri, 2026-09-04 at 11:57 +0200, Danilo Krummrich wrote:
> On Fri Sep 4, 2026 at 10:06 AM CEST, Jonghyuk Kim(MalHyuk) wrote:
> > Fixes: 506aa8b02a8d ("dma-fence: Add safe access helpers and document the rules")
> > Cc: stable@vger.kernel.org # we don't know since when
>
> How is that?
That is referring to the fact that it only is a reachable bug if the
respective driver exposes a mechanism that runs into
get_timeline_name().
>
> More in general, putting "we don't know since when" for the stable folks is
> rough. If neither the author of the patch nor the maintainers of the subsystem
> can figure it out, how are the stable folks supposed to?
That's on me, it was a joking phrase by me that Jonghyuk or his LLM
picked up 1:1.
Anyways. So would you suggest backporting a fix as far as possible to
the amd origin commit? That might not be backportable that far, so
stable would still have to figure it out.
P.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v4 2/3] drm/sched: add the fence ops-detach cleanup to the TODO list
2026-09-04 8:06 [PATCH v4 0/3] drm/sched: fix use-after-free of the fence timeline name Jonghyuk Kim(MalHyuk)
2026-09-04 8:06 ` [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free Jonghyuk Kim(MalHyuk)
@ 2026-09-04 8:06 ` Jonghyuk Kim(MalHyuk)
2026-09-04 8:06 ` [PATCH v4 3/3] drm/sched/tests: add a UAF regression test for the timeline name Jonghyuk Kim(MalHyuk)
2 siblings, 0 replies; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-04 8:06 UTC (permalink / raw)
To: phasta, christian.koenig, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, Jonghyuk Kim(MalHyuk)
The previous patch caches the timeline name so that get_timeline_name() no
longer dereferences a scheduler that a userspace-held fence has outlived.
That is a targeted fix: the underlying reason the callback is reachable at
all is that both drm_sched fences implement .release, so dma_fence never
detaches their ops on signalling. get_driver_name() has the same exposure
for module unload.
Dropping the .release callbacks is the complete fix, but it requires
auditing every to_drm_sched_fence() caller (ops-detach makes it return NULL
for signalled fences), a different identity mechanism for
pvr_queue_fence_is_native(), and a rework of the shared allocation's
reference handling. Record that as a TODO entry so the cleanup is not lost.
Suggested-by: Philipp Stanner <phasta@kernel.org>
Signed-off-by: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>
---
Documentation/gpu/todo.rst | 39 ++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 14cf37590fc7..284aeba3c752 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -990,6 +990,45 @@ Contact:
Level: Beginner
+Detach the scheduler fence ops on signalling
+--------------------------------------------
+
+The dma-fence contract forbids touching driver-provided data - everything
+reachable through &dma_fence.ops - once a fence is signalled. dma_fence enforces
+that by detaching a fence's ops on signalling, but only for fences that carry
+neither a .release nor a .wait callback (see
+dma_fence_signal_timestamp_locked()).
+
+Both drm_sched fences implement .release, so their ops stay attached forever.
+That leaves the callbacks reachable on a long-signalled fence that userspace
+still holds through a sync_file or drm_syncobj, even after the scheduler is
+gone: get_timeline_name() used to dereference the freed &drm_sched_fence.sched
+(fixed by caching the name), and get_driver_name() can still return a string
+literal belonging to a module that has since been unloaded.
+
+Dropping the .release callbacks so that the ops are detached on signalling is
+the complete fix, and it is what the dma-fence rules ask for. It is not
+straightforward:
+
+Tasks:
+
+- Audit every to_drm_sched_fence() caller. Detaching the ops makes the helper
+ return NULL for a signalled fence, and callers such as
+ amdgpu_cs_p2_dependencies() and amdgpu_ctx_fence_time() dereference the result
+ unconditionally.
+- drm/imagination uses the ops pointer as an identity test in
+ pvr_queue_fence_is_native(); that needs a different mechanism.
+- Rework the reference handling. The scheduled and the finished fence share one
+ allocation, and the finished fence's .release currently drops the scheduled
+ fence's reference, so the callbacks cannot simply be deleted.
+
+Contact:
+
+- Philipp Stanner <phasta@kernel.org>
+- Christian König <christian.koenig@amd.com>
+
+Level: Advanced
+
Outside DRM
===========
--
2.43.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v4 3/3] drm/sched/tests: add a UAF regression test for the timeline name
2026-09-04 8:06 [PATCH v4 0/3] drm/sched: fix use-after-free of the fence timeline name Jonghyuk Kim(MalHyuk)
2026-09-04 8:06 ` [PATCH v4 1/3] drm/sched: cache the timeline name to fix a use-after-free Jonghyuk Kim(MalHyuk)
2026-09-04 8:06 ` [PATCH v4 2/3] drm/sched: add the fence ops-detach cleanup to the TODO list Jonghyuk Kim(MalHyuk)
@ 2026-09-04 8:06 ` Jonghyuk Kim(MalHyuk)
2 siblings, 0 replies; 34+ messages in thread
From: Jonghyuk Kim(MalHyuk) @ 2026-09-04 8:06 UTC (permalink / raw)
To: phasta, christian.koenig, tursulin, matthew.brost, dakr
Cc: dri-devel, linux-kernel, mdaenzer, alessio.belle,
luigi.santivetti, Jonghyuk Kim(MalHyuk)
Add a KUnit test that reproduces the drm_sched_fence timeline-name
use-after-free fixed by the previous patch. It submits a job on the mock
scheduler, takes an independent reference on the finished fence (standing
in for a userspace sync_file), lets the job finish, frees the scheduler,
and then queries the timeline name through dma_fence_timeline_name().
Without the fix get_timeline_name() dereferences fence->sched of the freed
scheduler and KASAN reports a slab-use-after-free read in
drm_sched_fence_get_timeline_name(); with the fix the name was cached at
init and the freed scheduler is never touched.
The test needs no hardware - it exercises the drm_sched core through the
existing mock scheduler under KASAN. Per review it lives in a new
tests_integration.c rather than in tests_basic.c, since it is about the
scheduler's interaction with the dma-fence API rather than scheduler
behaviour in isolation.
Signed-off-by: Jonghyuk Kim(MalHyuk) <malhyuk97@gmail.com>
---
drivers/gpu/drm/scheduler/tests/Makefile | 1 +
.../drm/scheduler/tests/tests_integration.c | 88 +++++++++++++++++++
2 files changed, 89 insertions(+)
create mode 100644 drivers/gpu/drm/scheduler/tests/tests_integration.c
diff --git a/drivers/gpu/drm/scheduler/tests/Makefile b/drivers/gpu/drm/scheduler/tests/Makefile
index 9ec185fbbc15..10abe07c06d2 100644
--- a/drivers/gpu/drm/scheduler/tests/Makefile
+++ b/drivers/gpu/drm/scheduler/tests/Makefile
@@ -3,6 +3,7 @@
drm-sched-tests-y := \
mock_scheduler.o \
tests_basic.o \
+ tests_integration.o \
tests_scheduler.o
obj-$(CONFIG_DRM_SCHED_KUNIT_TEST) += drm-sched-tests.o
diff --git a/drivers/gpu/drm/scheduler/tests/tests_integration.c b/drivers/gpu/drm/scheduler/tests/tests_integration.c
new file mode 100644
index 000000000000..4a2d5571440d
--- /dev/null
+++ b/drivers/gpu/drm/scheduler/tests/tests_integration.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-fence.h>
+#include <linux/rcupdate.h>
+
+#include "sched_tests.h"
+
+/*
+ * Integration-style regression tests that exercise the interaction between the
+ * DRM scheduler and the dma-fence API, rather than scheduler behaviour in
+ * isolation.
+ */
+
+/*
+ * Reproduce the drm_sched_fence timeline-name use-after-free.
+ *
+ * drm_sched_fence_get_timeline_name() used to dereference fence->sched->name.
+ * A driver may free a per-context/per-queue/per-VM drm_gpu_scheduler while
+ * userspace still holds the exported ->finished fence (via sync_file /
+ * drm_syncobj). Querying the timeline name afterwards must not touch the freed
+ * scheduler.
+ *
+ * Without the fix this reads fence->sched->name from freed slab memory and
+ * KASAN reports a slab-use-after-free in drm_sched_fence_get_timeline_name();
+ * with the fix the name is cached at init and the freed scheduler is never
+ * dereferenced. Same class as CVE-2025-38703 (drm/xe) and CVE-2025-71302
+ * (drm/panthor).
+ */
+static void drm_sched_dma_fence_uaf(struct kunit *test)
+{
+ struct drm_mock_sched_entity *entity;
+ struct drm_mock_scheduler *sched;
+ struct drm_mock_sched_job *job;
+ struct dma_fence *finished;
+ const char __rcu *name;
+ bool done;
+
+ sched = drm_mock_sched_new(test, MAX_SCHEDULE_TIMEOUT);
+ entity = drm_mock_sched_entity_new(test, DRM_SCHED_PRIORITY_NORMAL,
+ sched);
+ job = drm_mock_sched_job_new(test, entity);
+
+ /* The s_fence is only created by drm_sched_job_arm(). */
+ drm_mock_sched_job_submit(job);
+
+ /* Independent reference on the finished fence == userspace sync_file. */
+ finished = dma_fence_get(&job->base.s_fence->finished);
+
+ /* Let the job get scheduled (hw fence created), then signal + finish. */
+ done = drm_mock_sched_job_wait_scheduled(job, HZ);
+ KUNIT_ASSERT_TRUE(test, done);
+ drm_mock_sched_advance(sched, 1);
+ done = drm_mock_sched_job_wait_finished(job, HZ);
+ KUNIT_ASSERT_TRUE(test, done);
+
+ /*
+ * Free the per-context scheduler while the finished fence is held.
+ * kunit_kfree() releases the backing memory immediately (rather than at
+ * test teardown) so that fence->sched becomes a dangling pointer now.
+ */
+ drm_mock_sched_entity_free(entity);
+ drm_mock_sched_fini(sched);
+ kunit_kfree(test, sched);
+
+ /*
+ * Query the timeline name of the now-stale fence. With the fix the name
+ * was cached at init, so the freed scheduler is not dereferenced;
+ * without it this is a use-after-free read of the freed scheduler.
+ */
+ rcu_read_lock();
+ name = dma_fence_timeline_name(finished);
+ KUNIT_EXPECT_NOT_NULL(test, name);
+ rcu_read_unlock();
+
+ dma_fence_put(finished);
+}
+
+static struct kunit_case drm_sched_dma_fence_tests[] = {
+ KUNIT_CASE(drm_sched_dma_fence_uaf),
+ {}
+};
+
+static struct kunit_suite drm_sched_dma_fence = {
+ .name = "drm_sched_dma_fence_uaf_tests",
+ .test_cases = drm_sched_dma_fence_tests,
+};
+
+kunit_test_suite(drm_sched_dma_fence);
--
2.43.0
^ permalink raw reply [flat|nested] 34+ messages in thread