net: sched: use counter to break reclassify loops

Seems all we want here is to avoid endless 'goto reclassify' loop. tc_classify_compat even resets this counter when something other than TC_ACT_RECLASSIFY is returned, so this skb-counter doesn't break hypothetical loops induced by something other than perpetual TC_ACT_RECLASSIFY return values.

skb_act_clone is now identical to skb_clone, so just use that.

Tested with following (bogus) filter:
tc filter add dev eth0 parent ffff: \
protocol ip u32 match u32 0 0 police rate 10Kbit burst \
64000 mtu 1500 action reclassify

Acked-by Daniel Borkmann <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

Seems all we want here is to avoid endless 'goto reclassify' loop. tc_classify_compat even resets this counter when something other than TC_ACT_RECLASSIFY is returned, so this skb-counter doesn't break hypothetical loops induced by something other than perpetual TC_ACT_RECLASSIFY return values.

skb_act_clone is now identical to skb_clone, so just use that.

Tested with following (bogus) filter:
tc filter add dev eth0 parent ffff: \
protocol ip u32 match u32 0 0 police rate 10Kbit burst \
64000 mtu 1500 action reclassify

Acked-by Daniel Borkmann <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

---
  Documentation/networking/tc-actions-env-rules.txt |  4 ----
  include/net/sch_generic.h                         | 15 ---------------
  include/uapi/linux/pkt_cls.h                      |  2 +-
  net/sched/act_mirred.c                            |  2 +-
  net/sched/sch_api.c                               | 12 +++---------
  5 files changed, 5 insertions(+), 30 deletions(-)

Documentation/networking/tc-actions-env-rules.txt +0 -4

index 95c7171..f378146 100644
@@ -8,10 +8,6 @@ For example if your action queues a packet to be processed later,
  or intentionally branches by redirecting a packet, then you need to
  clone the packet.
 
-There are certain fields in the skb tc_verd that need to be reset so we
-avoid loops, etc.  A few are generic enough that skb_act_clone()
-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.
 

include/net/sch_generic.h +0 -15

index 994b5a0..a611ed4 100644
@@ -745,21 +745,6 @@ static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, unsigned int pktlen)
  	return rtab->data[slot];
  }
 
-#ifdef CONFIG_NET_CLS_ACT
-static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask,
-					    int action)
-{
-	struct sk_buff *n;
-
-	n = skb_clone(skb, gfp_mask);
-
-	if (n) {
-		n->tc_verd = SET_TC_VERD(n->tc_verd, 0);
-	}
-	return n;
-}
-#endif
-
  struct psched_ratecfg {
  	u64	rate_bytes_ps; /* bytes per second */
  	u32	mult;

include/uapi/linux/pkt_cls.h +1 -1

index 596ffa0..ffc112c 100644
@@ -44,13 +44,13 @@ 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)
  #define G_TC_VERD(x)       _TC_GETVALUE(x,S_TC_VERD,M_TC_VERD)
  #define V_TC_VERD(x)       _TC_MAKEVALUE(x,S_TC_VERD)
  #define SET_TC_VERD(v,n)   ((V_TC_VERD(n)) | (v & ~M_TC_VERD))
+#endif
 
  #define S_TC_FROM          _TC_MAKE32(6)
  #define M_TC_FROM          _TC_MAKEMASK(2,S_TC_FROM)

net/sched/act_mirred.c +1 -1

index 3f63cea..a42a3b2 100644
@@ -151,7 +151,7 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
  	}
 
  	at = G_TC_AT(skb->tc_verd);
-	skb2 = skb_act_clone(skb, GFP_ATOMIC, m->tcf_action);
+	skb2 = skb_clone(skb, GFP_ATOMIC);
  	if (skb2 == NULL)
  		goto out;
 

net/sched/sch_api.c +3 -9

index ad9eed7..0b74dc0 100644
@@ -1816,13 +1816,8 @@ int tc_classify_compat(struct sk_buff *skb, const struct tcf_proto *tp,
  			continue;
  		err = tp->classify(skb, tp, res);
 
-		if (err >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-			if (err != TC_ACT_RECLASSIFY && skb->tc_verd)
-				skb->tc_verd = SET_TC_VERD(skb->tc_verd, 0);
-#endif
+		if (err >= 0)
  			return err;
-		}
  	}
  	return -1;
  }

@@ -1834,23 +1829,22 @@ int tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
  	int err = 0;
  #ifdef CONFIG_NET_CLS_ACT
  	const struct tcf_proto *otp = tp;
+	int limit = 0;
  reclassify:
  #endif
 
  	err = tc_classify_compat(skb, tp, res);
  #ifdef CONFIG_NET_CLS_ACT
  	if (err == TC_ACT_RECLASSIFY) {
-		u32 verd = G_TC_VERD(skb->tc_verd);
  		tp = otp;
 
-		if (verd++ >= MAX_REC_LOOP) {
+		if (unlikely(limit++ >= MAX_REC_LOOP)) {
  			net_notice_ratelimited("%s: packet reclassify loop rule prio %u protocol %02x\n",
  					       tp->q->ops->id,
  					       tp->prio & 0xffff,
  					       ntohs(tp->protocol));
  			return TC_ACT_SHOT;
  		}
-		skb->tc_verd = SET_TC_VERD(skb->tc_verd, verd);
  		goto reclassify;
  	}
  #endif

Florian,
In general i am in support of removing this - since the use case never materialized as being useful. However, this is not the same logic that was there before. To get equivalency you need to pass the limit into tc_classify_compat() so i can be reset.

BTW, a faster way to recreate
tc filter add dev eth0 parent ffff: \
protocol ip u32 match u32 0 0 \
action reclassify

Quoted 5 lines · Jamal Hadi Salim

Florian,
In general i am in support of removing this - since the use case never materialized as being useful. However, this is not the same logic that was there before. To get equivalency you need to pass the limit into tc_classify_compat() so i can be reset.

AFAICS this re-set only happens when we return something other than RECLASSIFY which means the caller will not check the limit.

So in fact it should be ok to remove this since the counter will always start from 0 on next tc_classify() invocation.

Quoted 7 lines · Florian Westphal

Quoted 5 lines · Jamal Hadi Salim

Florian,
In general i am in support of removing this - since the use case never materialized as being useful. However, this is not the same logic that was there before. To get equivalency you need to pass the limit into tc_classify_compat() so i can be reset.

Quoted 6 lines

AFAICS this re-set only happens when we return something other than RECLASSIFY which means the caller will not check the limit.

So in fact it should be ok to remove this since the counter will always start from 0 on next tc_classify() invocation.

Florian, consider the following scenario:
Assume X is the max allowed reclassified before bells start ringing. If we see upto X back-to-back reclassify - we are very much likely in a loop. We should see fire trucks arrive and bail out.
If we see X-1 "reclassify" followed by a "pipe" followed by X-1 "reclassify" followed by "ok" then that looks like a healthy policy. But that is a a total of 2X-2 reclassifies. You will bail out at X reclassifies; what i am saying is you shouldnt. And existing logic doesnt. Does that make sense?

Jamal Hadi Salim <[email protected]> wrote:

Yes, but, if we use your example above then:

tc_classify called
limit 0
tc_classify_compat called, ret RECLASSIFY
limit 1
tc_classify_compat called, ret RECLASSIFY
limit 2

     tc_classify_compat called, ret PIPE (== 3)

tc_classify returns 3
tc_classify called
limit 0
...

So we don't toss skb since any return value other than RECLASSIFY will make tc_classify() return to its caller, and when caller invokes tc_classify again the limit variable is set to 0 again.

Does that make sense to you?

Quoted 15 lines · Florian Westphal

Jamal Hadi Salim <[email protected]> wrote:
Yes, but, if we use your example above then:

tc_classify called
limit 0
tc_classify_compat called, ret RECLASSIFY
limit 1
tc_classify_compat called, ret RECLASSIFY
limit 2

    tc_classify_compat called, ret PIPE (== 3)

tc_classify returns 3
tc_classify called
limit 0
...

Quoted 6 lines

So we don't toss skb since any return value other than RECLASSIFY will make tc_classify() return to its caller, and when caller invokes tc_classify again the limit variable is set to 0 again.

Does that make sense to you?

I think you are right.

In any case, lets not slow this down; please add my acked-by and if i can think of something i will post. I have to take off, so i will go quiet for a while.

Seems all we want here is to avoid endless 'goto reclassify' loop. tc_classify_compat even resets this counter when something other than TC_ACT_RECLASSIFY is returned, so this skb-counter doesn't break hypothetical loops induced by something other than perpetual TC_ACT_RECLASSIFY return values.

skb_act_clone is now identical to skb_clone, so just use that.

Tested with following (bogus) filter:
tc filter add dev eth0 parent ffff: \
protocol ip u32 match u32 0 0 police rate 10Kbit burst \
64000 mtu 1500 action reclassify

Acked-by Daniel Borkmann <[email protected]>

Signed-off-by Florian Westphal <[email protected]>