From: Marek Czernohous <[email protected]> Three teardown fixes in nouveau, all the same shape: something that can still run after the thing it points at has been torn down or freed.
Two of them are not my finding. The Sashiko review bot flagged them as pre-existing issues in its review of my nv04 FIFO series, https://lore.kernel.org/nouveau/[email protected]/ naming nouveau_fence_context_del() and nouveau_connector_destroy() directly. It was right about both. 1/3 and 2/3 carry a Reported-by accordingly. 3/3 is mine, found while following the irq_work of 2/3 into its handler, which is nouveau_dp_irq(). 1/3 nouveau_fence_context_del() cancels the uevent work first and drops the event afterwards. In between, the event is still armed and nouveau_fence_wait_uevent_handler() queues the work unconditionally, so a non-stall interrupt in that window re-arms the work that was just cancelled. The callers free the context immediately after, which leaves nouveau_fence_uevent_work() walking freed memory. Destroy the event first, then drain. 2/3 nouveau_connector_destroy() drops the connector's two events but never drains nv_connector->irq_work, which is what the DP IRQ event schedules. The work can then run against a connector that is about to be, or has already been, freed. 3/3 nouveau_dp_irq() looks the encoder up and dereferences it in the declaration block, five lines above the NULL test that the same function already carries. 2/3 and 3/3 both point at the same commit. Commit 773eb04d14a1 ("drm/nouveau/disp: expose conn event class") turned nouveau_dp_irq() into a work callback, and that single change introduced both the undrained work and the early dereference: the drm pointer used to be an argument, and recovering it from the encoder put a dereference above the existing test. All three carry Fixes: and Cc: stable. 1/3 and 2/3 are use-after-free windows, and each commit message names the trigger, the window, and the freed object the work then touches. 3/3 is a NULL dereference sitting above the function's own NULL test. I also looked one level up, since it would have been the obvious next instance. drm->hpd_work is drained in nouveau_display_fini(), right after the hotplug events are blocked, under "if (!runtime && !drm->headless)". That guard does not exempt the teardown path: nouveau_drm.c:597 calls nouveau_display_fini(dev, false, false) immediately before nouveau_display_destroy(), so runtime is false there. The runtime exemption belongs to the suspend path (nouveau_display.c:781), which frees nothing. So there is no fourth patch here. Testing Reference hardware: Apple Macmini3,1, MCP79 / GeForce 9400M (NVAC), Core 2 Duo, Wayland (labwc). Note for 1/3 that this chip takes the nv84_fence path, which is the one where the event exists at all. Build. The series is built against the stated base commit, as a full kernel build rather than a module-only one, so modpost actually resolved the module's symbols instead of being skipped for want of Module.symvers: zero compiler warnings, zero compiler errors, nouveau.ko produced. checkpatch.pl --strict is clean on all three patches and on this cover. What the testing does not show, stated plainly: I have not managed to hit any of these three windows deliberately on this hardware. They are ordering bugs reasoned out from the source rather than from a reproduction, and I would rather say that than dress up a crash I do not have. Each patch names the file and the function it argues from so the reasoning can be checked directly. AI assistance Lyude asked on an earlier thread whether these patches were written by a human and pointed at Documentation/process/coding-assistants.rst. The answer, repeated here for the archive: this work is AI assisted. I use Claude (claude-opus-5) as a coding and analysis assistant. Every patch carries an Assisted-by trailer accordingly, and no Signed-off-by is added by the tool. Nature of the assistance, so you can calibrate your review: the assistant did most of the code archaeology and drafting. I described symptoms, asked for the mechanism to be traced in the source rather than guessed, and asked for each claim to be backed by a file and a line. The assistant also reviewed its own drafts adversarially, which is how two errors in 1/3 were caught before this posting: an earlier draft claimed nouveau_fence_context_kill() does not touch the event, which the source contradicts, and it illustrated the freeing caller with nv04_fence_context_del(), which is precisely the case that cannot reach the bug, since nouveau_fence_context_new() returns before nvif_event_ctor() unless nv84_fence_create() set priv->uevent. Both are corrected. I reviewed the result, I understand the code, and I take responsibility for it. Marek Czernohous (3): drm/nouveau: destroy the fence event before cancelling its work drm/nouveau: cancel the DP IRQ work before freeing the connector drm/nouveau: don't dereference outp before checking it in nouveau_dp_irq drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/nouveau/nouveau_dp.c | 4 +++- drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86 -- 2.54.0
