Message ID | 1312802283-9107-8-git-send-email-julia@diku.dk (mailing list archive) |
---|---|
State | Accepted, archived |
Commit | e3854b6e25d1b092c30c5f81a04fe6fc839b1e26 |
Delegated to: | Kumar Gala |
Headers | show |
On Mon, Aug 8, 2011 at 7:18 AM, Julia Lawall <julia@diku.dk> wrote: > diff --git a/arch/powerpc/sysdev/ehv_pic.c b/arch/powerpc/sysdev/ehv_pic.c > index af1a5df..b6731e4 100644 > --- a/arch/powerpc/sysdev/ehv_pic.c > +++ b/arch/powerpc/sysdev/ehv_pic.c > @@ -280,6 +280,7 @@ void __init ehv_pic_init(void) > > if (!ehv_pic->irqhost) { > of_node_put(np); > + kfree(ehv_pic); > return; > } Although the fix is correct, I think there is another bug in this function. 'np' is not released when the function finishes successfully. I've looked at other functions that use irq_alloc_host(), and most of them do the same thing: they don't call of_node_put() on the device node pointer. The only exception I've found is mpc5121_ads_cpld_pic_init(). Ben, Kumar: am I missing something? irq_alloc_host() calls of_node_get(): host->of_node = of_node_get(of_node); so doesn't that mean that the caller of irq_alloc_host() should release the device node pointer?
Julia Lawall wrote: > At this point, ehv_pic has been allocated but not stored anywhere, so it > should be freed before leaving the function. Acked-by: Timur Tabi <timur@freescale.com> FYI, Ashish is no longer with Freescale, so I've taken over maintainership of ehv_pic.
Ben, Kumar, can one of you take a look at my question and help me out? wrote: > On Mon, Aug 8, 2011 at 7:18 AM, Julia Lawall <julia@diku.dk> wrote: > >> diff --git a/arch/powerpc/sysdev/ehv_pic.c b/arch/powerpc/sysdev/ehv_pic.c >> index af1a5df..b6731e4 100644 >> --- a/arch/powerpc/sysdev/ehv_pic.c >> +++ b/arch/powerpc/sysdev/ehv_pic.c >> @@ -280,6 +280,7 @@ void __init ehv_pic_init(void) >> >> if (!ehv_pic->irqhost) { >> of_node_put(np); >> + kfree(ehv_pic); >> return; >> } > > Although the fix is correct, I think there is another bug in this > function. 'np' is not released when the function finishes > successfully. I've looked at other functions that use > irq_alloc_host(), and most of them do the same thing: they don't call > of_node_put() on the device node pointer. The only exception I've > found is mpc5121_ads_cpld_pic_init(). > > Ben, Kumar: am I missing something? irq_alloc_host() calls of_node_get(): > > host->of_node = of_node_get(of_node); > > so doesn't that mean that the caller of irq_alloc_host() should > release the device node pointer? >
On Aug 8, 2011, at 6:18 AM, Julia Lawall wrote: > From: Julia Lawall <julia@diku.dk> > > At this point, ehv_pic has been allocated but not stored anywhere, so it > should be freed before leaving the function. > > A simplified version of the semantic match that finds this problem is as > follows: (http://coccinelle.lip6.fr/) > > // <smpl> > @exists@ > local idexpression x; > statement S,S1; > expression E; > identifier fl; > expression *ptr != NULL; > @@ > > x = \(kmalloc\|kzalloc\|kcalloc\)(...); > ... > if (x == NULL) S > <... when != x > when != if (...) { <+...kfree(x)...+> } > when any > when != true x == NULL > x->fl > ...> > ( > if (x == NULL) S1 > | > if (...) { ... when != x > when forall > ( > return \(0\|<+...x...+>\|ptr\); > | > * return ...; > ) > } > ) > // </smpl> > > Signed-off-by: Julia Lawall <julia@diku.dk> > > --- > arch/powerpc/sysdev/ehv_pic.c | 1 + > 1 file changed, 1 insertion(+) applied to next - k
diff --git a/arch/powerpc/sysdev/ehv_pic.c b/arch/powerpc/sysdev/ehv_pic.c index af1a5df..b6731e4 100644 --- a/arch/powerpc/sysdev/ehv_pic.c +++ b/arch/powerpc/sysdev/ehv_pic.c @@ -280,6 +280,7 @@ void __init ehv_pic_init(void) if (!ehv_pic->irqhost) { of_node_put(np); + kfree(ehv_pic); return; }