Thank you for all of the submissions! JFYI - I should be able to get to reviewing this one tomorrow
On Sat, 2026-08-15 at 21:54 +0200, Marek Czernohous wrote: > 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
