Re: memory leak in net/sched/ipt.c?

Re: memory leak in net/sched/ipt.c?

[Herbert Xu]

Yichen Xie <yxie@cs.stanford.edu> wrote:
> Is the memory block allocated on line 315 leaked every time tcp_ipt_dump 
> is called?

It seems to be.  This patch should free it.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

BTW, please report networking bugs to netdev@oss.sgi.com.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
===== net/sched/ipt.c 1.14 vs edited =====
--- 1.14/net/sched/ipt.c	2005-02-07 16:39:40 +11:00
+++ edited/net/sched/ipt.c	2005-03-23 22:28:13 +11:00
@@ -284,10 +284,12 @@
 	tm.lastuse = jiffies_to_clock_t(jiffies - p->tm.lastuse);
 	tm.expires = jiffies_to_clock_t(p->tm.expires);
 	RTA_PUT(skb, TCA_IPT_TM, sizeof (tm), &tm);
+	kfree(t);
 	return skb->len;
 
       rtattr_failure:
 	skb_trim(skb, b - skb->data);
+	kfree(t);
 	return -1;
 }
 

Re: memory leak in net/sched/ipt.c?

[jamal]

On Wed, 2005-03-23 at 06:30, Herbert Xu wrote:
> Yichen Xie <yxie@cs.stanford.edu> wrote:
> > Is the memory block allocated on line 315 leaked every time tcp_ipt_dump 
> > is called?
> 
> It seems to be.  This patch should free it.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> BTW, please report networking bugs to netdev@oss.sgi.com.
> 

Thanks for pointing this out - I _know_ its in the kernel list FAQ.
I was sitting beside R Gooch when he added "thou shalt post to netdev".
And of course i am gonna bitch about it every time someone doesnt
post to netdev;-> 

Just a small correction to patchlet:
The second kfree should check for existence of t.

cheers,
jamal

> Thanks,

Re: memory leak in net/sched/ipt.c?

[Thomas Graf]

* jamal <1111581618.1088.72.camel@jzny.localdomain> 2005-03-23 07:40
> On Wed, 2005-03-23 at 06:30, Herbert Xu wrote:
> > Yichen Xie <yxie@cs.stanford.edu> wrote:
> > > Is the memory block allocated on line 315 leaked every time tcp_ipt_dump 
> > > is called?
> > 
> > It seems to be.  This patch should free it.
> > 
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

> Just a small correction to patchlet:
> The second kfree should check for existence of t.

t is either valid or NULL so it's not a problem, unless you want
to create janitor work of course. ;->

Re: memory leak in net/sched/ipt.c?

[jamal]

On Wed, 2005-03-23 at 07:55, Thomas Graf wrote:
> * jamal <1111581618.1088.72.camel@jzny.localdomain> 2005-03-23 07:40

> > Just a small correction to patchlet:
> > The second kfree should check for existence of t.
> 
> t is either valid or NULL so it's not a problem, unless you want
> to create janitor work of course. ;->

if t is null you still goto rtattr_failure
I have seen people put little comments of "kfree will work if you
pass it NULL" - are you saying such assumptions exist all over
net/sched? didnt understand the janitor part.

cheers,
jamal

Re: memory leak in net/sched/ipt.c?

[Thomas Graf]

* jamal <1111583497.1089.92.camel@jzny.localdomain> 2005-03-23 08:11
> On Wed, 2005-03-23 at 07:55, Thomas Graf wrote:
> > * jamal <1111581618.1088.72.camel@jzny.localdomain> 2005-03-23 07:40
> 
> > > Just a small correction to patchlet:
> > > The second kfree should check for existence of t.
> > 
> > t is either valid or NULL so it's not a problem, unless you want
> > to create janitor work of course. ;->
> 
> if t is null you still goto rtattr_failure
> I have seen people put little comments of "kfree will work if you
> pass it NULL" - are you saying such assumptions exist all over
> net/sched?

kfree simply does nothing if it is given a null pointer so that
goto rtattr_failure for t == NULL  is handled just fine without
a check. I will never get used to this behaviour and policy as
well though, it somewhat makes code less readable.

> didnt understand the janitor part.

It will probably be removed again by one of the regular 'remove
unnecessary pre kfree checks' patchsets.

Re: memory leak in net/sched/ipt.c?

[jamal]

On Wed, 2005-03-23 at 08:23, Thomas Graf wrote:

> 
> kfree simply does nothing if it is given a null pointer so that
> goto rtattr_failure for t == NULL  is handled just fine without
> a check. I will never get used to this behaviour and policy as
> well though, it somewhat makes code less readable.
> 

I cant get used to it either; actually i dont think i would be able 
to stop my fingers ;-> 

> > didnt understand the janitor part.
> 
> It will probably be removed again by one of the regular 'remove
> unnecessary pre kfree checks' patchsets.
> 

Yes, already Jan Engelhardt <jengelh@linux01.gwdg.de> has made that
point - although he did not post to netdev!! ;->

So ignore my comment Herbert

cheers,
jamal

Re: memory leak in net/sched/ipt.c?

[Jan Engelhardt]

>I have seen people put little comments of "kfree will work if you
>pass it NULL" - are you saying such assumptions exist all over
>net/sched?

Not only net/sched. The C standard requires that free(NULL) works.


Jan Engelhardt
-- 

Re: memory leak in net/sched/ipt.c?

[jamal]

On Wed, 2005-03-23 at 08:40, Jan Engelhardt wrote:
> >I have seen people put little comments of "kfree will work if you
> >pass it NULL" - are you saying such assumptions exist all over
> >net/sched?
> 
> Not only net/sched. The C standard requires that free(NULL) works.

Thanks for clarifying this.

cheers,
jamal

Re: memory leak in net/sched/ipt.c?

[jamal]

Actually its well documented even on the man pages - so should have not
even have bothered discussing it;-> Should get new brand of coffee.
Apologies for unecessarily abused electrons - which means dont 
respond ;->

cheers,
jamal

On Wed, 2005-03-23 at 09:05, jamal wrote:
> On Wed, 2005-03-23 at 08:40, Jan Engelhardt wrote:
> > >I have seen people put little comments of "kfree will work if you
> > >pass it NULL" - are you saying such assumptions exist all over
> > >net/sched?
> > 
> > Not only net/sched. The C standard requires that free(NULL) works.
> 
> Thanks for clarifying this.
> 
> cheers,
> jamal
> 
> 
> 

RE: memory leak in net/sched/ipt.c?

[Yichen Xie]

Thanks for confirming. Are you guys interested in this kind of leaks? I have
a list of about a hundred generated by our tool. -yichen

> -----Original Message-----
> From: Herbert Xu [mailto:herbert@gondor.apana.org.au] 
> Sent: Wednesday, March 23, 2005 3:31 AM
> To: Yichen Xie
> Cc: linux-kernel@vger.kernel.org; netdev@oss.sgi.com; 
> davem@davemloft.net; kaber@trash.net; hadi@cyberus.ca
> Subject: Re: memory leak in net/sched/ipt.c?
> 
> Yichen Xie <yxie@cs.stanford.edu> wrote:
> > Is the memory block allocated on line 315 leaked every time 
> > tcp_ipt_dump is called?
> 
> It seems to be.  This patch should free it.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> BTW, please report networking bugs to netdev@oss.sgi.com.
> 
> Thanks,
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> ===== net/sched/ipt.c 1.14 vs edited =====
> --- 1.14/net/sched/ipt.c	2005-02-07 16:39:40 +11:00
> +++ edited/net/sched/ipt.c	2005-03-23 22:28:13 +11:00
> @@ -284,10 +284,12 @@
>  	tm.lastuse = jiffies_to_clock_t(jiffies - p->tm.lastuse);
>  	tm.expires = jiffies_to_clock_t(p->tm.expires);
>  	RTA_PUT(skb, TCA_IPT_TM, sizeof (tm), &tm);
> +	kfree(t);
>  	return skb->len;
>  
>        rtattr_failure:
>  	skb_trim(skb, b - skb->data);
> +	kfree(t);
>  	return -1;
>  }
>  
> 

Re: memory leak in net/sched/ipt.c?

[Herbert Xu]

On Wed, Mar 23, 2005 at 10:27:32AM -0800, Yichen Xie wrote:
> Thanks for confirming. Are you guys interested in this kind of leaks? I have
> a list of about a hundred generated by our tool. -yichen

Certainly.  Please post all the networking leaks to netdev@oss.sgi.com.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt