Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'

[Dan Carpenter <dan.carpenter@oracle.com>]

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@acm.org, zweiss@equinix.com, andrew@aj.id.au, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
> > 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