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

[lyude@redhat.com]

This makes sense to me.

Reviewed-by: Lyude Paul <lyude@redhat.com>

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@czernohous.de>
> 
> 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@kernel.org>
> Closes:
> https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gmail.com/
> Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to
> workqueue")
> Cc: <stable@vger.kernel.org> # 6.10.x
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Marek Czernohous <marek@czernohous.de>
> ---
> 
> 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@smtp.kernel.org/
>  - 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@gmail.com/
> 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