NET: Clone the sk_buff->iif field properly
Subsystems (2)
RFC PATCH 1/1 NET: Clone the sk_buff->iif field properly
When sk_buffs are cloned the iif field of the new, cloned packet is neither zeroed out or copied from the existing sk_buff. The result is that the newly cloned sk_buff has garbage in the iif field which is a Bad Thing. This patch fixes this problem by copying the iif field along with the other sk_buff critical fields in __copy_skb_header().
This patch is needed by some of the labeled networking changes proposed for 2.6.25, does anyone have any objections?
---
net/core/skbuff.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
net/core/skbuff.c +1 -0
@@ -371,6 +371,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) { new->tstamp = old->tstamp; new->dev = old->dev; + new->iif = old->iif; new->transport_header = old->transport_header; new->network_header = old->network_header; new->mac_header = old->mac_header;
Quoted 8 lines · Paul Moore
When
sk_buffsare cloned the iif field of the new, cloned packet is neither zeroed out or copied from the existingsk_buff. The result is that the newly clonedsk_buffhas garbage in the iif field which is a Bad Thing. This patch fixes this problem by copying the iif field along with the othersk_buffcritical fields in__copy_skb_header().This patch is needed by some of the labeled networking changes proposed for 2.6.25, does anyone have any objections?
Probably Jamal could be the most interested (added to CC):
Regards,
Jarek P.
Quoted 18 lines
---
net/core/skbuff.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)net/core/skbuff.c +1
-0@@ -371,6 +371,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) { new->tstamp = old->tstamp; new->dev = old->dev; + new->iif = old->iif; new->transport_header = old->transport_header; new->network_header = old->network_header; new->mac_header = old->mac_header;
Quoted 7 lines · Jarek Poplawski
Quoted 2 lines · Paul Moore
This patch is needed by some of the labeled networking changes proposed for 2.6.25, does anyone have any objections?
Probably Jamal could be the most interested (added to CC):
Gracias Jarek.
Paul, (out of curiosity more than anything) what are the circumstances of the cloned skb - are you going to reinject it back at some point?
I cant think of any good reason why iif shouldnt be copied - thats how its been from the begining (dammit;->). The reason it hasnt mattered so far is everything that needs to write the iif never copied (refer to Documentation/networking/tc-actions-env-rules.txt). For correctness i think it should be copied. So no objections;
The better patch would be to just put it in skb_clone and remove it from tc_act_clone.
cheers,
jamal
Quoted 8 lines · jamal
Quoted 5 lines · Jarek Poplawski
Quoted 2 lines · Paul Moore
This patch is needed by some of the labeled networking changes proposed for 2.6.25, does anyone have any objections?
Probably Jamal could be the most interested (added to CC):
Gracias Jarek.
Yes, thank you. One of these days I need to learn some git commands other than clone, update, and push ;)
Quoted 2 lines
Paul, (out of curiosity more than anything) what are the circumstances of the cloned skb - are you going to reinject it back at some point?
Well, I'm not planning on reinjecting the cloned skb at present (doesn't mean I won't think up some contrived use in the future) but the stack appears to do this already in a few cases and it is causing problems when we try to perform access control on the cloned skb. The git-lblnet "horkage" in the -mm tree just before the holiday is the most notable example.
Quoted 5 lines
I cant think of any good reason why iif shouldnt be copied - thats how its been from the begining (dammit;->). The reason it hasnt mattered so far is everything that needs to write the iif never copied (refer to
Documentation/networking/tc-actions-env-rules.txt). For correctness i think it should be copied. So no objections;
Great.
Quoted 2 lines
The better patch would be to just put it in
skb_cloneand remove it fromtc_act_clone.
I assume you mean skb_act_clone()? That sounds like the best idea, I'll fixup the patch and resend it today for more review.
Thanks guys.
Signature · 2 lines
paul moore linux security @ hp
Quoted 12 lines · jamal
Quoted 5 lines · Jarek Poplawski
Quoted 2 lines · Paul Moore
This patch is needed by some of the labeled networking changes proposed for 2.6.25, does anyone have any objections?
Probably Jamal could be the most interested (added to CC):
Gracias Jarek.
Paul, (out of curiosity more than anything) what are the circumstances of the cloned skb - are you going to reinject it back at some point?
Quoted 7 lines
I cant think of any good reason why iif shouldnt be copied - thats how its been from the begining (dammit;->). The reason it hasnt mattered so far is everything that needs to write the iif never copied (refer to
Documentation/networking/tc-actions-env-rules.txt). For correctness i think it should be copied. So no objections; The better patch would be to just put it inskb_cloneand remove it fromtc_act_clone.
While I'm at it, is there some reason for this #define in __skb_clone()?
#define C(x) n->x = skb->x
... it seems kinda silly to me and I tend to think the code would be better without it.
Signature · 2 lines
paul moore linux security @ hp
On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
...
Quoted 6 lines
While I'm at it, is there some reason for this #define in
__skb_clone()?#define C(x)
n->x=skb->x... it seems kinda silly to me and I tend to think the code would be better without it.
IMHO, if there are a lot of this, it's definitely more readable: easier to check which values are simply copied and which need something more. But, as usual, it's probably a question of taste, and of course without it it would definitely look classier...
Cheers,
Jarek P.
PS: I hope you didn't suggest earlier my (better?) knowlege of git; otherwise don't bother: with your git push you are far ahead of my gitweb 'degree'.
Quoted 8 lines · Jarek Poplawski
On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
...Quoted 5 lines
While I'm at it, is there some reason for this #define in
__skb_clone()?#define C(x)
n->x=skb->x
Quoted 7 lines
Quoted 2 lines
... it seems kinda silly to me and I tend to think the code would be better without it.
IMHO, if there are a lot of this, it's definitely more readable:
easier to check which values are simply copied and which need something more. But, as usual, it's probably a question of taste, and of course without it it would definitely look classier...
For me personally, I would argue the readability bit. Whenever I see a function/macro call I have to go find the function/macro definition before I can understand what it is doing. Granted, the macro is defined "local" to the function but my point is that being able to look at a line of code and understand it without having to look elsewhere is a nice quality. To loose that simply because someone wants to save a few keystrokes is a mistake from my point of view.
Besides, if we are really interested in writing a kernel with the least number of keystrokes possible wouldn't we be doing it in perl? I'm sure somebody out there has ported the current kernel source to a single line of perl ... ;)
Quoted 3 lines
PS: I hope you didn't suggest earlier my (better?) knowlege of git; otherwise don't bother: with your git push you are far ahead of my gitweb 'degree'.
;)
On a serious note, your comment about gitweb made me poke around with some of the extra little features ... that 'history' link for each file is pretty cool!
Signature · 2 lines
paul moore linux security @ hp
On Thu, Jan 03, 2008 at 04:20:06PM -0500, Paul Moore wrote:
...
Quoted 7 lines
For me personally, I would argue the readability bit. Whenever I see a function/macro call I have to go find the function/macro definition before I can understand what it is doing. Granted, the macro is defined "local" to the function but my point is that being able to look at a line of code and understand it without having to look elsewhere is a nice quality. To loose that simply because someone wants to save a few keystrokes is a mistake from my point of view.
When I first read this __skb_clone() I had mixed emotions about this macro too. Later I didn't think about it, and only now, after your question I've done a quick test, compared with __copy_skb_header() and it seems there is really less rightwords eye moving. So, it was only about this one very "local" macro.
I'm not macros fan in general: just yesterday I've cursed a bit at some guy (I forgot the name...), who gave all these "meaningful" names to macros in linux/pkt_cls.h. But, maybe after some time I'll start to defend them as well... Especially when I try to imagine doing the same without them.
Jarek P.
On Thu, Jan 03, 2008 at 11:06:08PM +0100, Jarek Poplawski wrote:
...
Quoted 5 lines
I'm not macros fan in general: just yesterday I've cursed a bit at some guy (I forgot the name...), who gave all these "meaningful" names to macros in
linux/pkt_cls.h. But, maybe after some time I'll start to defend them as well... Especially when I try to imagine doing the same without them.
...Jamal, as a matter of fact, I don't know why, just about writing this previous message, I started to like all these names without exception! Isn't it a miracle?
Jarek P.
David Miller
maintainerFrom: Paul Moore <[email protected]>
Date: Thu, 3 Jan 2008 16:20:06 -0500
Quoted 12 lines · Jarek Poplawski
Quoted 11 lines · Paul Moore
...
Quoted 7 lines
While I'm at it, is there some reason for this #define in
__skb_clone()?#define C(x)
n->x=skb->x... it seems kinda silly to me and I tend to think the code would be better without it.
Quoted 6 lines
Quoted 4 lines
IMHO, if there are a lot of this, it's definitely more readable:
easier to check which values are simply copied and which need something more. But, as usual, it's probably a question of taste, and of course without it it would definitely look classier...For me personally, I would argue the readability bit.
I definitely think the C() thing is more readable.
Less typing, less reading...
Quoted 20 lines · David Miller
From: Paul Moore <[email protected]>
Date: Thu, 3 Jan 2008 16:20:06 -0500Quoted 17 lines · Jarek Poplawski
Quoted 15 lines · Paul Moore
...
Quoted 7 lines
While I'm at it, is there some reason for this #define in
__skb_clone()?#define C(x)
n->x=skb->x... it seems kinda silly to me and I tend to think the code would be better without it.
IMHO, if there are a lot of this, it's definitely more readable:
easier to check which values are simply copied and which need something more. But, as usual, it's probably a question of taste, and of course without it it would definitely look classier...
Quoted 5 lines
Quoted 1 line
For me personally, I would argue the readability bit.
I definitely think the
C()thing is more readable.Less typing, less reading...
Well, you're the boss :) I just put the C() macro back in, but I kept the reordering that was suggested to help reduce cacheline bounces since that still makes sense to me. The function now looks like this:
static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
{
#define C(x) n->x = skb->x
n->next = n->prev = NULL; n->sk = NULL; __copy_skb_header(n, skb); C(len); C(data_len); C(mac_len); n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len; n->cloned = 1; n->nohdr = 0; n->destructor = NULL; C(iif); C(tail); C(end); C(head); C(data); C(truesize); atomic_set(&n->users, 1); atomic_inc(&(skb_shinfo(skb)->dataref)); skb->cloned = 1; return n;
#undef C
}
Signature · 2 lines
paul moore linux security @ hp
David Miller
maintainerFrom: Paul Moore <[email protected]>
Date: Thu, 3 Jan 2008 18:13:57 -0500
Quoted 23 lines · David Miller
Quoted 22 lines
From: Paul Moore <[email protected]>
Date: Thu, 3 Jan 2008 16:20:06 -0500Quoted 18 lines · Jarek Poplawski
Quoted 8 lines · Paul Moore
...
Quoted 5 lines
While I'm at it, is there some reason for this #define in
__skb_clone()?#define C(x)
n->x=skb->xQuoted 7 lines
Quoted 2 lines
... it seems kinda silly to me and I tend to think the code would be better without it.
IMHO, if there are a lot of this, it's definitely more readable:
easier to check which values are simply copied and which need something more. But, as usual, it's probably a question of taste, and of course without it it would definitely look classier...For me personally, I would argue the readability bit.
Quoted 7 lines
Quoted 3 lines
I definitely think the
C()thing is more readable.Less typing, less reading...
Well, you're the boss :) I just put the
C()macro back in, but I kept the reordering that was suggested to help reduce cacheline bounces since that still makes sense to me. The function now looks like this:
Looks ok to me.
Quoted 28 lines · Paul Moore
static struct
sk_buff*__skb_clone(structsk_buff*n, structsk_buff*skb)
{
#define C(x)n->x=skb->xn->next = n->prev = NULL; n->sk = NULL; __copy_skb_header(n, skb); C(len); C(data_len); C(mac_len); n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len; n->cloned = 1; n->nohdr = 0; n->destructor = NULL; C(iif); C(tail); C(end); C(head); C(data); C(truesize); atomic_set(&n->users, 1); atomic_inc(&(skb_shinfo(skb)->dataref)); skb->cloned = 1; return n;
Quoted 1 line
#undef C
Perhaps move the skb->cloned = 1 to just after n->cloned = 1 or
skb->cloned = n->cloned = 1; or maybe skb->cloned = 1; C(cloned);
Quoted 31 lines · Joe Perches
Quoted 28 lines · Paul Moore
static struct
sk_buff*__skb_clone(structsk_buff*n, structsk_buff*skb)
{
#define C(x)n->x=skb->xn->next = n->prev = NULL; n->sk = NULL; __copy_skb_header(n, skb); C(len); C(data_len); C(mac_len); n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len; n->cloned = 1; n->nohdr = 0; n->destructor = NULL; C(iif); C(tail); C(end); C(head); C(data); C(truesize); atomic_set(&n->users, 1); atomic_inc(&(skb_shinfo(skb)->dataref)); skb->cloned = 1; return n;Quoted 1 line
#undef C
Quoted 6 lines
Perhaps move the
skb->cloned= 1 to just aftern->cloned= 1 orskb->cloned = n->cloned = 1; or maybe skb->cloned = 1; C(cloned);
I thought about that, but I kinda like how the parent-skb-only changes are grouped together at the end. I think the distinction helps readability, but then again we've already seen how subjective readability can be :)
Signature · 2 lines
paul moore linux security @ hp
David Miller
maintainerFrom: Paul Moore <[email protected]>
Date: Thu, 3 Jan 2008 22:19:03 -0500
Quoted 10 lines
Quoted 6 lines
Perhaps move the
skb->cloned= 1 to just aftern->cloned= 1 orskb->cloned = n->cloned = 1; or maybe skb->cloned = 1; C(cloned);I thought about that, but I kinda like how the parent-skb-only changes are grouped together at the end. I think the distinction helps readability, but then again we've already seen how subjective readability can be :)
I think either way is fine, as long as the stores are ordered properly. The ordering of the loads and little issues like this skb->cloned thing are much less important.
Actually, if you look at the generated assembler, GCC makes a mess of all of these bitfield accesses, largely destroying the pure store stream since it does a read/modify/write on a word for each bitfield set.