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

[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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Moving `nvif_event_dtor()` before `nouveau_fence_context_kill()` introduces an unprotected Time-of-Check to Time-of-Use (TOCTOU) race condition that results in a NULL pointer dereference in `nvif_object_ioctl`.
--

--- Patch [1]: [PATCH 1/3] drm/nouveau: destroy the fence event before cancelling its work ---
commit 248f9b5f2479dc65d3572babf739eb3883d7b9f7
Author: Marek Czernohous <marek@czernohous.de>

drm/nouveau: destroy the fence event before cancelling its work

This commit reorders the teardown sequence in nouveau_fence_context_del() by
moving nvif_event_dtor() before cancel_work_sync() and
nouveau_fence_context_kill(). It aims to prevent a newly arriving interrupt
from re-arming the work that was just cancelled.

Link: https://sashiko.dev/#/patchset/20260812231330.705425-1-mczernohous@gmail.com?part=1

> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c
> index edbe9e08ba0fd..4a3698dc2cd14 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);

[Severity: Critical]
Does moving nvif_event_dtor() before nouveau_fence_context_kill() introduce a
Time-of-Check to Time-of-Use (TOCTOU) race condition?

Since nouveau_fence_context_kill() hasn't run yet, the fences are still
active and unsignaled. If Thread A is polling a fence and calls
nouveau_fence_enable_signaling() which reaches nvif_event_allow(), it might
evaluate nvif_event_constructed() right before Thread B destroys the event:

Thread A:
drivers/gpu/drm/nouveau/nvif/event.c:nvif_event_allow() {
	if (nvif_event_constructed(event)) {

Thread B concurrently cleans up the channel:
drivers/gpu/drm/nouveau/nouveau_fence.c:nouveau_fence_context_del() {
	nvif_event_dtor(&fctx->event);
	...
drivers/gpu/drm/nouveau/nvif/object.c:nvif_object_dtor() {
	nvif_object_ioctl(object, &args, sizeof(args), NULL);
	object->client = NULL;

Thread A then continues and calls into nvif_mthd():
drivers/gpu/drm/nouveau/nvif/event.c:nvif_event_allow() {
		int ret = nvif_mthd(&event->object, NVIF_EVENT_V0_ALLOW, ...);
		...
drivers/gpu/drm/nouveau/nvif/object.c:nvif_object_ioctl() {
	struct nvif_client *client = object->client;
	/* ... */
	return client->driver->ioctl(client->object.priv, data, size, hack);

Could Thread A read the stale object->client as NULL and fatally dereference
it here, since both polling on an exported sync_file fd and channel cleanup
are accessible to unprivileged userspace?

> -	nvif_event_dtor(&fctx->event);
>  	fctx->dead = 1;
>  
>  	/*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178682366001.3748010.7798811159846779765@gmail.com?part=1

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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free / Memory Leak in nouveau_connector_create() on LVDS initialization error path.
- [High] NULL pointer dereference in nouveau_dp_irq() when initializing the drm variable.
- [High] Use-After-Free on the drm object due to late irq_work cancellation.
--

--- Patch [2]: [PATCH 2/3] drm/nouveau: cancel the DP IRQ work before freeing the connector ---
commit f68e09b38970f3b87ae7ed5977b718c0b00485df
Author: Marek Czernohous <marek@czernohous.de>

drm/nouveau: cancel the DP IRQ work before freeing the connector

This patch adds a cancel_work_sync() call to nouveau_connector_destroy()
to ensure the DP IRQ work is cancelled before the connector is freed. This
prevents a use-after-free where the irq_work remains on the system queue
past the kfree().

Link: https://sashiko.dev/#/patchset/20260812231330.705425-1-mczernohous@gmail.com?part=1

> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index b0b0ad9a0c248..e49dcaa6d210b 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);

[Severity: High]
This isn't a bug introduced by this patch, but does cancelling the work here
leave a race condition that causes a use-after-free on the drm object?

During driver teardown, nouveau_display_fini() cancels drm->hpd_work. Later,
drm_mode_config_cleanup() calls nouveau_connector_destroy(), which now waits
on nv_connector->irq_work here.

If irq_work is actively running on a worker thread when this executes,
cancel_work_sync() will block. While finishing, nouveau_dp_irq() calls
nouveau_connector_hpd(), which unconditionally schedules drm->hpd_work:

nouveau_dp_irq() {
    ...
    nouveau_connector_hpd(nv_connector, NVIF_CONN_EVENT_V0_IRQ | hpd);
}

Since drm->hpd_work was already cancelled and is never cancelled again, it is
placed back on the system workqueue. After teardown completes and the drm
object is freed in nouveau_drm_device_remove(), the workqueue could execute
nouveau_display_hpd_work(), dereferencing the freed pointer. Should this
cancellation happen earlier, such as in nouveau_display_fini()?

[Severity: High]
This is a pre-existing issue, but looking at nouveau_connector_create() in
nouveau_connector.c, does the LVDS initialization error path leak the
connector and cause a use-after-free?

nouveau_connector_create() {
    ...
    drm_connector_init(dev, &nv_connector->base, funcs, type);
    ...
    if (type == DRM_MODE_CONNECTOR_LVDS) {
        ...
        ret = nouveau_bios_parse_lvds_table(dev, 0, &dummy, &dummy);
        if (ret) {
            NV_ERROR(drm, "Error parsing LVDS table, disabling\n");
            kfree(nv_connector);
            return ERR_PTR(ret);
        }
    ...
}

drm_connector_init() adds the connector to dev->mode_config.connector_list and
allocates an IDA. If the LVDS table parsing fails, the error path frees
nv_connector with kfree() but doesn't call drm_connector_cleanup(). Does this
leave a dangling pointer in the global connector list that will crash when
traversed, and leak the allocated IDA?

[Severity: High]
This is a pre-existing issue, but looking at nouveau_dp_irq() in
nouveau_dp.c, can we encounter a NULL pointer dereference here?

nouveau_dp_irq() {
    ...
    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() can return NULL if a valid DisplayPort encoder isn't found,
which the code anticipates. However, outp is immediately dereferenced during
variable initialization to fetch the dev pointer for nouveau_drm() before the
null check executes. Can this cause a kernel oops if outp is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178682366001.3748010.7798811159846779765@gmail.com?part=2

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.

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.