* [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
@ 2020-05-29 8:00 Dinghao Liu
2020-05-31 21:26 ` Ben Skeggs
0 siblings, 1 reply; 6+ messages in thread
From: Dinghao Liu @ 2020-05-29 8:00 UTC (permalink / raw)
To: dinghao.liu, kjlu
Cc: Ben Skeggs, David Airlie, Daniel Vetter, dri-devel, nouveau,
linux-kernel
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@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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
2020-05-29 8:00 [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new Dinghao Liu
@ 2020-05-31 21:26 ` Ben Skeggs
2020-06-01 3:26 ` dinghao.liu
0 siblings, 1 reply; 6+ messages in thread
From: Ben Skeggs @ 2020-05-31 21:26 UTC (permalink / raw)
To: Dinghao Liu
Cc: kjlu, David Airlie, ML nouveau, LKML, ML dri-devel, Ben Skeggs
On Sat, 30 May 2020 at 19:42, Dinghao Liu <dinghao.liu@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
2020-05-31 21:26 ` Ben Skeggs
@ 2020-06-01 3:26 ` dinghao.liu
2020-06-01 3:37 ` Ben Skeggs
0 siblings, 1 reply; 6+ messages in thread
From: dinghao.liu @ 2020-06-01 3:26 UTC (permalink / raw)
To: Ben Skeggs
Cc: kjlu, David Airlie, ML nouveau, LKML, ML dri-devel, Ben Skeggs,
Markus.Elfring
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
2020-06-01 3:26 ` dinghao.liu
@ 2020-06-01 3:37 ` Ben Skeggs
2020-06-01 3:41 ` Ben Skeggs
0 siblings, 1 reply; 6+ messages in thread
From: Ben Skeggs @ 2020-06-01 3:37 UTC (permalink / raw)
To: dinghao.liu
Cc: kjlu, David Airlie, ML nouveau, LKML, ML dri-devel, Ben Skeggs,
Markus.Elfring
On Mon, 1 Jun 2020 at 13:27, <dinghao.liu@zju.edu.cn> wrote:
>
>
> 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?
If there's *any* error, it'll check the pointer, if it's non-NULL,
it'll call the destructor. If kzalloc() fails, the pointer will be
NULL, there's no double-free bug. *every* subdev is written this way
to avoid duplicating cleanup logic.
Ben.
>
> Regards,
> Dinghao
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
2020-06-01 3:37 ` Ben Skeggs
@ 2020-06-01 3:41 ` Ben Skeggs
2020-06-01 5:21 ` dinghao.liu
0 siblings, 1 reply; 6+ messages in thread
From: Ben Skeggs @ 2020-06-01 3:41 UTC (permalink / raw)
To: dinghao.liu
Cc: kjlu, David Airlie, ML nouveau, LKML, ML dri-devel, Ben Skeggs,
Markus.Elfring
On Mon, 1 Jun 2020 at 13:37, Ben Skeggs <skeggsb@gmail.com> wrote:
>
> On Mon, 1 Jun 2020 at 13:27, <dinghao.liu@zju.edu.cn> wrote:
> >
> >
> > 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?
> If there's *any* error, it'll check the pointer, if it's non-NULL,
> it'll call the destructor. If kzalloc() fails, the pointer will be
> NULL, there's no double-free bug. *every* subdev is written this way
> to avoid duplicating cleanup logic.
Actually, gm20b_clk_new_speedo0() may have a bug here if kzalloc()
fails as it doesn't overwrite the previous pointer from
gm20b_clk_new(). That whole ctor() sequence is written a little
strangely.
>
> Ben.
> >
> > Regards,
> > Dinghao
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
2020-06-01 3:41 ` Ben Skeggs
@ 2020-06-01 5:21 ` dinghao.liu
0 siblings, 0 replies; 6+ messages in thread
From: dinghao.liu @ 2020-06-01 5:21 UTC (permalink / raw)
To: Ben Skeggs
Cc: kjlu, David Airlie, ML nouveau, LKML, ML dri-devel, Ben Skeggs,
Markus.Elfring
> > If there's *any* error, it'll check the pointer, if it's non-NULL,
> > it'll call the destructor. If kzalloc() fails, the pointer will be
> > NULL, there's no double-free bug. *every* subdev is written this way
> > to avoid duplicating cleanup logic.
> Actually, gm20b_clk_new_speedo0() may have a bug here if kzalloc()
> fails as it doesn't overwrite the previous pointer from
> gm20b_clk_new(). That whole ctor() sequence is written a little
> strangely.
>
It's clear to me, thank your for your explanation! As for
gm20b_clk_new_speedo0(), I think its bug pattern is not very
clear. Maybe we should keep it until we find an use chain that
could lead to a bug.
Regards,
Dinghao
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-01 5:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-29 8:00 [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new Dinghao Liu
2020-05-31 21:26 ` Ben Skeggs
2020-06-01 3:26 ` dinghao.liu
2020-06-01 3:37 ` Ben Skeggs
2020-06-01 3:41 ` Ben Skeggs
2020-06-01 5:21 ` dinghao.liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).