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
