[fw filter]: Broken! fw mark based tc class selection not working

[fw filter]: Broken! fw mark based tc class selection not working

[Akshat Kakkar]

Recently I came to know that,
Without any options fw classifier maps fwmark to classid.

tc filter add dev <iface> parent <qhandle> protocol ip prio 1 fw

i.e. if my packet has mark(0x10001) and class id is not set,
then above tc filter, will set class id = 0x10001 i.e. 1:1

But when I am trying it out, its not working!
I am having class 1:1 defined but its not at all hit.

Re: [fw filter]: Broken! fw mark based tc class selection not working

[Cong Wang]

On Fri, Sep 11, 2015 at 9:34 AM, Akshat Kakkar <akshat.1984@gmail.com> wrote:
> Recently I came to know that,
> Without any options fw classifier maps fwmark to classid.
>
> tc filter add dev <iface> parent <qhandle> protocol ip prio 1 fw
>
> i.e. if my packet has mark(0x10001) and class id is not set,
> then above tc filter, will set class id = 0x10001 i.e. 1:1
>
> But when I am trying it out, its not working!
> I am having class 1:1 defined but its not at all hit.

Did you specify a right filter handle? What is your setup?

Re: [fw filter]: Broken! fw mark based tc class selection not working

[Akshat Kakkar]

There is no handle with fw filter. That's the whole point is. If
handle and class (flow id) is not specified, then whatever be the mark
on the packet, its automatically set as flowid. So if mark is 0x10003,
then this fw filter

tc filter add dev eth0 parent 1:0 protocol ip fw

will cause 0x10003 being set as classid I.e. 1:3.


tc qdisc add dev eth0 root handle 1: htb
tc class add dev eth0 parent 1: classid 1:a htb rate 1mbit
tc class add dev eth0 parent 1: classid 1:b htb rate 1mbit
tc class add dev eth0 parent 1: classid 1:c htb rate 1mbit
tc filter add dev eth0 parent 1:0 protocol ip fw

iptables -t mangle -I OUTPUT -o eth0 -p tcp -j MARK --set-mark 0x1000a
iptables -t mangle -I OUTPUT -o eth0 -p icmp -j MARK --set-mark 0x1000b
iptables -t mangle -I OUTPUT -o eth0 -p udp -j MARK --set-mark 0x1000c

Re: [fw filter]: Broken! fw mark based tc class selection not working

[Cong Wang]

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

On Fri, Sep 11, 2015 at 3:24 PM, Akshat Kakkar <akshat.1984@gmail.com> wrote:
> There is no handle with fw filter. That's the whole point is. If
> handle and class (flow id) is not specified, then whatever be the mark
> on the packet, its automatically set as flowid. So if mark is 0x10003,
> then this fw filter
>
> tc filter add dev eth0 parent 1:0 protocol ip fw
>
> will cause 0x10003 being set as classid I.e. 1:3.
>
>
> tc qdisc add dev eth0 root handle 1: htb
> tc class add dev eth0 parent 1: classid 1:a htb rate 1mbit
> tc class add dev eth0 parent 1: classid 1:b htb rate 1mbit
> tc class add dev eth0 parent 1: classid 1:c htb rate 1mbit
> tc filter add dev eth0 parent 1:0 protocol ip fw
>
> iptables -t mangle -I OUTPUT -o eth0 -p tcp -j MARK --set-mark 0x1000a
> iptables -t mangle -I OUTPUT -o eth0 -p icmp -j MARK --set-mark 0x1000b
> iptables -t mangle -I OUTPUT -o eth0 -p udp -j MARK --set-mark 0x1000c


Hmm, I didn't know that before either. Looks like my tp->init change
breaks it.

Could you try the following patch?

Thanks!

[-- Attachment #2: cls_fw.diff --]
[-- Type: text/plain, Size: 971 bytes --]

diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 715e01e..b6394ab 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -34,6 +34,7 @@
 struct fw_head {
 	u32			mask;
 	bool			mask_set;
+	bool			old_method;
 	struct fw_filter __rcu	*ht[HTSIZE];
 	struct rcu_head		rcu;
 };
@@ -65,7 +66,7 @@ static int fw_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 	int r;
 	u32 id = skb->mark;
 
-	if (head != NULL) {
+	if (!head->old_method) {
 		id &= head->mask;
 
 		for (f = rcu_dereference_bh(head->ht[fw_hash(id)]); f;
@@ -120,6 +121,7 @@ static int fw_init(struct tcf_proto *tp)
 	if (head == NULL)
 		return -ENOBUFS;
 
+	head->old_method = true;
 	head->mask_set = false;
 	rcu_assign_pointer(tp->root, head);
 	return 0;
@@ -302,6 +304,7 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 	if (!handle)
 		return -EINVAL;
 
+	head->old_method = false;
 	if (!head->mask_set) {
 		head->mask = 0xFFFFFFFF;
 		if (tb[TCA_FW_MASK])

Re: [fw filter]: Broken! fw mark based tc class selection not working

[Jamal Hadi Salim]

On 09/11/15 20:00, Cong Wang wrote:
> On Fri, Sep 11, 2015 at 3:24 PM, Akshat Kakkar <akshat.1984@gmail.com> wrote:

> Hmm, I didn't know that before either. Looks like my tp->init change
> breaks it.
>
> Could you try the following patch?
>

I would just make init() empty for this classifier (return 0?).
If someone wants to add classids ids, change() is available.
The most common (efficient) use case is what Akshat shows.
So even the check in the classify should optimize for that i.e
if (head == NULL)
     do old method
else
     ...

cheers,
jamal

Re: [fw filter]: Broken! fw mark based tc class selection not working

[Cong Wang]

On Mon, Sep 14, 2015 at 5:28 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 09/11/15 20:00, Cong Wang wrote:
>>
>> On Fri, Sep 11, 2015 at 3:24 PM, Akshat Kakkar <akshat.1984@gmail.com>
>> wrote:
>
>
>> Hmm, I didn't know that before either. Looks like my tp->init change
>> breaks it.
>>
>> Could you try the following patch?
>>
>
> I would just make init() empty for this classifier (return 0?).
> If someone wants to add classids ids, change() is available.
> The most common (efficient) use case is what Akshat shows.
> So even the check in the classify should optimize for that i.e
> if (head == NULL)
>     do old method
> else
>     ...

That is exactly the original code. But it is not readable at all,
at least I still missed it when I touched the tp->init() part. :(
Having a boolean doesn't harm anything.

Re: [fw filter]: Broken! fw mark based tc class selection not working

[Jamal Hadi Salim]

On 09/14/15 18:04, Cong Wang wrote:

>
> That is exactly the original code. But it is not readable at all,
> at least I still missed it when I touched the tp->init() part. :(
> Having a boolean doesn't harm anything.

The default should really be no head alloced (given that is the
main use case).
The other part you can make more readable.

cheers,
jamal