[net-next PATCH v2 0/1] tc reclassification needs to consider ether protocol changes

From: Jamal Hadi Salim <jhs@mojatatu.com>

v2:
Use Daniel's suggestion on when to set the skb proto
v1:
fix compile error found by build bot when CONFIG_NET_CLS_ACT was off


Jamal Hadi Salim (1):
  net_sched fix: reclassification needs to consider ether protocol
    changes

 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.9.1

From: Jamal Hadi Salim <jhs@mojatatu.com>

actions could change the etherproto in particular with ethernet
tunnelled data. Typically such actions, after peeling the outer header,
will ask for the packet to be  reclassified. We then need to restart
the classification with the new proto header.

Example setup used to catch this:
sudo tc qdisc add dev $ETH ingress
sudo $TC filter add dev $ETH parent ffff: pref 1 protocol 802.1Q \
u32 match u32 0 0 flowid 1:1 \
action  vlan pop reclassify

Fixes: 3b3ae880266d ("net: sched: consolidate tc_classify{,_compat}")
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index b5c2cf2..af46fee 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c

@@ -1822,7 +1822,6 @@ int tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 #ifdef CONFIG_NET_CLS_ACT
 	const struct tcf_proto *old_tp = tp;
 	int limit = 0;
-
 reclassify:
 #endif
 	for (; tp; tp = rcu_dereference_bh(tp->next)) {

@@ -1852,6 +1851,7 @@ reset:
 	}
 
 	tp = old_tp;
+	protocol = tc_skb_protocol(skb);
 	goto reclassify;
 #endif
 }

-- 
1.9.1

On Wed, Feb 17, 2016 at 3:37 AM, Jamal Hadi Salim [off-list ref] wrote:

Why remove this empty line? I think it is still useful. :)

Also your patch should be targeted to -net instead of -net-next, since it
fixes a bug.

Thanks.

On 02/17/2016 06:27 PM, Cong Wang wrote:

On Wed, Feb 17, 2016 at 3:37 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

From: Jamal Hadi Salim <jhs@mojatatu.com>

actions could change the etherproto in particular with ethernet
tunnelled data. Typically such actions, after peeling the outer header,
will ask for the packet to be  reclassified. We then need to restart
the classification with the new proto header.

Example setup used to catch this:
sudo tc qdisc add dev $ETH ingress
sudo $TC filter add dev $ETH parent ffff: pref 1 protocol 802.1Q \
u32 match u32 0 0 flowid 1:1 \
action  vlan pop reclassify

Fixes: 3b3ae880266d ("net: sched: consolidate tc_classify{,_compat}")
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
  net/sched/sch_api.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index b5c2cf2..af46fee 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c

@@ -1822,7 +1822,6 @@ int tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
  #ifdef CONFIG_NET_CLS_ACT
         const struct tcf_proto *old_tp = tp;
         int limit = 0;
-

Why remove this empty line? I think it is still useful. :)

Also your patch should be targeted to -net instead of -net-next, since it
fixes a bug.

Yeah, good point. That should go to -net (minus the unrelated white space change).

Thanks,
Daniel

On 16-02-18 06:53 AM, Daniel Borkmann wrote:

On 02/17/2016 06:27 PM, Cong Wang wrote:

quoted

quoted

         const struct tcf_proto *old_tp = tp;
         int limit = 0;
-

Why remove this empty line? I think it is still useful. :)

Also your patch should be targeted to -net instead of -net-next, since it
fixes a bug.

Yeah, good point. That should go to -net (minus the unrelated white
space change).

One patch coming right up boys;->

cheers,
jamal