tc rsvp filter show broke

Hi Jamal,

Here is another strange 'tc' result that I think is a bug. If it was intended I can't see why. Show filter commands on rsvp classifiers only list the first entry?

# for i in {10..20}; do
tc filter add dev p3p2 protocol ip parent 8001: prio $i \
	rsvp session 12.0.0.$i ipproto udp classid 1:$i;
done

# tc filter show dev p3p2
filter parent 8001: protocol ip pref 10 rsvp
# tc filter show dev p3p2  prio 11
filter parent 8001: protocol ip rsvp
# tc filter show dev p3p2  prio 22

Although the entries must still be around because listing the prio explicitly show the entries. Also the case where the prio doesn't exist returns without any errors. A better response might be 'EINVAL' or something.

This is on 3.16 so nothing to do with my changes. I'm not sure I'll get a chance until Monday to look at it so figured I'll document it here in case someone else has a moment.

Thanks,
John

John Fastabend         Intel Corporation

Hrm - poking inside there and this seems to be one of those classifiers that hasnt been getting a lot of love. Doesnt support actions well for example.

It seems to work for me:
----------
/sbin/tc -V
tc utility, iproute2-ss140610

uname -a
Linux jhs-1 3.16.0-rc2+ #4 SMP Fri Aug 29 11:13:53 EDT 2014 x86_64 x86_64 x86_64 GNU/Linux

sudo /sbin/tc qdisc add dev eth0 root handle 1:0 prio
sudo /sbin/tc filter add dev eth0 pref 10 proto ip parent 1:0 rsvp session 10.0.0.1 ipproto icmp classid 1:1 police rate 1kbit burst 90k conform-exceed drop/ok /sbin/tc -s filter show dev eth0 parent 1:0
filter protocol ip pref 10 rsvp
filter protocol ip pref 10 rsvp fh 0x0001100a flowid 1:1 session 10.0.0.1 ipproto icmp
Sent 0 bytes 0 pkts (dropped 0, overlimits 0)

Now lets send a fast ping:
sudo ping -f 10.0.0.1 -c 10000
.......
10000 packets transmitted, 386 received, 96% packet loss, time 115495ms rtt min/avg/max/mdev = 0.300/0.499/1.262/0.164 ms, ipg/ewma 11.550/0.463 ms

/sbin/tc -s filter show dev eth0 parent 1:0
filter protocol ip pref 10 rsvp
filter protocol ip pref 10 rsvp fh 0x0001100a flowid 1:1 session 10.0.0.1 ipproto icmp
Sent 980000 bytes 10000 pkts (dropped 9614, overlimits 9614)

cheers,
jamal

This works for me as well try adding two or more policers and check if they all get printed.

Multiple actions wont work for this classifier. I have time, will send patch.

cheers,
jamal

Hmm, looks like there is a bug in police dump. Please try the attached patch.

Thanks.

index c28b0d3..1218900 100644
@@ -576,16 +576,18 @@ int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 			if (tcf_action_dump(skb, &exts->actions, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
-		} else if (exts->police) {
-			struct tc_action *act = tcf_exts_first_act(exts);
-			nest = nla_nest_start(skb, exts->police);
-			if (nest == NULL || !act)
-				goto nla_put_failure;
-			if (tcf_action_dump_old(skb, act, 0, 0) < 0)
-				goto nla_put_failure;
-			nla_nest_end(skb, nest);
 		}
 	}
+
+	if (exts->police) {
+		struct tc_action *act = tcf_exts_first_act(exts);
+		nest = nla_nest_start(skb, exts->police);
+		if (nest == NULL || !act)
+			goto nla_put_failure;
+		if (tcf_action_dump_old(skb, act, 0, 0) < 0)
+			goto nla_put_failure;
+		nla_nest_end(skb, nest);
+	}
 	return 0;
 
 nla_put_failure:

I don't think we need this change, (or perhaps it needs to be a bit more complete if something is missing) take a look a tcf_exts_validate(), notice the policer is added to act->list so the if block in dump() work out,

int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct
nlattr **tb,
struct nlattr *rate_tlv, struct tcf_exts *exts, bool
ovr)
{

 #ifdef CONFIG_NET_CLS_ACT
         {
                 struct tc_action *act;

                 INIT_LIST_HEAD(&exts->actions);
                 if (exts->police && tb[exts->police]) {
                         act = tcf_action_init_1(net, tb[exts->police], 
rate_tlv,
                                                 "police", ovr,
                                                 TCA_ACT_BIND);
                         if (IS_ERR(act))
                                 return PTR_ERR(act);

                         act->type = exts->type = TCA_OLD_COMPAT;
                         list_add(&act->list, &exts->actions);
			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

                 } else if (exts->action && tb[exts->action]) {
                         int err;
                         err = tcf_action_init(net, tb[exts->action], 
rate_tlv,
                                               NULL, ovr,
                                               TCA_ACT_BIND, 
&exts->actions);
                         if (err)
                                 return err;
                 }
         }

[...]

John Fastabend         Intel Corporation

Right.

Note, this exts->xx thing we should eventually kill. It was intended to allow for old binary compatibility (for about 10 years now). In the earlier days, policer was a speacial action and one could invoke it directly from "tc filter ....."
But over time, the more sane way is:
"tc filter .. action police ..."
We have killed ability to compile in the kernel policer to support to allow old tools to continue to use it at least 5 years ago. tc still has support but I doubt if anyone uses it. Otherwise there would have been a lot of fallout from
commit 4bfb21ca2043dc5251102e4debcc0ae27f7304e7 in iproute2.

So if there are any takers for patches, i think the first thing is to kill every call in iproute2 which calls police directly. And if nobody whines, then get rid of most of exts->XXX (at minimal there would be no need for exts->police)

cheers,
jamal

I thought it's clear that act->list is correct here. :)

I was thinking the dump logic was wrong, but you are right that it should match the logic in validate. Actually the bug is we forgot to copy exts->type, I am going to submit a patch below:

index 77147c8..aad6a67 100644
@@ -549,6 +549,7 @@ void tcf_exts_change(struct tcf_proto *tp, struct

tcf_exts *dst,
tcf_tree_lock(tp);
list_splice_init(&dst->actions, &tmp);
list_splice(&src->actions, &dst->actions);
+ dst->type = src->type;
tcf_tree_unlock(tp);
tcf_action_destroy(&tmp, TCA_ACT_UNBIND);
#endif

Yep it does seem to be missing...

Although with the latest code can we just drop tcf_exts_change() and call tcf_exts_validate() directly. I'll check in the morning but a quick glance makes me think it should be OK and simplifies things.

.John

John Fastabend         Intel Corporation