On Wed, Sep 08, 2021 at 08:50:14AM +0200, Marion et Christophe JAILLET wrote:
>
>
>
> > Message du 08/09/21 08:28
> > De : "Dan Carpenter"
> > A : "Christophe JAILLET"
> > Copie à : minyard@xxxxxxx, zweiss@xxxxxxxxxxx, andrew@xxxxxxxx, openipmi-developer@xxxxxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, kernel-janitors@xxxxxxxxxxxxxxx
> > Objet : Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
> >
> > On Tue, Sep 07, 2021 at 11:06:32PM +0200, Christophe JAILLET wrote:
> > > In the unlikely event where 'devm_kzalloc()' fails and 'kzalloc()'
> > > succeeds, 'port' would be leaking.
> > >
> > > Test each allocation separately to avoid the leak.
> > >
> > > Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor")
> > > Signed-off-by: Christophe JAILLET
> > > ---
> > > drivers/char/ipmi/kcs_bmc_serio.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> > > index 7948cabde50b..7e2067628a6c 100644
> > > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > > +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> > > @@ -73,10 +73,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
> > > struct serio *port;
> > >
> > > priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > >
> > > /* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
> >
> > The serio_unregister_port() calls serio_destroy_port() which calls
> > put_device(&serio->dev). But I wasn't able to track it further than
> > that to the actual kfree().
>
> Hi Dan,
>
> Checking this release path was not the goal of this patch.
Yeah. I was just curious.
> It was only about the VERRYYYY unlikely memory leak.
>
> However my understanding is:
> kcs_bmc_serio_add_device
> --> serio_register_port
> --> __serio_register_port
> --> serio_init_port
> --> serio->dev.release = serio_release_port
>
> And in serio_release_port:
> struct serio *serio = to_serio_port(dev);
> kfree(serio);
>
> For me, this 'serio' looks to the one allocated by 'kcs_bmc_serio_add_device'.
> I think that the comment is correct.
Thanks. This really helps me actually. I could just make a list of
the functions which take a container_of(dev) get a struct serio and then
free it. Then if there is only one function that matches that, I could
assume it's what put_device() will call.
regards,
dan carpenter
