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

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

[Marek Czernohous]

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

[PATCH 1/3] drm/nouveau: destroy the fence event before cancelling its work

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nouveau_fence_context_del() cancels the uevent work first and only tears
the event down afterwards:

	cancel_work_sync(&fctx->uevent_work);
	nouveau_fence_context_kill(fctx, 0);
	nvif_event_dtor(&fctx->event);

Between the cancel and the dtor the event is still armed, and
nouveau_fence_wait_uevent_handler() queues the work unconditionally:

	schedule_work(&fctx->uevent_work);
	return NVIF_EVENT_KEEP;

So a non-stall interrupt arriving in that window re-arms the work that
was just cancelled. The callers free the context immediately afterwards,
for example nv84_fence_context_del():

	nouveau_fence_context_del(&fctx->base);
	chan->fence = NULL;
	nouveau_fence_context_free(&fctx->base);

nouveau_fence_uevent_work() then runs against freed memory, taking
fctx->lock and walking fctx->pending.

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.

Drop the event first, so no further work can be queued, and only then
drain what is already queued.

nouveau_fence_context_kill() keeps its place after the drain. It can
still touch the event: nouveau_fence_signal() returns true when a fence
that had enable_signaling() called on it is signalled and
fctx->notify_ref drops to zero, and the loop then calls
nvif_event_block() once. That call runs in the same thread just after
the dtor, where nvif_event_constructed() is false and it is a no-op.
Blocking an event that no longer exists would be pointless anyway.

The reordering does open one window, so it is worth saying what closes
it. A fence holder that reaches nouveau_fence_enable_signaling()
between the dtor and the kill gets a silent no-op from
nvif_event_allow(), so that fence will not be woken by a non-stall
interrupt any more. It does not have to be: the kill runs immediately
afterwards, signals every fence on fctx->pending under fctx->lock and
sets fctx->killed, after which nouveau_fence_emit() refuses further
work with -ENODEV.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260812231330.705425-1-mczernohous@gmail.com?part=1
Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to workqueue")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 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..4a3698dc2cd1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
@@ -96,9 +96,9 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error)
 void
 nouveau_fence_context_del(struct nouveau_fence_chan *fctx)
 {
+	nvif_event_dtor(&fctx->event);
 	cancel_work_sync(&fctx->uevent_work);
 	nouveau_fence_context_kill(fctx, 0);
-	nvif_event_dtor(&fctx->event);
 	fctx->dead = 1;
 
 	/*
-- 
2.54.0

[PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nouveau_connector_destroy() tears the two nvif events down and then
frees the connector, but never cancels the work the IRQ event queues:

	nvif_event_dtor(&nv_connector->irq);
	nvif_event_dtor(&nv_connector->hpd);
	kfree(nv_connector->edid);
	...
	kfree(connector);

nouveau_connector_irq() queues that work unconditionally:

	schedule_work(&nv_connector->irq_work);
	return NVIF_EVENT_KEEP;

A DP IRQ arriving just before nvif_event_dtor() therefore leaves
nv_connector->irq_work on the system queue past the kfree(). When it
runs, nouveau_dp_irq() derives both nv_connector and connector from the
work_struct and dereferences them, and goes on to take
outp->dp.hpd_irq_lock.

There is no cancel_work_sync() for irq_work anywhere in the driver, so
nothing else covers this. Add it after the event teardown, where no
further work can be queued, and before anything is freed.

Reported by the Sashiko review bot as a pre-existing issue, in its review
of an earlier nv04 FIFO series of mine, and confirmed against the source.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260812231330.705425-1-mczernohous@gmail.com?part=1
Fixes: 773eb04d14a1 ("drm/nouveau/disp: expose conn event class")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index b0b0ad9a0c24..e49dcaa6d210 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -397,6 +397,7 @@ nouveau_connector_destroy(struct drm_connector *connector)
 	struct nouveau_connector *nv_connector = nouveau_connector(connector);
 	nvif_event_dtor(&nv_connector->irq);
 	nvif_event_dtor(&nv_connector->hpd);
+	cancel_work_sync(&nv_connector->irq_work);
 	kfree(nv_connector->edid);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
-- 
2.54.0

[PATCH 3/3] drm/nouveau: don't dereference outp before checking it in nouveau_dp_irq

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

nouveau_dp_irq() looks the encoder up and dereferences it in the same
breath, five lines before testing it:

	struct nouveau_encoder *outp = find_encoder(connector, DCB_OUTPUT_DP);
	struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev);
	...
	if (!outp)
		return;

find_encoder() walks the connector's possible encoders and returns NULL
when none of them matches the requested type, so the NULL test is not
decoration: it is the author saying this can happen. The initialiser
above it dereferences the same pointer regardless.

The NULL test predates the dereference. commit 773eb04d14a1
("drm/nouveau/disp: expose conn event class") turned nouveau_dp_irq()
into a work callback, and since the drm pointer was no longer passed in
as an argument it was recovered from the encoder in the declaration
block, which put the dereference above the existing test.

Move the drm lookup below the test. No functional change when outp is
non-NULL.

Fixes: 773eb04d14a1 ("drm/nouveau/disp: expose conn event class")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---
 drivers/gpu/drm/nouveau/nouveau_dp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 55691ec44aba..738802358d85 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -486,7 +486,7 @@ nouveau_dp_irq(struct work_struct *work)
 		container_of(work, typeof(*nv_connector), irq_work);
 	struct drm_connector *connector = &nv_connector->base;
 	struct nouveau_encoder *outp = find_encoder(connector, DCB_OUTPUT_DP);
-	struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev);
+	struct nouveau_drm *drm;
 	struct nv50_mstm *mstm;
 	u64 hpd = 0;
 	int ret;
@@ -494,6 +494,8 @@ nouveau_dp_irq(struct work_struct *work)
 	if (!outp)
 		return;
 
+	drm = nouveau_drm(outp->base.base.dev);
+
 	mstm = outp->dp.mstm;
 	NV_DEBUG(drm, "service %s\n", connector->name);
 
-- 
2.54.0

Re: [PATCH 1/3] drm/nouveau: destroy the fence event before cancelling its work

[Marek Czernohous]

The bot is right, and this is worse than a wording problem: 1/3 does
introduce the race, it does not merely fail to rule it out.  Please
do not apply 1/3.  2/3 and 3/3 are independent of it and unaffected.

What I missed is why the old order was safe in the first place.  It was
not an accident of ordering, it was load-bearing:

  nouveau_fence_context_kill() signals every fence on fctx->pending,
  and dma_fence_add_callback() returns -ENOENT for an already signalled
  fence before it ever reaches __dma_fence_enable_signaling()
  (drivers/dma-buf/dma-fence.c:707-710).  So once the kill has run,
  nouveau_fence_enable_signaling() is no longer reachable for those
  fences, and nvif_event_dtor() afterwards has nobody left to race
  with.

Moving the dtor to the front puts it exactly where those fences are
still live, so nvif_event_allow() can be in flight on another CPU with
nvif_event_constructed() already evaluated to true.  There is nothing to
serialise the two: nouveau_fence_context_del() takes no lock at all,
enable_signaling() runs under fence->lock, which for nouveau is
fctx->lock (nouveau_fence.c:218-219), and the dtor cannot take that,
since the nvif ioctl may sleep and fctx->lock is taken with interrupts
off.  The window is then held open for the whole of cancel_work_sync(),
which can block arbitrarily long.

So my patch traded a narrow re-arm window for a wider NULL-deref window.
That is a bad trade and my commit message argued for it with a "guard"
that is a plain unsynchronised read of object->client.

The re-arm problem the patch was aimed at is real, but the fix has to
keep the kill in front of the dtor.  The obvious shape is to move the
drain to the back instead of the dtor to the front:

	nouveau_fence_context_kill(fctx, 0);
	nvif_event_dtor(&fctx->event);
	cancel_work_sync(&fctx->uevent_work);

The kill closes enable_signaling(), the dtor then stops the handler, and
the drain last picks up anything the handler queued on its way out.  I
want to convince myself properly that kill-before-drain is safe, rather
than send a second version tonight on the strength of it looking right,
so I will post a v2 once I have.

Thanks to the bot for catching this before anyone applied it.  For what
it is worth, my own review pass had found the same mechanism a few hours
earlier and I mis-filed it as a wording problem in the commit message
instead of asking whether the patch itself was wrong.  That one is on
me.

Re: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

[Marek Czernohous]

Three findings, three different answers, so let me take them in turn.

The NULL dereference in nouveau_dp_irq() is 3/3 of this series.  Same
thread, sent alongside this patch.  Nothing further needed there, and
the two are related on purpose: 2/3 drains the work, 3/3 fixes the
handler that work runs.

The LVDS error path in nouveau_connector_create() is real as far as I
can see, and independent of anything here.  drm_connector_init() has
already put the connector on dev->mode_config.connector_list when the
nouveau_bios_parse_lvds_table() failure path kfree()s it without
drm_connector_cleanup().  I am not touching it in this series; it wants
its own patch and I have no way to reach that path on my hardware.

The hpd_work point is the interesting one, and I owe a correction on it.

2/3 does not create that path.  Without this patch the same already
queued irq_work still runs to completion and still ends in
nouveau_connector_hpd(), which schedules drm->hpd_work under
drm->hpd_lock; the only thing this patch adds is that the destroy now
waits for it.  If anything that narrows the exposure, because without
the wait the work can run later still, potentially after the connector
is gone.  That is the bug 2/3 is about.

But the wider question the bot is asking is fair, and my cover letter
answered it too confidently.  It says "there is no fourth patch here"
on the strength of drm->hpd_work being drained in
nouveau_display_fini().  Having looked again after the bot's mail: that
drain runs at nouveau_display.c:600 under "if (!runtime &&
!drm->headless)", and disp->fini() drains it a second time under the
same condition (dispnv50/disp.c:2686, dispnv04/disp.c:72, which I had
not spotted when I wrote the cover).  Both of those are before
drm_mode_config_cleanup() reaches nouveau_connector_destroy().  So a
late irq_work really can re-arm hpd_work after every drain, and nothing
drains it again.  Whether that is reachable in practice I do not know:
nvif_event_block() on conn->irq has already run by then, so it needs
work that was queued before the block and has not run yet.

I cannot rule it out, so I should not have written that sentence as a
finding.  It should have said that I looked and did not find a fourth
patch, not that there is none.  If the maintainers want, the shape of a
fix is probably a drain of hpd_work after the connectors are gone, or
making nouveau_connector_hpd() a no-op once teardown has started, but
that is a separate change from this series and I would rather someone
who knows the hotplug path weighs in before I write it.

For the avoidance of doubt: 2/3 and 3/3 still stand as posted.  1/3 is
withdrawn, for an unrelated reason, in the sibling thread.

[PATCH v2] drm/nouveau: disable the fence uevent work instead of just cancelling it

[Marek Czernohous]

From: Marek Czernohous <marek@czernohous.de>

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 <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gmail.com/
Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to workqueue")
Cc: <stable@vger.kernel.org> # 6.10.x
Assisted-by: Claude:claude-opus-5
Signed-off-by: Marek Czernohous <marek@czernohous.de>
---

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/20260815200914.8A1131F000E9@smtp.kernel.org/
 - 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/178682366002.3748010.12779628082366287968@gmail.com/
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

Re: [PATCH v2] drm/nouveau: disable the fence uevent work instead of just cancelling it

[Marek Czernohous]

One correction to a trailer in this patch, before anyone picks it up.

The Closes: line points at the cover of the series the bot was reviewing:

  https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gmail.com/

That URL resolves, but it is the nouveau view of the thread, and the
bot's replies are not in it: that view has five messages and none of
them is the report.  The bot posts to dri-devel only, so /nouveau/
returns 404 for its message-id.  A tag that sends the reader somewhere
the report demonstrably is not seems worse than no tag, so it should be

  Closes: https://lore.kernel.org/all/20260812233022.159301F000E9@smtp.kernel.org/

which is the report itself:

  "[High] Canceling `uevent_work` before destroying `fctx->event` in
   `nouveau_fence_context_del` leaves a window for use-after-free."

That also fits this version of the commit message better than the cover
link did, since the report says use-after-free and so does the patch now.

I can send a v3 with the corrected trailer if you would rather have it
right in the patch than fixed up on apply.  The diff is the same one
word either way, and it did not seem worth a respin without asking.

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

[lyude]

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

Re: [PATCH v2] drm/nouveau: disable the fence uevent work instead of just cancelling it

[lyude]

This makes sense to me.

Reviewed-by: Lyude Paul <lyude@redhat.com>

Will push to drm-misc-next-fixes in just a moment

On Sun, 2026-08-16 at 14:58 +0200, Marek Czernohous wrote:
> From: Marek Czernohous <marek@czernohous.de>
> 
> 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 <sashiko-bot@kernel.org>
> Closes:
> https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gmail.com/
> Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to
> workqueue")
> Cc: <stable@vger.kernel.org> # 6.10.x
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Marek Czernohous <marek@czernohous.de>
> ---
> 
> 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/20260815200914.8A1131F000E9@smtp.kernel.org/
>  - 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/178682366002.3748010.12779628082366287968@gmail.com/
> 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

Re: [PATCH v2] drm/nouveau: disable the fence uevent work instead of just cancelling it

[lyude]

(I misspoke, this patch ended up in drm-misc-next instead as fixes is
currently closed)

On Thu, 2026-08-20 at 13:55 -0400, lyude@redhat.com wrote:
> This makes sense to me.
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> 
> Will push to drm-misc-next-fixes in just a moment
> 
> On Sun, 2026-08-16 at 14:58 +0200, Marek Czernohous wrote:
> > From: Marek Czernohous <marek@czernohous.de>
> > 
> > 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 <sashiko-bot@kernel.org>
> > Closes:
> > https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gmail.com/
> > Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to
> > workqueue")
> > Cc: <stable@vger.kernel.org> # 6.10.x
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Marek Czernohous <marek@czernohous.de>
> > ---
> > 
> > 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/20260815200914.8A1131F000E9@smtp.kernel.org/
> >  - 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/178682366002.3748010.12779628082366287968@gmail.com/
> > 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

Re: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

[lyude]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Sat, 2026-08-15 at 21:54 +0200, Marek Czernohous wrote:
> From: Marek Czernohous <marek@czernohous.de>
> 
> nouveau_connector_destroy() tears the two nvif events down and then
> frees the connector, but never cancels the work the IRQ event queues:
> 
> 	nvif_event_dtor(&nv_connector->irq);
> 	nvif_event_dtor(&nv_connector->hpd);
> 	kfree(nv_connector->edid);
> 	...
> 	kfree(connector);
> 
> nouveau_connector_irq() queues that work unconditionally:
> 
> 	schedule_work(&nv_connector->irq_work);
> 	return NVIF_EVENT_KEEP;
> 
> A DP IRQ arriving just before nvif_event_dtor() therefore leaves
> nv_connector->irq_work on the system queue past the kfree(). When it
> runs, nouveau_dp_irq() derives both nv_connector and connector from
> the
> work_struct and dereferences them, and goes on to take
> outp->dp.hpd_irq_lock.
> 
> There is no cancel_work_sync() for irq_work anywhere in the driver,
> so
> nothing else covers this. Add it after the event teardown, where no
> further work can be queued, and before anything is freed.
> 
> Reported by the Sashiko review bot as a pre-existing issue, in its
> review
> of an earlier nv04 FIFO series of mine, and confirmed against the
> source.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Link:
> https://sashiko.dev/#/patchset/20260812231330.705425-1-mczernohous@gmail.com?part=1
> Fixes: 773eb04d14a1 ("drm/nouveau/disp: expose conn event class")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Marek Czernohous <marek@czernohous.de>
> ---
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index b0b0ad9a0c24..e49dcaa6d210 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -397,6 +397,7 @@ nouveau_connector_destroy(struct drm_connector
> *connector)
>  	struct nouveau_connector *nv_connector =
> nouveau_connector(connector);
>  	nvif_event_dtor(&nv_connector->irq);
>  	nvif_event_dtor(&nv_connector->hpd);
> +	cancel_work_sync(&nv_connector->irq_work);
>  	kfree(nv_connector->edid);
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);

Re: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

[lyude]

On Sat, 2026-08-15 at 22:42 +0200, Marek Czernohous wrote:
> 
> I cannot rule it out, so I should not have written that sentence as a
> finding.  It should have said that I looked and did not find a fourth
> patch, not that there is none.  If the maintainers want, the shape of
> a
> fix is probably a drain of hpd_work after the connectors are gone, or
> making nouveau_connector_hpd() a no-op once teardown has started, but
> that is a separate change from this series and I would rather someone
> who knows the hotplug path weighs in before I write it.

I wrote a lot of that code, so I can just send out a patch for it in a
moment and CC you.

Re: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

[lyude]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Sat, 2026-08-15 at 21:54 +0200, Marek Czernohous wrote:
> From: Marek Czernohous <marek@czernohous.de>
> 
> nouveau_connector_destroy() tears the two nvif events down and then
> frees the connector, but never cancels the work the IRQ event queues:
> 
> 	nvif_event_dtor(&nv_connector->irq);
> 	nvif_event_dtor(&nv_connector->hpd);
> 	kfree(nv_connector->edid);
> 	...
> 	kfree(connector);
> 
> nouveau_connector_irq() queues that work unconditionally:
> 
> 	schedule_work(&nv_connector->irq_work);
> 	return NVIF_EVENT_KEEP;
> 
> A DP IRQ arriving just before nvif_event_dtor() therefore leaves
> nv_connector->irq_work on the system queue past the kfree(). When it
> runs, nouveau_dp_irq() derives both nv_connector and connector from
> the
> work_struct and dereferences them, and goes on to take
> outp->dp.hpd_irq_lock.
> 
> There is no cancel_work_sync() for irq_work anywhere in the driver,
> so
> nothing else covers this. Add it after the event teardown, where no
> further work can be queued, and before anything is freed.
> 
> Reported by the Sashiko review bot as a pre-existing issue, in its
> review
> of an earlier nv04 FIFO series of mine, and confirmed against the
> source.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Link:
> https://sashiko.dev/#/patchset/20260812231330.705425-1-mczernohous@gmail.com?part=1
> Fixes: 773eb04d14a1 ("drm/nouveau/disp: expose conn event class")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Marek Czernohous <marek@czernohous.de>
> ---
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index b0b0ad9a0c24..e49dcaa6d210 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -397,6 +397,7 @@ nouveau_connector_destroy(struct drm_connector
> *connector)
>  	struct nouveau_connector *nv_connector =
> nouveau_connector(connector);
>  	nvif_event_dtor(&nv_connector->irq);
>  	nvif_event_dtor(&nv_connector->hpd);
> +	cancel_work_sync(&nv_connector->irq_work);
>  	kfree(nv_connector->edid);
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);

Re: [PATCH 3/3] drm/nouveau: don't dereference outp before checking it in nouveau_dp_irq

[lyude]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Sat, 2026-08-15 at 21:54 +0200, Marek Czernohous wrote:
> From: Marek Czernohous <marek@czernohous.de>
> 
> nouveau_dp_irq() looks the encoder up and dereferences it in the same
> breath, five lines before testing it:
> 
> 	struct nouveau_encoder *outp = find_encoder(connector,
> DCB_OUTPUT_DP);
> 	struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev);
> 	...
> 	if (!outp)
> 		return;
> 
> find_encoder() walks the connector's possible encoders and returns
> NULL
> when none of them matches the requested type, so the NULL test is not
> decoration: it is the author saying this can happen. The initialiser
> above it dereferences the same pointer regardless.
> 
> The NULL test predates the dereference. commit 773eb04d14a1
> ("drm/nouveau/disp: expose conn event class") turned nouveau_dp_irq()
> into a work callback, and since the drm pointer was no longer passed
> in
> as an argument it was recovered from the encoder in the declaration
> block, which put the dereference above the existing test.
> 
> Move the drm lookup below the test. No functional change when outp is
> non-NULL.
> 
> Fixes: 773eb04d14a1 ("drm/nouveau/disp: expose conn event class")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Marek Czernohous <marek@czernohous.de>
> ---
>  drivers/gpu/drm/nouveau/nouveau_dp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c
> b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 55691ec44aba..738802358d85 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -486,7 +486,7 @@ nouveau_dp_irq(struct work_struct *work)
>  		container_of(work, typeof(*nv_connector), irq_work);
>  	struct drm_connector *connector = &nv_connector->base;
>  	struct nouveau_encoder *outp = find_encoder(connector,
> DCB_OUTPUT_DP);
> -	struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev);
> +	struct nouveau_drm *drm;
>  	struct nv50_mstm *mstm;
>  	u64 hpd = 0;
>  	int ret;
> @@ -494,6 +494,8 @@ nouveau_dp_irq(struct work_struct *work)
>  	if (!outp)
>  		return;
>  
> +	drm = nouveau_drm(outp->base.base.dev);
> +
>  	mstm = outp->dp.mstm;
>  	NV_DEBUG(drm, "service %s\n", connector->name);
>  

Re: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector

[lyude]

On Sat, 2026-08-15 at 22:42 +0200, Marek Czernohous wrote:
> But the wider question the bot is asking is fair, and my cover letter
> answered it too confidently.  It says "there is no fourth patch here"
> on the strength of drm->hpd_work being drained in
> nouveau_display_fini().  Having looked again after the bot's mail: that
> drain runs at nouveau_display.c:600 under "if (!runtime &&
> !drm->headless)", and disp->fini() drains it a second time under the
> same condition (dispnv50/disp.c:2686, dispnv04/disp.c:72, which I had
> not spotted when I wrote the cover).  Both of those are before
> drm_mode_config_cleanup() reaches nouveau_connector_destroy().  So a
> late irq_work really can re-arm hpd_work after every drain, and nothing
> drains it again.  Whether that is reachable in practice I do not know:
> nvif_event_block() on conn->irq has already run by then, so it needs
> work that was queued before the block and has not run yet.

Actually - after rereading this again, nah - this isn't an issue. So
long as the connector IRQs are blocked at that point, it should be
good. MST connectors aren't, but that's also fine - they use the IRQ
notify thingies of the non-MST connectors, so they're indirectly
blocked by that.