Re: [PATCH] fix: drm/nouveau: nvkm_chan_new_: multiple error paths return without releasing acquired references

[sashiko-bot@kernel.org]

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