(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
