* [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
@ 2021-09-07 21:06 Christophe JAILLET
2021-09-08 6:27 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Christophe JAILLET @ 2021-09-07 21:06 UTC (permalink / raw)
To: minyard, zweiss, andrew
Cc: openipmi-developer, linux-kernel, kernel-janitors, Christophe JAILLET
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 <christophe.jaillet@wanadoo.fr>
---
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() */
port = kzalloc(sizeof(*port), GFP_KERNEL);
- if (!(priv && port))
+ if (!port)
return -ENOMEM;
port->id.type = SERIO_8042;
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
2021-09-07 21:06 [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()' Christophe JAILLET
@ 2021-09-08 6:27 ` Dan Carpenter
2021-09-08 6:50 ` Marion et Christophe JAILLET
2021-10-29 4:09 ` Andrew Jeffery
2021-10-29 12:25 ` Corey Minyard
2 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-09-08 6:27 UTC (permalink / raw)
To: Christophe JAILLET
Cc: minyard, zweiss, andrew, openipmi-developer, linux-kernel,
kernel-janitors
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 <christophe.jaillet@wanadoo.fr>
> ---
> 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().
Is there a trick to finding ->release() functions?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
2021-09-08 6:27 ` Dan Carpenter
@ 2021-09-08 6:50 ` Marion et Christophe JAILLET
2021-09-08 11:32 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Marion et Christophe JAILLET @ 2021-09-08 6:50 UTC (permalink / raw)
To: Dan Carpenter
Cc: minyard, zweiss, andrew, openipmi-developer, linux-kernel,
kernel-janitors
> 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.
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.
CJ
>
> Is there a trick to finding ->release() functions?
>
> regards,
> dan carpenter
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
2021-09-08 6:50 ` Marion et Christophe JAILLET
@ 2021-09-08 11:32 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-09-08 11:32 UTC (permalink / raw)
To: Marion et Christophe JAILLET
Cc: minyard, zweiss, andrew, openipmi-developer, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
2021-09-07 21:06 [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()' Christophe JAILLET
2021-09-08 6:27 ` Dan Carpenter
@ 2021-10-29 4:09 ` Andrew Jeffery
2021-10-29 12:25 ` Corey Minyard
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Jeffery @ 2021-10-29 4:09 UTC (permalink / raw)
To: Christophe JAILLET, Corey Minyard, Zev Weiss
Cc: openipmi-developer, linux-kernel, kernel-janitors
On Wed, 8 Sep 2021, at 06:36, 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 <christophe.jaillet@wanadoo.fr>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Sorry for the delay
> ---
> 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() */
> port = kzalloc(sizeof(*port), GFP_KERNEL);
> - if (!(priv && port))
> + if (!port)
> return -ENOMEM;
>
> port->id.type = SERIO_8042;
> --
> 2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
2021-09-07 21:06 [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()' Christophe JAILLET
2021-09-08 6:27 ` Dan Carpenter
2021-10-29 4:09 ` Andrew Jeffery
@ 2021-10-29 12:25 ` Corey Minyard
2 siblings, 0 replies; 6+ messages in thread
From: Corey Minyard @ 2021-10-29 12:25 UTC (permalink / raw)
To: Christophe JAILLET
Cc: zweiss, andrew, openipmi-developer, linux-kernel, kernel-janitors
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.
Yeah, looks reasonable. It's in my queue.
-corey
>
> Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> 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() */
> port = kzalloc(sizeof(*port), GFP_KERNEL);
> - if (!(priv && port))
> + if (!port)
> return -ENOMEM;
>
> port->id.type = SERIO_8042;
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-29 12:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 21:06 [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()' Christophe JAILLET
2021-09-08 6:27 ` Dan Carpenter
2021-09-08 6:50 ` Marion et Christophe JAILLET
2021-09-08 11:32 ` Dan Carpenter
2021-10-29 4:09 ` Andrew Jeffery
2021-10-29 12:25 ` Corey Minyard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.