[PATCH 0/3] drm/nouveau: teardown ordering fixes for events and work

[Marek Czernohous <mczernohous@gmail.com>]

From: Marek Czernohous <marek@czernohous.de>

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/20260812231330.705425-1-mczernohous@gmail.com/

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