dinghao.liu at zju.edu.cn
2020-Jun-01 03:26 UTC
[Nouveau] [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
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
Ben Skeggs
2020-Jun-01 03:37 UTC
[Nouveau] [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
On Mon, 1 Jun 2020 at 13:27, <dinghao.liu at 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
Ben Skeggs
2020-Jun-01 03:41 UTC
[Nouveau] [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
On Mon, 1 Jun 2020 at 13:37, Ben Skeggs <skeggsb at gmail.com> wrote:> > On Mon, 1 Jun 2020 at 13:27, <dinghao.liu at 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
Seemingly Similar Threads
- [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
- [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
- drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
- drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
- [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new