Re: [PATCH v3 1/5] drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch erroneously changes the success return value of `nouveau_drm_device_new()` from returning the allocated `drm` struct pointer to returning `0`. In C, returning `0` in a pointer context evaluates to a NULL pointer, which bypasses `IS_ERR()` checks in callers and leads to an immediate kernel panic upon dereference.
--

--- Patch [1]: [PATCH v3 1/5] drm/nouveau: Fix cleanup bug in nouveau_drm_device_new() ---
commit 764ec8f5aa922a913d49de424c5e71dadb787207
Author: Lyude Paul <[email protected]>

drm/nouveau: Fix cleanup bug in nouveau_drm_device_new()

This patch refactors the error handling in nouveau_drm_device_new() to avoid
freeing an error pointer if the DRM device pointer allocation fails. It replaces
the single done label with specific error labels for different cleanup paths.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 4d1ad718e09b7..3dfe21cd4bde4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -762,39 +762,43 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
[ ... ]
>  	ret = nvif_mmu_ctor(&drm->device.object, "drmMmu", mmus[ret].oclass, &drm->mmu);
>  	if (ret) {
>  		NV_ERROR(drm, "MMU allocation failed: %d\n", ret);
> -		goto done;
> +		goto err_device_del;
>  	}
>  
> -done:
> -	if (ret) {
> -		nouveau_drm_device_del(drm);
> -		drm = NULL;
> -	}
> +	return 0;

[Severity: High]
Does returning 0 here evaluate to a NULL pointer since the function's return
type is a pointer to struct nouveau_drm?

Callers like nouveau_drm_probe() check the return value using IS_ERR():

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_probe() {
	...
	drm = nouveau_drm_device_new(&driver_pci, &pdev->dev, device);
	if (IS_ERR(drm)) {
		ret = PTR_ERR(drm);
		goto fail_nvkm;
	}

	ret = nouveau_drm_device_init(drm);
	...
}

Because IS_ERR(NULL) is false, the error check is bypassed. The NULL pointer
is then passed to nouveau_drm_device_init() where it is immediately
dereferenced:

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_init() {
	struct drm_device *dev = drm->dev;
	...
}

Could this lead to a kernel panic during device probe? Should this return the
allocated drm pointer instead of 0?

> +
> +err_free_drm:
> +	kfree(drm);
> +
> +	return ERR_PTR(ret);
> +
> +err_device_del:
> +	nouveau_drm_device_del(drm);
>  
> -	return ret ? ERR_PTR(ret) : drm;
> +	return ERR_PTR(ret);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1