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
--
2.54.0
