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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.