Re: [PATCH v2] drm/nouveau: disable the fence uevent work instead of just cancelling it

[lyude]

(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, lyude@xxxxxxxxxx wrote:
> This makes sense to me.
> 
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
> 
> 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 <marek@xxxxxxxxxxxxx>
> > 
> > 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 <sashiko-bot@xxxxxxxxxx>
> > Closes:
> > https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@xxxxxxxxx/
> > Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to
> > workqueue")
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.10.x
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Marek Czernohous <marek@xxxxxxxxxxxxx>
> > ---
> > 
> > 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/20260815200914.8A1131F000E9@xxxxxxxxxxxxxxx/
> >  - 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/178682366002.3748010.12779628082366287968@xxxxxxxxx/
> > 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