Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new

[Ben Skeggs]

On Mon, 1 Jun 2020 at 13:37, Ben Skeggs <skeggsb@xxxxxxxxx> wrote:
>
> On Mon, 1 Jun 2020 at 13:27, <dinghao.liu@xxxxxxxxxx> 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