[PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[jamal]

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

A resubmit against net-2.6 from about an hour ago.

cheers,
jamal


[-- Attachment #2: 0002-NET_CLS_ACT-Use-skb_act_clone.txt --]
[-- Type: application/mbox, Size: 1486 bytes --]

Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Sun, 21 Oct 2007 15:20:20 -0400

> A resubmit against net-2.6 from about an hour ago.

Applied.

[PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[jamal]

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


cheers,
jamal

[-- Attachment #2: 0002-NET_CLS_ACT-Use-skb_act_clone --]
[-- Type: text/plain, Size: 1314 bytes --]

[NET_CLS_ACT] Use skb_act_clone
clean skb_clone of any signs of CONFIG_NET_CLS_ACT and
have mirred us skb_act_clone()

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
 net/core/skbuff.c      |    7 -------
 net/sched/act_mirred.c |    2 +-
 2 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 70d9b5d..593a8da 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -415,13 +415,6 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 	n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
 	n->nohdr = 0;
 	n->destructor = NULL;
-#ifdef CONFIG_NET_CLS_ACT
-	/* FIXME What is this and why don't we do it in copy_skb_header? */
-	n->tc_verd = SET_TC_VERD(n->tc_verd,0);
-	n->tc_verd = CLR_TC_OK2MUNGE(n->tc_verd);
-	n->tc_verd = CLR_TC_MUNGED(n->tc_verd);
-	C(iif);
-#endif
 	C(truesize);
 	atomic_set(&n->users, 1);
 	C(head);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index fd7bca4..c3fde91 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -166,7 +166,7 @@ bad_mirred:
 		return TC_ACT_SHOT;
 	}
 
-	skb2 = skb_clone(skb, GFP_ATOMIC);
+	skb2 = skb_act_clone(skb, GFP_ATOMIC);
 	if (skb2 == NULL)
 		goto bad_mirred;
 	if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
-- 
1.4.4.1.gaed4

Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[Patrick McHardy]

jamal wrote:

[Can't quote for some reason ..]

I don't think this change is safe since we might loose the
redirect counter and have endless redirect loops. ipip for
example calls skb_realloc_headroom, which clones the skb
when delta > 0, so it seems we really need to copy tc_verd
in skb_clone(). Generally the assumption that drivers don't
clone packets doesn't seem safe.

Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[jamal]

On Tue, 2007-16-10 at 14:53 +0200, Patrick McHardy wrote:
> jamal wrote:
> 
> [Can't quote for some reason ..]

Sorry, trying to be clever with git-format-patch but chickened out
sending using git-am - so patch was just attached (Dave seems to be 
friendly to such an approach when he applies)

> I don't think this change is safe since we might loose the
> redirect counter and have endless redirect loops. ipip for
> example calls skb_realloc_headroom, which clones the skb
> when delta > 0, so it seems we really need to copy tc_verd
> in skb_clone(). Generally the assumption that drivers don't
> clone packets doesn't seem safe.

Does the copying of tc_verd in __copy_skb_header() not cover that?

cheers,
jamal

Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[Herbert Xu]

On Tue, Oct 16, 2007 at 09:22:31AM -0400, jamal wrote:
>
> Does the copying of tc_verd in __copy_skb_header() not cover that?

Yep, I'm pretty sure it does.

In fact this patch completely resolves my concerns about tc_verd.

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

Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[Patrick McHardy]

Herbert Xu wrote:
> On Tue, Oct 16, 2007 at 09:22:31AM -0400, jamal wrote:
>> Does the copying of tc_verd in __copy_skb_header() not cover that?
> 
> Yep, I'm pretty sure it does.
> 
> In fact this patch completely resolves my concerns about tc_verd.


Indeed, I was looking at the wrong tree :)

Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone

[jamal]

On Tue, 2007-16-10 at 16:10 +0200, Patrick McHardy wrote:
> Herbert Xu wrote:
> > On Tue, Oct 16, 2007 at 09:22:31AM -0400, jamal wrote:
> >> Does the copying of tc_verd in __copy_skb_header() not cover that?
> > 
> > Yep, I'm pretty sure it does.
> > 
> > In fact this patch completely resolves my concerns about tc_verd.
> 
> 
> Indeed, I was looking at the wrong tree :)

Ok, thanks guys.

cheers,
jamal