Nouveau - May 2020 - [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new

When gk20a_clk_ctor() returns an error code, pointer "clk"
should be released. It's the same when gm20b_clk_new()
returns from elsewhere following this call.

Signed-off-by: Dinghao Liu <dinghao.liu at zju.edu.cn>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
index b284e949f732..a5aeba74d3b7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
@@ -1039,7 +1039,7 @@ gm20b_clk_new(struct nvkm_device *device, int index,
struct nvkm_clk **pclk)
 	ret = gk20a_clk_ctor(device, index, &gm20b_clk, clk_params,
 			     &clk->base);
 	if (ret)
-		return ret;
+		goto out_free;
 
 	/*
 	 * NAPLL can only work with max_u, clamp the m range so
@@ -1067,8 +1067,8 @@ gm20b_clk_new(struct nvkm_device *device, int index,
struct nvkm_clk **pclk)
 		nvkm_warn(subdev, "no fused calibration parameters\n");
 
 	ret = gm20b_clk_init_safe_fmax(clk);
-	if (ret)
-		return ret;
 
-	return 0;
+out_free:
+	kfree(clk);
+	return ret;
 }
-- 
2.17.1

On Sat, 30 May 2020 at 19:42, Dinghao Liu <dinghao.liu at zju.edu.cn>
wrote:
>
> When gk20a_clk_ctor() returns an error code, pointer "clk"
> should be released. It's the same when gm20b_clk_new()
> returns from elsewhere following this call.
This shouldn't be necessary.  If a subdev constructor fails, and
returns a pointer, the core will call the destructor to clean things
up.

Ben.
>
> Signed-off-by: Dinghao Liu <dinghao.liu at zju.edu.cn>
> ---
>  drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
> index b284e949f732..a5aeba74d3b7 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c
> @@ -1039,7 +1039,7 @@ gm20b_clk_new(struct nvkm_device *device, int index,
struct nvkm_clk **pclk)
>         ret = gk20a_clk_ctor(device, index, &gm20b_clk, clk_params,
>                              &clk->base);
>         if (ret)
> -               return ret;
> +               goto out_free;
>
>         /*
>          * NAPLL can only work with max_u, clamp the m range so
> @@ -1067,8 +1067,8 @@ gm20b_clk_new(struct nvkm_device *device, int index,
struct nvkm_clk **pclk)
>                 nvkm_warn(subdev, "no fused calibration
parameters\n");
>
>         ret = gm20b_clk_init_safe_fmax(clk);
> -       if (ret)
> -               return ret;
>
> -       return 0;
> +out_free:
> +       kfree(clk);
> +       return ret;
>  }
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Ben,
> > When gk20a_clk_ctor() returns an error code, pointer "clk"
> > should be released. It's the same when gm20b_clk_new()
> > returns from elsewhere following this call.
> This shouldn't be necessary.  If a subdev constructor fails, and
> returns a pointer, the core will call the destructor to clean things
> up.
> 
I'm not familiar with the behavior of the caller of gm20b_clk_new(). 
If the subdev constructor fails, the core will check the pointer
(here is "pclk"), then it's ok and there is no bug (Do you mean 
this?). If the core executes error handling code only according to 
the error code, there may be a memory leak bug (the caller cannot 
know if -ENOMEM comes from the failure of kzalloc or gk20a_clk_ctor). 
If the core always calls the destructor as long as the constructor 
fails (even if the kzalloc fails), we may have a double free bug. 

Would you like to give a more detailed explanation about the behavior
of the core? 

Regards,
Dinghao