[RFC PATCH] NET: Clone the sk_buff->iif field properly

[RFC PATCH] NET: Clone the sk_buff->iif field properly

[Paul Moore]

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(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5b4ce9b..9cb7bb7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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;

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Jarek Poplawski]

On 02-01-2008 17:01, Paul Moore wrote:
> 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?

Probably Jamal could be the most interested (added to CC):

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a057ae3c104dd2c661e55d2af37e70d168c65e00

Regards,
Jarek P.

> ---
> 
>  net/core/skbuff.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 5b4ce9b..9cb7bb7 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -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;
> 

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[jamal]

On Thu, 2008-03-01 at 10:58 +0100, Jarek Poplawski wrote:
> On 02-01-2008 17:01, Paul Moore wrote:

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

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Paul Moore]

On Thursday 03 January 2008 6:23:22 am jamal wrote:
> On Thu, 2008-03-01 at 10:58 +0100, Jarek Poplawski wrote:
> > On 02-01-2008 17:01, Paul Moore wrote:
> > > 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 ;)

> 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.

> 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.

> The better patch would be to just put it in skb_clone and remove it from
> tc_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.

-- 
paul moore
linux security @ hp

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Paul Moore]

On Thursday 03 January 2008 6:23:22 am jamal wrote:
> On Thu, 2008-03-01 at 10:58 +0100, Jarek Poplawski wrote:
> > On 02-01-2008 17:01, Paul Moore wrote:
> > > 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.

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.

-- 
paul moore
linux security @ hp

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Jarek Poplawski]

On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
...
> 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'.

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Paul Moore]

On Thursday 03 January 2008 4:13:12 pm Jarek Poplawski wrote:
> On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
> ...
>
> > 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...

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 ... ;)

> 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!

-- 
paul moore
linux security @ hp

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Jarek Poplawski]

On Thu, Jan 03, 2008 at 04:20:06PM -0500, Paul Moore wrote:
...
> 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.

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Jarek Poplawski]

On Thu, Jan 03, 2008 at 11:06:08PM +0100, Jarek Poplawski wrote:
...
> 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.

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Thu, 3 Jan 2008 16:20:06 -0500

> On Thursday 03 January 2008 4:13:12 pm Jarek Poplawski wrote:
> > On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
> > ...
> >
> > > 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...
> 
> For me personally, I would argue the readability bit.

I definitely think the C() thing is more readable.

Less typing, less reading...

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Paul Moore]

On Thursday 03 January 2008 6:05:18 pm David Miller wrote:
> From: Paul Moore <paul.moore@hp.com>
> Date: Thu, 3 Jan 2008 16:20:06 -0500
>
> > On Thursday 03 January 2008 4:13:12 pm Jarek Poplawski wrote:
> > > On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
> > > ...
> > >
> > > > 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...
> >
> > 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
}

-- 
paul moore
linux security @ hp

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Thu, 3 Jan 2008 18:13:57 -0500

> On Thursday 03 January 2008 6:05:18 pm David Miller wrote:
> > From: Paul Moore <paul.moore@hp.com>
> > Date: Thu, 3 Jan 2008 16:20:06 -0500
> >
> > > On Thursday 03 January 2008 4:13:12 pm Jarek Poplawski wrote:
> > > > On Thu, Jan 03, 2008 at 11:15:34AM -0500, Paul Moore wrote:
> > > > ...
> > > >
> > > > > 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...
> > >
> > > 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:

Looks ok to me.

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Joe Perches]

On Thu, 2008-01-03 at 18:13 -0500, Paul Moore wrote:
> 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

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);

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[Paul Moore]

On Thursday 03 January 2008 6:40:07 pm Joe Perches wrote:
> On Thu, 2008-01-03 at 18:13 -0500, Paul Moore wrote:
> > 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
>
> 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);

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 :)

-- 
paul moore
linux security @ hp

Re: [RFC PATCH] NET: Clone the sk_buff->iif field properly

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Thu, 3 Jan 2008 22:19:03 -0500

> > 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);
> 
> 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.