[PATCH] [NET] warn when accounting an skb that already has a destructor

[PATCH] [NET] warn when accounting an skb that already has a destructor

[Johannes Berg]

This makes the networking layer warn when something tries to
charge an skb to a socket that already is charged to another
socket (or has a destructor from somewhere else.)

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Not sure if the code size increase is prohibitive, haven't really
checked. Might have helped in early mac80211 days (when we didn't
have the skb_orphan call) but for normal skb usage this shouldn't
ever happen.

 include/net/sock.h |    2 ++
 1 file changed, 2 insertions(+)

--- everything.orig/include/net/sock.h	2008-05-03 15:51:36.000000000 +0200
+++ everything/include/net/sock.h	2008-05-03 15:51:56.000000000 +0200
@@ -1167,6 +1167,7 @@ static inline void skb_set_owner_w(struc
 {
 	sock_hold(sk);
 	skb->sk = sk;
+	WARN_ON(skb->destructor);
 	skb->destructor = sock_wfree;
 	atomic_add(skb->truesize, &sk->sk_wmem_alloc);
 }
@@ -1174,6 +1175,7 @@ static inline void skb_set_owner_w(struc
 static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
 	skb->sk = sk;
+	WARN_ON(skb->destructor);
 	skb->destructor = sock_rfree;
 	atomic_add(skb->truesize, &sk->sk_rmem_alloc);
 	sk_mem_charge(sk, skb->truesize);

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sun, 04 May 2008 20:32:20 +0200

> This makes the networking layer warn when something tries to
> charge an skb to a socket that already is charged to another
> socket (or has a destructor from somewhere else.)
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

I completely agree that this a correct assertion, that
absolutely should never happen in the current tree.

But if we are going to do this, I'm pretty sure we'll
need to uninline these two routines.

I'll leave it up to you, if you think it's still worthwhile
with the uninlining, please resubmit :-)

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On Mon, 2008-05-05 at 00:20 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Sun, 04 May 2008 20:32:20 +0200
> 
> > This makes the networking layer warn when something tries to
> > charge an skb to a socket that already is charged to another
> > socket (or has a destructor from somewhere else.)
> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
> I completely agree that this a correct assertion, that
> absolutely should never happen in the current tree.
> 
> But if we are going to do this, I'm pretty sure we'll
> need to uninline these two routines.
> 
> I'll leave it up to you, if you think it's still worthwhile
> with the uninlining, please resubmit :-)

Given that it's a corner case and no regular net drivers would ever want
to do something that could possibly cause this condition to be true,
it's probably not worth it either way. Even mac80211 doesn't run into
it, although it might have helped a bit to remember to add skb_orphan()
to the re-injection.

If we decide to uninline those functions for another reason (used too
much, code size, ...) then we can still do that.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 05 May 2008 09:31:15 +0200

> If we decide to uninline those functions for another reason (used too
> much, code size, ...) then we can still do that.

Agreed.

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Ilpo Järvinen]

On Mon, 5 May 2008, David Miller wrote:

> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Mon, 05 May 2008 09:31:15 +0200
> 
> > If we decide to uninline those functions for another reason (used too
> > much, code size, ...) then we can still do that.
> 
> Agreed.

According to my measurements the size bloat of those two is
(x86/32bit, gcc 4.1.2 redhat something):

-1091  40 funcs, 89 +, 1180 -, diff: -1091 --- skb_set_owner_r
-495  46 funcs, 70 +, 565 -, diff: -495 --- skb_set_owner_w

-- 
 i.

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Mon, 2008-05-05 at 12:43 +0300, Ilpo Järvinen wrote:
> On Mon, 5 May 2008, David Miller wrote:
> 
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Mon, 05 May 2008 09:31:15 +0200
> > 
> > > If we decide to uninline those functions for another reason (used too
> > > much, code size, ...) then we can still do that.
> > 
> > Agreed.
> 
> According to my measurements the size bloat of those two is
> (x86/32bit, gcc 4.1.2 redhat something):
> 
> -1091  40 funcs, 89 +, 1180 -, diff: -1091 --- skb_set_owner_r
> -495  46 funcs, 70 +, 565 -, diff: -495 --- skb_set_owner_w

So do we want to out-line them? I don't really know the scale involved.
If so I can resubmit this patch with them outlined and the warnings
added (but did you account for the EXPORT_SYMBOL space?)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1063 bytes --]

On Mon, 5 May 2008, Johannes Berg wrote:

> On Mon, 2008-05-05 at 12:43 +0300, Ilpo Järvinen wrote:
> > On Mon, 5 May 2008, David Miller wrote:
> > 
> > > From: Johannes Berg <johannes@sipsolutions.net>
> > > Date: Mon, 05 May 2008 09:31:15 +0200
> > > 
> > > > If we decide to uninline those functions for another reason (used too
> > > > much, code size, ...) then we can still do that.
> > > 
> > > Agreed.
> > 
> > According to my measurements the size bloat of those two is
> > (x86/32bit, gcc 4.1.2 redhat something):
> > 
> > -1091  40 funcs, 89 +, 1180 -, diff: -1091 --- skb_set_owner_r
> > -495  46 funcs, 70 +, 565 -, diff: -495 --- skb_set_owner_w
> 
> So do we want to out-line them? I don't really know the scale involved.
> If so I can resubmit this patch with them outlined and the warnings
> added 

...It's not among the topmost of them, here's the full list:

http://www.cs.helsinki.fi/u/ijjarvin/inlines/sorted.v2.6.25-rc2-mm1

> (but did you account for the EXPORT_SYMBOL space?)

Not really. I don't even know how to measure such...

-- 
 i.

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]


> > (but did you account for the EXPORT_SYMBOL space?)
> 
> Not really. I don't even know how to measure such...

I guess it's not much, it seems to be an unsigned long, a pointer and
the name string.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 5 May 2008 12:43:25 +0300 (EEST)

> On Mon, 5 May 2008, David Miller wrote:
> 
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Mon, 05 May 2008 09:31:15 +0200
> > 
> > > If we decide to uninline those functions for another reason (used too
> > > much, code size, ...) then we can still do that.
> > 
> > Agreed.
> 
> According to my measurements the size bloat of those two is
> (x86/32bit, gcc 4.1.2 redhat something):
> 
> -1091  40 funcs, 89 +, 1180 -, diff: -1091 --- skb_set_owner_r
> -495  46 funcs, 70 +, 565 -, diff: -495 --- skb_set_owner_w

That's not too bad, but adding the WARN_ON() we're discussing
will plump that up a bit, percentage wise, which is why I
said we should inline it in such a case.

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Arnaldo Carvalho de Melo]

Em Mon, May 05, 2008 at 12:08:11PM -0700, David Miller escreveu:
> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 5 May 2008 12:43:25 +0300 (EEST)
> 
> > On Mon, 5 May 2008, David Miller wrote:
> > 
> > > From: Johannes Berg <johannes@sipsolutions.net>
> > > Date: Mon, 05 May 2008 09:31:15 +0200
> > > 
> > > > If we decide to uninline those functions for another reason (used too
> > > > much, code size, ...) then we can still do that.
> > > 
> > > Agreed.
> > 
> > According to my measurements the size bloat of those two is
> > (x86/32bit, gcc 4.1.2 redhat something):
> > 
> > -1091  40 funcs, 89 +, 1180 -, diff: -1091 --- skb_set_owner_r
> > -495  46 funcs, 70 +, 565 -, diff: -495 --- skb_set_owner_w
> 
> That's not too bad, but adding the WARN_ON() we're discussing
> will plump that up a bit, percentage wise, which is why I
> said we should inline it in such a case.

s/inline/uninline/g

According to some long e-mail today its worth ~198 unexport patches! 8-)

- Arnaldo

Re: [PATCH] [NET] warn when accounting an skb that already has a destructor

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1245 bytes --]

On Mon, 5 May 2008, Arnaldo Carvalho de Melo wrote:

> Em Mon, May 05, 2008 at 12:08:11PM -0700, David Miller escreveu:
> > From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> > Date: Mon, 5 May 2008 12:43:25 +0300 (EEST)
> > 
> > > On Mon, 5 May 2008, David Miller wrote:
> > > 
> > > > From: Johannes Berg <johannes@sipsolutions.net>
> > > > Date: Mon, 05 May 2008 09:31:15 +0200
> > > > 
> > > > > If we decide to uninline those functions for another reason (used too
> > > > > much, code size, ...) then we can still do that.
> > > > 
> > > > Agreed.
> > > 
> > > According to my measurements the size bloat of those two is
> > > (x86/32bit, gcc 4.1.2 redhat something):
> > > 
> > > -1091  40 funcs, 89 +, 1180 -, diff: -1091 --- skb_set_owner_r
> > > -495  46 funcs, 70 +, 565 -, diff: -495 --- skb_set_owner_w
> > 
> > That's not too bad, but adding the WARN_ON() we're discussing
> > will plump that up a bit, percentage wise, which is why I
> > said we should inline it in such a case.
> 
> s/inline/uninline/g
> 
> According to some long e-mail today its worth ~198 unexport patches! 8-)

...and there would be hundreths of similar with that order of magnitude,
thus one could, for starters, multiply that by ~200 or so... 8-D

-- 
 i.