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 <[email protected]> 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/[email protected]?part=1
