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
