(I misspoke, this patch ended up in drm-misc-next instead as fixes is
currently closed)
On Thu, 2026-08-20 at 13:55 -0400, [email protected] wrote:
> This makes sense to me.
>
> Reviewed-by: Lyude Paul <[email protected]>
>
> Will push to drm-misc-next-fixes in just a moment
>
> On Sun, 2026-08-16 at 14:58 +0200, Marek Czernohous wrote:
> > From: Marek Czernohous <[email protected]>
> >
> > nouveau_fence_context_del() drains the uevent work while the event
> > that
> > feeds it is still armed:
> >
> > cancel_work_sync(&fctx->uevent_work);
> > nouveau_fence_context_kill(fctx, 0);
> > nvif_event_dtor(&fctx->event);
> >
> > nouveau_fence_wait_uevent_handler() queues the work
> > unconditionally:
> >
> > schedule_work(&fctx->uevent_work);
> > return NVIF_EVENT_KEEP;
> >
> > so a non-stall interrupt arriving after cancel_work_sync() has
> > returned
> > re-arms the work that was just drained. The window closes in two
> > steps,
> > neither of which is the drain. The kill blocks the event when the
> > last
> > fence holding a notify_ref is signalled, which reaches
> > atomic_xchg(&ntfy->allowed, 0) (nvkm/core/event.c:104), and
> > nvkm_event_ntfy() skips a ntfy that is not allowed (:183). That
> > stops
> > further handlers from starting, but not one that is already inside
> > nvkm_event_ntfy(): the event is created with wait = false
> > (nouveau_fence.c:201), so nvkm_event_ntfy_block_() leaves it on the
> > list
> > and never takes event->list_lock. Only nvif_event_dtor() waits
> > that
> > one
> > out: nvkm_event_ntfy_del() (:141) goes through
> > nvkm_event_ntfy_remove(),
> > which takes write_lock_irq() on that same list_lock (:84).
> >
> > Either way the re-arm happens after the drain, and the caller drops
> > its
> > reference immediately afterwards, for example
> > nv84_fence_context_del():
> >
> > nouveau_fence_context_del(&fctx->base);
> > chan->fence = NULL;
> > nouveau_fence_context_free(&fctx->base);
> >
> > That is a kref_put() on fctx->fence_ref, so the context outlives
> > the
> > teardown only while emitted fences still hold a reference of their
> > own.
> > That is no safety net: whenever none do, the count reaches zero
> > right
> > there and nouveau_fence_context_put() kfree()s fctx while the work
> > is
> > still queued. &fctx->uevent_work is embedded in that allocation,
> > so
> > the
> > workqueue already dereferences freed memory when it picks the item
> > up,
> > and nouveau_fence_uevent_work() can then take fctx->lock on it.
> > With
> > CONFIG_DEBUG_OBJECTS_WORK and CONFIG_DEBUG_OBJECTS_FREE, kfree() of
> > a
> > still-queued work item is reported as the free of an active object.
> >
> > On live memory the re-armed work has nothing left to do:
> > nouveau_fence_context_kill() empties fctx->pending and sets fctx-
> > > killed
> > under fctx->lock, nouveau_fence_emit() then returns -ENODEV rather
> > than
> > queueing anything new, and nouveau_fence_update() only reaches
> > nvif_event_block() if it signalled something off that list. The
> > defect
> > is the access to freed memory, not what the work would have found.
> >
> > Only chips from G84 on can reach this at all:
> > nouveau_fence_context_new()
> > returns before nvif_event_ctor() when priv->uevent is clear, and
> > nv84_fence_create() is the only place that sets it.
> > nv84_fence_context_del() is the context_del for all of those,
> > because
> > nvc0_fence_create() and gv100_fence_create() build on
> > nv84_fence_create()
> > and override only context_new.
> >
> > Use disable_work_sync() instead. It drains the work exactly like
> > cancel_work_sync() does, and additionally increments the work
> > item's
> > disable count, after which "any attempt to queue @work will fail
> > and
> > return %false" (kernel/workqueue.c, disable_work()). The handler's
> > schedule_work() then has nothing to re-arm, and the teardown order
> > stays
> > as it is.
> >
> > Draining a second time after nvif_event_dtor() would close the
> > window
> > as
> > well, and without the newer API: once nvkm_event_ntfy_remove() has
> > returned, no handler can start or still be running, so nothing re-
> > arms
> > the work past that point. disable_work_sync() is preferred here
> > because
> > it needs one synchronisation point instead of two, it keeps the
> > work
> > from being queued at all rather than cleaning up after it, and it
> > is
> > what drm has settled on for this (drm/xe, drm/panthor,
> > drm_pagemap).
> > Blocking the event rather than the work is not an option: fctx-
> > >event
> > is
> > created with wait = false, so a handler already inside
> > nvkm_event_ntfy()
> > can still queue the work.
> >
> > Note for backports: disable_work_sync() arrived in v6.10 with
> > commit 86898fa6b8cd ("workqueue: Implement disable/enable for
> > (delayed)
> > work items"), while the fix being corrected here reached 6.6.18 and
> > 6.7.6. linux-6.6.y therefore carries this bug without the API, and
> > this
> > patch would apply there and then fail to build. A 6.6.y backport
> > wants
> > the second drain described above instead, as its own patch.
> >
> > Reported-by: sashiko-bot <[email protected]>
> > Closes:
> > https://lore.kernel.org/nouveau/[email protected]/
> > Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to
> > workqueue")
> > Cc: <[email protected]> # 6.10.x
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Marek Czernohous <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Do not reorder the teardown. v1 moved nvif_event_dtor() ahead
> > of
> > nouveau_fence_context_kill(); with the fences still unsignalled
> > that
> > leaves nouveau_fence_enable_signaling() reachable, and
> > nvif_event_constructed() is a plain unlocked read of object-
> > > client,
> > so the dtor could race an nvif_event_allow() already past that
> > check.
> > Reported as [Critical] by the bot, and withdrawn:
> >
> > https://lore.kernel.org/all/[email protected]/
> > - Change cancel_work_sync() to disable_work_sync() instead, which
> > leaves
> > every ordering alone.
> > - Pin the damage down. Both versions call it a use-after-free;
> > this
> > one
> > adds that &fctx->uevent_work is embedded in the freed
> > allocation,
> > and
> > that the re-armed work has nothing left to do on a live context,
> > so
> > the access to freed memory is the whole of it.
> > - Correct the backport note. v1 claimed no longterm tree sat in
> > the
> > gap
> > between the bug and disable_work_sync(); 6.6.y does. The stable
> > tag is
> > annotated accordingly.
> >
> > This replaces 1/3 of
> > https://lore.kernel.org/all/[email protected]/
> > 2/3 and 3/3 of that series are unaffected and still stand.
> >
> > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c
> > b/drivers/gpu/drm/nouveau/nouveau_fence.c
> > index edbe9e08ba0f..11e95c37ce50 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
> > @@ -96,7 +96,7 @@ nouveau_fence_context_kill(struct
> > nouveau_fence_chan *fctx, int error)
> > void
> > nouveau_fence_context_del(struct nouveau_fence_chan *fctx)
> > {
> > - cancel_work_sync(&fctx->uevent_work);
> > + disable_work_sync(&fctx->uevent_work);
> > nouveau_fence_context_kill(fctx, 0);
> > nvif_event_dtor(&fctx->event);
> > fctx->dead = 1;
> >
> > base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.