[PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Masayuki Nakagawa]

This patch simply replaces __kfree_skb() in exit path with kfree_skb().
In tcp_rcv_state_process(), generally skbs should be destroyed only when
the ref count is zero.
That is the way things are supposed to be done in the kernel.

This change might reveals a memory leak of skb.
If it happens, it would be because someone doesn't deal with the skb properly.

Signed-off-by: Masayuki Nakagawa <nakagawa.msy@ncos.nec.co.jp>

--- linux-2.6/net/ipv4/tcp_input.c.orig	2007-01-25 07:04:35.000000000 -0800
+++ linux-2.6/net/ipv4/tcp_input.c	2007-01-25 07:05:05.000000000 -0800
@@ -4423,8 +4423,6 @@ int tcp_rcv_state_process(struct sock *s
 			 * in the interest of security over speed unless
 			 * it's still in use.
 			 */
-			kfree_skb(skb);
-			return 0;
 		}
 		goto discard;

@@ -4634,7 +4632,7 @@ int tcp_rcv_state_process(struct sock *s

 	if (!queued) {
 discard:
-		__kfree_skb(skb);
+		kfree_skb(skb);
 	}
 	return 0;
 }

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On 26-01-2007 04:37, Masayuki Nakagawa wrote:
> This patch simply replaces __kfree_skb() in exit path with kfree_skb().
> In tcp_rcv_state_process(), generally skbs should be destroyed only when
> the ref count is zero.
> That is the way things are supposed to be done in the kernel.
> 
> This change might reveals a memory leak of skb.
> If it happens, it would be because someone doesn't deal with the skb properly.

So maybe for some time there should be added skb->users
test with a warning to reveal this?

Regards,
Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Herbert Xu]

On Fri, Jan 26, 2007 at 09:28:38AM +0100, Jarek Poplawski wrote:
> 
> So maybe for some time there should be added skb->users
> test with a warning to reveal this?

Or just replace __kfree_skb by kfree_skb.  The atomic operation (the
expensive part of kfree_skb vs. __kfree_skb) is only taken when it
is needed.

Cheers,
-- 
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

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Fri, Jan 26, 2007 at 08:16:06PM +1100, Herbert Xu wrote:
> On Fri, Jan 26, 2007 at 09:28:38AM +0100, Jarek Poplawski wrote:
> > 
> > So maybe for some time there should be added skb->users
> > test with a warning to reveal this?
> 
> Or just replace __kfree_skb by kfree_skb.  The atomic operation (the
> expensive part of kfree_skb vs. __kfree_skb) is only taken when it
> is needed.

But: 

> This change might reveals a memory leak of skb.
> If it happens, it would be because someone doesn't deal with the skb properly.

How do we know about those improper deals? 
I understand there should be no other users here
if it's __kfree_skb now. So I mean to test and warn
before kfree_skb for some debugging time.

Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Herbert Xu]

On Fri, Jan 26, 2007 at 10:49:50AM +0100, Jarek Poplawski wrote:
> 
> How do we know about those improper deals? 
> I understand there should be no other users here
> if it's __kfree_skb now. So I mean to test and warn
> before kfree_skb for some debugging time.

We only need to do that if there is a legitimate reason to use
__kfree_skb.  Which there was when this code was first written
since kfree_skb had an unconditional atomic op back then.

Now that it's a conditinoal atomic op __kfree_skb is no longer
necessary.

Cheers,
-- 
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

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Fri, Jan 26, 2007 at 08:52:51PM +1100, Herbert Xu wrote:
> On Fri, Jan 26, 2007 at 10:49:50AM +0100, Jarek Poplawski wrote:
> > 
> > How do we know about those improper deals? 
> > I understand there should be no other users here
> > if it's __kfree_skb now. So I mean to test and warn
> > before kfree_skb for some debugging time.
> 
> We only need to do that if there is a legitimate reason to use
> __kfree_skb.  Which there was when this code was first written
> since kfree_skb had an unconditional atomic op back then.
> 
> Now that it's a conditinoal atomic op __kfree_skb is no longer
> necessary.

I don't mean it's necessary. I mean now skb is freed
unconditionally and after this patch, if there is some
error in counting, skb will stay. I thought Masayuki
wrote about such possibility, but if I missed his
point, then the rest is really O.K.
 
Cheers,
Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Herbert Xu]

On Fri, Jan 26, 2007 at 11:18:38AM +0100, Jarek Poplawski wrote:
> 
> I don't mean it's necessary. I mean now skb is freed
> unconditionally and after this patch, if there is some
> error in counting, skb will stay. I thought Masayuki
> wrote about such possibility, but if I missed his
> point, then the rest is really O.K.

OK, I see what you mean.

I'm not aware of anybody who has coded in this way.  Alexey & Dave,
do you know of any place where __kfree_skb is used to free an skb
whose ref count is greater than 1?

Cheers,
-- 
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

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 26 Jan 2007 21:45:18 +1100

> I'm not aware of anybody who has coded in this way.  Alexey & Dave,
> do you know of any place where __kfree_skb is used to free an skb
> whose ref count is greater than 1?

Not at all.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Fri, Jan 26, 2007 at 09:45:18PM +1100, Herbert Xu wrote:
> On Fri, Jan 26, 2007 at 11:18:38AM +0100, Jarek Poplawski wrote:
> > 
> > I don't mean it's necessary. I mean now skb is freed
> > unconditionally and after this patch, if there is some
> > error in counting, skb will stay. I thought Masayuki
> > wrote about such possibility, but if I missed his
> > point, then the rest is really O.K.
> 
> OK, I see what you mean.
> 
> I'm not aware of anybody who has coded in this way.  Alexey & Dave,
> do you know of any place where __kfree_skb is used to free an skb
> whose ref count is greater than 1?

I'm sure it wasn't done on purpose! But it could hide
some errors anyway.

Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Fri, Jan 26, 2007 at 12:02:35PM +0100, Jarek Poplawski wrote:
> On Fri, Jan 26, 2007 at 09:45:18PM +1100, Herbert Xu wrote:
> > On Fri, Jan 26, 2007 at 11:18:38AM +0100, Jarek Poplawski wrote:
> > > 
> > > I don't mean it's necessary. I mean now skb is freed
> > > unconditionally and after this patch, if there is some
> > > error in counting, skb will stay. I thought Masayuki
> > > wrote about such possibility, but if I missed his
> > > point, then the rest is really O.K.
> > 
> > OK, I see what you mean.
> > 
> > I'm not aware of anybody who has coded in this way.  Alexey & Dave,
> > do you know of any place where __kfree_skb is used to free an skb
> > whose ref count is greater than 1?
> 
> I'm sure it wasn't done on purpose! But it could hide
> some errors anyway.

But not sure anymore...

I've only now read the original thread of this problem
and this long note about security. I need more time to
understand this, but now I'm not sure Masayuki's server
isn't doing something against the policy of this note
(at the first sight it looks like this kind of skb
isn't welcommed there).

So maybe I miss something, but there could be also
possibility this __kfree_skb prevents some errors in
a hard way - I'm not sure: on purpose or accidentally.

Have a lot of fun in the weekend,
Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Fri, Jan 26, 2007 at 02:18:08PM +0100, Jarek Poplawski wrote:
...
> I've only now read the original thread of this problem
> and this long note about security. I need more time to
> understand this, but now I'm not sure Masayuki's server
> isn't doing something against the policy of this note

This would be socket case then (not server), sorry. 

Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Alexey Kuznetsov]

Hello!

> do you know of any place where __kfree_skb is used to free an skb
> whose ref count is greater than 1?

No.

Actually, since kfree_skb is not inline, __kfree_skb could be made static
and remaining places still using it switched to kfree_skb.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Fri, Jan 26, 2007 at 09:45:18PM +1100, Herbert Xu wrote:
> On Fri, Jan 26, 2007 at 11:18:38AM +0100, Jarek Poplawski wrote:
> > 
> > I don't mean it's necessary. I mean now skb is freed
> > unconditionally and after this patch, if there is some
> > error in counting, skb will stay. I thought Masayuki
> > wrote about such possibility, but if I missed his
> > point, then the rest is really O.K.
> 
> OK, I see what you mean.

Now I see what I mean too... And it's very bad!

This patch was done to allow more skb->users here, so
this debugging would warn mainly about something we've
just enabled.

I was simply fooled by this change - how it was
possible to work all these days? I see, the main idea
is: the data from skb is copied at this level only and
no cloning (tcp_ipv4). Except tcp_ipv6.

So this patch is changing it to ipv6 way. It seems to
be quite brave. I think there was some reason to do
this "old way" and I hope consequences were analyzed.

There are also other doubts: what decisions are made?
According to tcp_rcv_state_process skb should be
dropped. tcp_v6_do_rcv seems to have other opinion.
And after this change there could be, probably, more
such doubtful places.

Maybe it would be sufficient to enable this by some
additional "if" and return code from conn_request
in the "if (th->syn)" block? 

Regards,
Jarek P.

Re: [PATCH] TCP: Replace __kfree_skb() with kfree_skb()

[Jarek Poplawski]

On Mon, Jan 29, 2007 at 09:26:16AM +0100, Jarek Poplawski wrote:
...
> So this patch is changing it to ipv6 way. It seems to
> be quite brave. I think there was some reason to do
> this "old way" and I hope consequences were analyzed.
...

But since Alexey Kuznetsov accepted this change too,
they surely were. Let's forget about my doubts.

Sorry for disturbing,
Jarek P.