[PATCH -next] net: sched: remove TC_MUNGED bits

[PATCH -next] net: sched: remove TC_MUNGED bits

[Florian Westphal]

Not used.

pedit sets TC_MUNGED when packet content was altered, but all the core
does is unset MUNGED again and then set OK2MUNGE.

And the latter isn't tested anywhere. So lets remove both
TC_MUNGED and TC_OK2MUNGE.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 This is part of a (larger) patchset to dissect tc_verd to determine
 which parts of the state/information kept therein is required.

 I will send the other changes if this one is accepted.

 Documentation/networking/tc-actions-env-rules.txt | 2 --
 include/net/sch_generic.h                         | 2 --
 include/uapi/linux/pkt_cls.h                      | 3 +++
 net/sched/act_api.c                               | 5 -----
 net/sched/act_pedit.c                             | 5 +----
 5 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/Documentation/networking/tc-actions-env-rules.txt b/Documentation/networking/tc-actions-env-rules.txt
index 70d6cf6..95c7171 100644
--- a/Documentation/networking/tc-actions-env-rules.txt
+++ b/Documentation/networking/tc-actions-env-rules.txt
@@ -14,8 +14,6 @@ resets them for you, so invoke skb_act_clone() rather than skb_clone().
 
 2) If you munge any packet thou shalt call pskb_expand_head in the case
 someone else is referencing the skb. After that you "own" the skb.
-You must also tell us if it is ok to munge the packet (TC_OK2MUNGE),
-this way any action downstream can stomp on the packet.
 
 3) Dropping packets you don't own is a no-no. You simply return
 TC_ACT_SHOT to the caller and they will drop it.
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 6d778ef..994b5a0 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -755,8 +755,6 @@ static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask,
 
 	if (n) {
 		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);
 	}
 	return n;
 }
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index bf08e76..6810ca4 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -35,6 +35,8 @@ bits 9,10,11: redirect counter -  redirect TTL. Loop avoidance
  *
  * */
 
+#ifndef __KERNEL__
+/* backwards compat for userspace only */
 #define TC_MUNGED          _TC_MAKEMASK1(0)
 #define SET_TC_MUNGED(v)   ( TC_MUNGED | (v & ~TC_MUNGED))
 #define CLR_TC_MUNGED(v)   ( v & ~TC_MUNGED)
@@ -42,6 +44,7 @@ bits 9,10,11: redirect counter -  redirect TTL. Loop avoidance
 #define TC_OK2MUNGE        _TC_MAKEMASK1(1)
 #define SET_TC_OK2MUNGE(v)   ( TC_OK2MUNGE | (v & ~TC_OK2MUNGE))
 #define CLR_TC_OK2MUNGE(v)   ( v & ~TC_OK2MUNGE)
+#endif
 
 #define S_TC_VERD          _TC_MAKE32(2)
 #define M_TC_VERD          _TC_MAKEMASK(4,S_TC_VERD)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3d43e49..af427a3 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -392,11 +392,6 @@ int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 	list_for_each_entry(a, actions, list) {
 repeat:
 		ret = a->ops->act(skb, a, res);
-		if (TC_MUNGED & skb->tc_verd) {
-			/* copied already, allow trampling */
-			skb->tc_verd = SET_TC_OK2MUNGE(skb->tc_verd);
-			skb->tc_verd = CLR_TC_MUNGED(skb->tc_verd);
-		}
 		if (ret == TC_ACT_REPEAT)
 			goto repeat;	/* we need a ttl - JHS */
 		if (ret != TC_ACT_PIPE)
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 59649d5..17e6d66 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -108,7 +108,7 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a,
 		     struct tcf_result *res)
 {
 	struct tcf_pedit *p = a->priv;
-	int i, munged = 0;
+	int i;
 	unsigned int off;
 
 	if (skb_unclone(skb, GFP_ATOMIC))
@@ -156,11 +156,8 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a,
 			*ptr = ((*ptr & tkey->mask) ^ tkey->val);
 			if (ptr == &_data)
 				skb_store_bits(skb, off + offset, ptr, 4);
-			munged++;
 		}
 
-		if (munged)
-			skb->tc_verd = SET_TC_MUNGED(skb->tc_verd);
 		goto done;
 	} else
 		WARN(1, "pedit BUG: index %d\n", p->tcf_index);
-- 
2.0.5

Re: [PATCH -next] net: sched: remove TC_MUNGED bits

[Alexei Starovoitov]

On Thu, Apr 30, 2015 at 12:12:00PM +0200, Florian Westphal wrote:
> Not used.
> 
> pedit sets TC_MUNGED when packet content was altered, but all the core
> does is unset MUNGED again and then set OK2MUNGE.
> 
> And the latter isn't tested anywhere. So lets remove both
> TC_MUNGED and TC_OK2MUNGE.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Wanted to do the same.
iproute2 doesn't use 'munge' flag either.

Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Re: [PATCH -next] net: sched: remove TC_MUNGED bits

[Jamal Hadi Salim]

On 04/30/15 17:16, Alexei Starovoitov wrote:
> On Thu, Apr 30, 2015 at 12:12:00PM +0200, Florian Westphal wrote:
>> Not used.
>>
>> pedit sets TC_MUNGED when packet content was altered, but all the core
>> does is unset MUNGED again and then set OK2MUNGE.
>>
>> And the latter isn't tested anywhere. So lets remove both
>> TC_MUNGED and TC_OK2MUNGE.
>>
>> Signed-off-by: Florian Westphal <fw@strlen.de>
>
> Wanted to do the same.
> iproute2 doesn't use 'munge' flag either.
>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>

Florian,
If you are going to take this path then fix pedit to do a pskb_expand.

I think it would be better to fix the actions that do
pskb_expand_head() and let them indicated they were munged.
The flag was intended to be an optimization where it would indicate
to the action processing a packet to not bother and just trample
on the packet if noone is referencing it.
That was the rule, unfortunately nobody paid attention and it
didnt matter because it doesnt seem there was a use case where two
actions in a graph would be editing packets one after the other).

cheers,
jamal

Re: [PATCH -next] net: sched: remove TC_MUNGED bits

[Florian Westphal]

Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 04/30/15 17:16, Alexei Starovoitov wrote:
> >On Thu, Apr 30, 2015 at 12:12:00PM +0200, Florian Westphal wrote:
> >>Not used.
> >>
> >>pedit sets TC_MUNGED when packet content was altered, but all the core
> >>does is unset MUNGED again and then set OK2MUNGE.
> >>
> >>And the latter isn't tested anywhere. So lets remove both
> >>TC_MUNGED and TC_OK2MUNGE.
> >>
> >>Signed-off-by: Florian Westphal <fw@strlen.de>
> >
> >Wanted to do the same.
> >iproute2 doesn't use 'munge' flag either.
> >
> >Acked-by: Alexei Starovoitov <ast@plumgrid.com>
> >
> 
> Florian,
> If you are going to take this path then fix pedit to do a pskb_expand.

Jamal, what about this:

- I'll wait for this patch to be accepted or rejected
- same for your suggested rttl removal patch to go in

After that I will then send out all my pending tc_verd patches.

As for pedit, my suggestion would be to use skb_make_writeable(),
something like.... (untested):

-                       ptr = skb_header_pointer(skb, off + offset, 4, &_data);
-                       if (!ptr)
+                       if (!skb_make_writable(skb, off + offset + 4))
                                goto bad;
+
+                       ptr = skb->data + off + offset;
+

Does that sound ok?  I can send a followup patch to take care of pedit.

[ I'd first move skb_make_writeable out of netfilter core, of course ]

> I think it would be better to fix the actions that do
> pskb_expand_head() and let them indicated they were munged.

I don't think 'i was munged' flag is needed, the helper should
do on-demand copy if needed to get us exclusive access.

Thanks Jamal.

Re: [PATCH -next] net: sched: remove TC_MUNGED bits

[Jamal Hadi Salim]

Hi Florian,

On 05/01/15 05:46, Florian Westphal wrote:
> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 04/30/15 17:16, Alexei Starovoitov wrote:
>>> On Thu, Apr 30, 2015 at 12:12:00PM +0200, Florian Westphal wrote:
>>>> Not used.
>>>>
>>>> pedit sets TC_MUNGED when packet content was altered, but all the core
>>>> does is unset MUNGED again and then set OK2MUNGE.
>>>>
>>>> And the latter isn't tested anywhere. So lets remove both
>>>> TC_MUNGED and TC_OK2MUNGE.
>>>>
>>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>>
>>> Wanted to do the same.
>>> iproute2 doesn't use 'munge' flag either.
>>>
>>> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>>>
>>
>> Florian,
>> If you are going to take this path then fix pedit to do a pskb_expand.
>
> Jamal, what about this:
>
> - I'll wait for this patch to be accepted or rejected
> - same for your suggested rttl removal patch to go in
>

I am fine with what you suggest.
So here's my ack: Jamal Hadi Salim <jhs@mojatatu.com>

Acked-by:
Can you please CC me on the other patches so i can review?

cheers,
jamal

Re: [PATCH -next] net: sched: remove TC_MUNGED bits

[Daniel Borkmann]

On 04/30/2015 12:12 PM, Florian Westphal wrote:
> Not used.
>
> pedit sets TC_MUNGED when packet content was altered, but all the core
> does is unset MUNGED again and then set OK2MUNGE.
>
> And the latter isn't tested anywhere. So lets remove both
> TC_MUNGED and TC_OK2MUNGE.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Also looks good, thanks Florian!

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH -next] net: sched: remove TC_MUNGED bits

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Thu, 30 Apr 2015 12:12:00 +0200

> Not used.
> 
> pedit sets TC_MUNGED when packet content was altered, but all the core
> does is unset MUNGED again and then set OK2MUNGE.
> 
> And the latter isn't tested anywhere. So lets remove both
> TC_MUNGED and TC_OK2MUNGE.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Applied.