* [PATCH] fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references
@ 2026-06-26 14:39 ` WenTao Liang
0 siblings, 0 replies; 6+ messages in thread
From: WenTao Liang @ 2026-06-26 14:39 UTC (permalink / raw)
To: Lyude Paul, Danilo Krummrich, dri-devel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Ben Skeggs, nouveau, stable, linux-kernel,
WenTao Liang
After allocating chan and acquiring references on cgrp, vmm, memory, and
chid, numerous subsequent error paths return ret directly without calling
kfree, nvkm_cgrp_unref, nvkm_vmm_unref, nvkm_memory_unref, or
nvkm_chid_put. Every error return after the initial allocations leaks one
or more acquired resources.
Cc: stable@vger.kernel.org
Fixes: 06db7fded6de ("drm/nouveau/fifo: add new channel classes")
Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
---
.../gpu/drm/nouveau/nvkm/engine/fifo/chan.c | 25 +++++++++++++------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
index 418a8918bcb8..1bf595bafa9a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
@@ -410,18 +410,18 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
&chan->inst);
if (ret) {
RUNL_DEBUG(runl, "inst %d", ret);
- return ret;
+ goto done;
}
/* Initialise virtual address-space. */
if (func->inst->vmm) {
if (WARN_ON(vmm->mmu != device->mmu))
- return -EINVAL;
+ goto done;
ret = nvkm_vmm_join(vmm, chan->inst->memory);
if (ret) {
RUNL_DEBUG(runl, "vmm %d", ret);
- return ret;
+ goto done;
}
chan->vmm = nvkm_vmm_ref(vmm);
@@ -432,7 +432,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
ret = nvkm_object_bind(&dmaobj->object, chan->inst, -16, &chan->push);
if (ret) {
RUNL_DEBUG(runl, "bind %d", ret);
- return ret;
+ goto done;
}
}
@@ -443,13 +443,13 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
if (ouserd + chan->func->userd->size >=
nvkm_memory_size(userd)) {
RUNL_DEBUG(runl, "ouserd %llx", ouserd);
- return -EINVAL;
+ goto done;
}
ret = nvkm_memory_kmap(userd, &chan->userd.mem);
if (ret) {
RUNL_DEBUG(runl, "userd %d", ret);
- return ret;
+ goto done;
}
chan->userd.base = ouserd;
@@ -461,7 +461,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
if (chan->id < 0) {
RUNL_ERROR(runl, "!chids");
- return -ENOSPC;
+ goto done;
}
if (cgrp->id < 0)
@@ -475,8 +475,17 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
ret = chan->func->ramfc->write(chan, offset, length, devm, priv);
if (ret) {
RUNL_DEBUG(runl, "ramfc %d", ret);
- return ret;
+ goto done;
}
return 0;
+
+done:
+ if (chan->id >= 0)
+ nvkm_chid_put(runl->chid, chan->id, &chan->cgrp->lock);
+ nvkm_memory_unref(&chan->userd.mem);
+ nvkm_vmm_unref(&chan->vmm);
+ nvkm_cgrp_unref(&chan->cgrp);
+ *pchan = NULL;
+ return ret;
}
--
2.39.5 (Apple Git-154)
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references
@ 2026-06-26 14:39 ` WenTao Liang
0 siblings, 0 replies; 6+ messages in thread
From: WenTao Liang @ 2026-06-26 14:39 UTC (permalink / raw)
To: Lyude Paul, Danilo Krummrich, dri-devel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Ben Skeggs, nouveau, stable, linux-kernel,
WenTao Liang
After allocating chan and acquiring references on cgrp, vmm, memory, and
chid, numerous subsequent error paths return ret directly without calling
kfree, nvkm_cgrp_unref, nvkm_vmm_unref, nvkm_memory_unref, or
nvkm_chid_put. Every error return after the initial allocations leaks one
or more acquired resources.
Cc: stable@vger.kernel.org
Fixes: 06db7fded6de ("drm/nouveau/fifo: add new channel classes")
Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
---
.../gpu/drm/nouveau/nvkm/engine/fifo/chan.c | 25 +++++++++++++------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
index 418a8918bcb8..1bf595bafa9a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
@@ -410,18 +410,18 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
&chan->inst);
if (ret) {
RUNL_DEBUG(runl, "inst %d", ret);
- return ret;
+ goto done;
}
/* Initialise virtual address-space. */
if (func->inst->vmm) {
if (WARN_ON(vmm->mmu != device->mmu))
- return -EINVAL;
+ goto done;
ret = nvkm_vmm_join(vmm, chan->inst->memory);
if (ret) {
RUNL_DEBUG(runl, "vmm %d", ret);
- return ret;
+ goto done;
}
chan->vmm = nvkm_vmm_ref(vmm);
@@ -432,7 +432,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
ret = nvkm_object_bind(&dmaobj->object, chan->inst, -16, &chan->push);
if (ret) {
RUNL_DEBUG(runl, "bind %d", ret);
- return ret;
+ goto done;
}
}
@@ -443,13 +443,13 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
if (ouserd + chan->func->userd->size >=
nvkm_memory_size(userd)) {
RUNL_DEBUG(runl, "ouserd %llx", ouserd);
- return -EINVAL;
+ goto done;
}
ret = nvkm_memory_kmap(userd, &chan->userd.mem);
if (ret) {
RUNL_DEBUG(runl, "userd %d", ret);
- return ret;
+ goto done;
}
chan->userd.base = ouserd;
@@ -461,7 +461,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
if (chan->id < 0) {
RUNL_ERROR(runl, "!chids");
- return -ENOSPC;
+ goto done;
}
if (cgrp->id < 0)
@@ -475,8 +475,17 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
ret = chan->func->ramfc->write(chan, offset, length, devm, priv);
if (ret) {
RUNL_DEBUG(runl, "ramfc %d", ret);
- return ret;
+ goto done;
}
return 0;
+
+done:
+ if (chan->id >= 0)
+ nvkm_chid_put(runl->chid, chan->id, &chan->cgrp->lock);
+ nvkm_memory_unref(&chan->userd.mem);
+ nvkm_vmm_unref(&chan->vmm);
+ nvkm_cgrp_unref(&chan->cgrp);
+ *pchan = NULL;
+ return ret;
}
--
2.39.5 (Apple Git-154)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references
2026-06-26 14:39 ` WenTao Liang
(?)
(?)
@ 2026-06-26 14:53 ` sashiko-bot
-1 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-06-26 14:53 UTC (permalink / raw)
To: WenTao Liang; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Silent failure converting error paths to success, leading to immediate NULL pointer dereference.
- [High] Setting `*pchan = NULL` on error bypasses framework destructors, causing severe memory and GPU resource leaks.
--
commit 36c8694a5768a3efc7f331411db88cf737b0facf
Author: WenTao Liang <vulab@iscas.ac.cn>
fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references
This commit aims to fix resource leaks in error paths by routing them through
a single cleanup block.
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> index 418a8918bcb8f..1bf595bafa9ac 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> @@ -410,18 +410,18 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> &chan->inst);
> if (ret) {
> RUNL_DEBUG(runl, "inst %d", ret);
> - return ret;
> + goto done;
> }
>
> /* Initialise virtual address-space. */
> if (func->inst->vmm) {
> if (WARN_ON(vmm->mmu != device->mmu))
> - return -EINVAL;
> + goto done;
[Severity: Critical]
Does this leave the local variable ret set to 0 from the earlier successful
nvkm_gpuobj_new() call?
If so, the function returns success while setting *pchan = NULL. When the
caller receives a successful return code, it might proceed to dereference the
channel, leading to a null pointer dereference:
nvkm_uchan_new() {
...
chan = uchan->chan;
if (chan->func->doorbell_handle) {
...
}
Could we set ret = -EINVAL before jumping to the done label?
[ ... ]
> @@ -443,13 +443,13 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> if (ouserd + chan->func->userd->size >=
> nvkm_memory_size(userd)) {
> RUNL_DEBUG(runl, "ouserd %llx", ouserd);
> - return -EINVAL;
> + goto done;
> }
[Severity: Critical]
Is there a similar missing error code assignment here? It looks like this path
would also return success without initializing the channel.
[ ... ]
> @@ -461,7 +461,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
>
> if (chan->id < 0) {
> RUNL_ERROR(runl, "!chids");
> - return -ENOSPC;
> + goto done;
> }
[Severity: Critical]
Does this path also need ret = -ENOSPC before jumping to the cleanup block?
[ ... ]
> +done:
> + if (chan->id >= 0)
> + nvkm_chid_put(runl->chid, chan->id, &chan->cgrp->lock);
> + nvkm_memory_unref(&chan->userd.mem);
> + nvkm_vmm_unref(&chan->vmm);
> + nvkm_cgrp_unref(&chan->cgrp);
> + *pchan = NULL;
> + return ret;
> }
[Severity: High]
Does setting *pchan = NULL hide the partially allocated object from the
caller's framework destructor?
When an error occurs, the native destructors like nvkm_object_del() and
nvkm_chan_del() are usually invoked by the framework to clean up. Since the
done block misses freeing the channel struct and initialized GPU objects like
chan->inst, clearing the pointer here seems to prevent the framework from
finishing the cleanup, resulting in a resource leak.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626143953.48952-1-vulab@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references
2026-06-26 14:39 ` WenTao Liang
@ 2026-06-28 3:56 ` WenTao Liang
-1 siblings, 0 replies; 6+ messages in thread
From: WenTao Liang @ 2026-06-28 3:56 UTC (permalink / raw)
To: Lyude Paul, Danilo Krummrich, dri-devel
Cc: Maarten Lankhorst, Maxime Ripard, Simona Vetter, Ben Skeggs,
nouveau, stable, linux-kernel
> 2026年6月26日 22:39，WenTao Liang <vulab@iscas.ac.cn> 写道：
>
> After allocating chan and acquiring references on cgrp, vmm, memory, and
> chid, numerous subsequent error paths return ret directly without calling
> kfree, nvkm_cgrp_unref, nvkm_vmm_unref, nvkm_memory_unref, or
> nvkm_chid_put. Every error return after the initial allocations leaks one
> or more acquired resources.
>
> Cc: stable@vger.kernel.org
> Fixes: 06db7fded6de ("drm/nouveau/fifo: add new channel classes")
> Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
> ---
> .../gpu/drm/nouveau/nvkm/engine/fifo/chan.c | 25 +++++++++++++------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> index 418a8918bcb8..1bf595bafa9a 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> @@ -410,18 +410,18 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> &chan->inst);
> if (ret) {
> RUNL_DEBUG(runl, "inst %d", ret);
> - return ret;
> + goto done;
> }
>
> /* Initialise virtual address-space. */
> if (func->inst->vmm) {
> if (WARN_ON(vmm->mmu != device->mmu))
> - return -EINVAL;
> + goto done;
>
> ret = nvkm_vmm_join(vmm, chan->inst->memory);
> if (ret) {
> RUNL_DEBUG(runl, "vmm %d", ret);
> - return ret;
> + goto done;
> }
>
> chan->vmm = nvkm_vmm_ref(vmm);
> @@ -432,7 +432,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> ret = nvkm_object_bind(&dmaobj->object, chan->inst, -16, &chan->push);
> if (ret) {
> RUNL_DEBUG(runl, "bind %d", ret);
> - return ret;
> + goto done;
> }
> }
>
> @@ -443,13 +443,13 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> if (ouserd + chan->func->userd->size >=
> nvkm_memory_size(userd)) {
> RUNL_DEBUG(runl, "ouserd %llx", ouserd);
> - return -EINVAL;
> + goto done;
> }
>
> ret = nvkm_memory_kmap(userd, &chan->userd.mem);
> if (ret) {
> RUNL_DEBUG(runl, "userd %d", ret);
> - return ret;
> + goto done;
> }
>
> chan->userd.base = ouserd;
> @@ -461,7 +461,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
>
> if (chan->id < 0) {
> RUNL_ERROR(runl, "!chids");
> - return -ENOSPC;
> + goto done;
> }
>
> if (cgrp->id < 0)
> @@ -475,8 +475,17 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> ret = chan->func->ramfc->write(chan, offset, length, devm, priv);
> if (ret) {
> RUNL_DEBUG(runl, "ramfc %d", ret);
> - return ret;
> + goto done;
> }
>
> return 0;
> +
> +done:
> + if (chan->id >= 0)
> + nvkm_chid_put(runl->chid, chan->id, &chan->cgrp->lock);
> + nvkm_memory_unref(&chan->userd.mem);
> + nvkm_vmm_unref(&chan->vmm);
> + nvkm_cgrp_unref(&chan->cgrp);
> + *pchan = NULL;
> + return ret;
> }
> --
> 2.39.5 (Apple Git-154)
Please ignore this patch. I will resend a proper version after
learning the kernel submission process.
Apologies for the noise.
Best regards,
WenTao Liang
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references
@ 2026-06-28 3:56 ` WenTao Liang
0 siblings, 0 replies; 6+ messages in thread
From: WenTao Liang @ 2026-06-28 3:56 UTC (permalink / raw)
To: Lyude Paul, Danilo Krummrich, dri-devel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Ben Skeggs, nouveau, stable, linux-kernel
> 2026年6月26日 22:39，WenTao Liang <vulab@iscas.ac.cn> 写道：
>
> After allocating chan and acquiring references on cgrp, vmm, memory, and
> chid, numerous subsequent error paths return ret directly without calling
> kfree, nvkm_cgrp_unref, nvkm_vmm_unref, nvkm_memory_unref, or
> nvkm_chid_put. Every error return after the initial allocations leaks one
> or more acquired resources.
>
> Cc: stable@vger.kernel.org
> Fixes: 06db7fded6de ("drm/nouveau/fifo: add new channel classes")
> Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
> ---
> .../gpu/drm/nouveau/nvkm/engine/fifo/chan.c | 25 +++++++++++++------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> index 418a8918bcb8..1bf595bafa9a 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/chan.c
> @@ -410,18 +410,18 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> &chan->inst);
> if (ret) {
> RUNL_DEBUG(runl, "inst %d", ret);
> - return ret;
> + goto done;
> }
>
> /* Initialise virtual address-space. */
> if (func->inst->vmm) {
> if (WARN_ON(vmm->mmu != device->mmu))
> - return -EINVAL;
> + goto done;
>
> ret = nvkm_vmm_join(vmm, chan->inst->memory);
> if (ret) {
> RUNL_DEBUG(runl, "vmm %d", ret);
> - return ret;
> + goto done;
> }
>
> chan->vmm = nvkm_vmm_ref(vmm);
> @@ -432,7 +432,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> ret = nvkm_object_bind(&dmaobj->object, chan->inst, -16, &chan->push);
> if (ret) {
> RUNL_DEBUG(runl, "bind %d", ret);
> - return ret;
> + goto done;
> }
> }
>
> @@ -443,13 +443,13 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> if (ouserd + chan->func->userd->size >=
> nvkm_memory_size(userd)) {
> RUNL_DEBUG(runl, "ouserd %llx", ouserd);
> - return -EINVAL;
> + goto done;
> }
>
> ret = nvkm_memory_kmap(userd, &chan->userd.mem);
> if (ret) {
> RUNL_DEBUG(runl, "userd %d", ret);
> - return ret;
> + goto done;
> }
>
> chan->userd.base = ouserd;
> @@ -461,7 +461,7 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
>
> if (chan->id < 0) {
> RUNL_ERROR(runl, "!chids");
> - return -ENOSPC;
> + goto done;
> }
>
> if (cgrp->id < 0)
> @@ -475,8 +475,17 @@ nvkm_chan_new_(const struct nvkm_chan_func *func, struct nvkm_runl *runl, int ru
> ret = chan->func->ramfc->write(chan, offset, length, devm, priv);
> if (ret) {
> RUNL_DEBUG(runl, "ramfc %d", ret);
> - return ret;
> + goto done;
> }
>
> return 0;
> +
> +done:
> + if (chan->id >= 0)
> + nvkm_chid_put(runl->chid, chan->id, &chan->cgrp->lock);
> + nvkm_memory_unref(&chan->userd.mem);
> + nvkm_vmm_unref(&chan->vmm);
> + nvkm_cgrp_unref(&chan->cgrp);
> + *pchan = NULL;
> + return ret;
> }
> --
> 2.39.5 (Apple Git-154)
Please ignore this patch. I will resend a proper version after
learning the kernel submission process.
Apologies for the noise.
Best regards,
WenTao Liang
^ permalink raw reply [flat|nested] 6+ messages in thread