| Message ID | 20190112025542.397-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net_sched: refetch skb protocol for each filter | expand |
On 2019-01-11 9:55 p.m., Cong Wang wrote: > Martin reported a set of filters don't work after changing > from reclassify to continue. Looking into the code, it > looks like skb protocol is not always fetched for each > iteration of the filters. But, as demonstrated by Martin, > TC actions could modify skb->protocol, for example act_vlan, > this means we have to refetch skb protocol in each iteration, > rather than using the one we fetch in the beginning of the loop. > > This bug is _not_ introduced by commit 3b3ae880266d > ("net: sched: consolidate tc_classify{,_compat}"), technically, > if act_vlan is the only action that modifies skb protocol, then > it is commit c7e2b9689ef8 ("sched: introduce vlan action") which > introduced this bug. > +Cc Lucas Do we have a test case for a setup like this in tdc? i.e incoming tagged and then vlan popped by action? Otherwise a test with IFE which resets the ethertype would be sufficient i.e just something that will messup with skb->protocol. Cong, I am a little worried about the impact of this change. Smells more like it has to do with Vlan action or related issues than with reclassifying. Martin, did this setup use to work before? If yes, rough idea of which kernels? cheers, jamal
On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote: > Do we have a test case for a setup like this in tdc? > i.e incoming tagged and then vlan popped by action? > Otherwise a test with IFE which resets the ethertype > would be sufficient i.e just something that will messup > with skb->protocol. And here is a slightly complex test with IFE. Wanted to show both reclassify and continue in play as well as something that change skb->protocol. I used VMs to test, but two hosts or two containers with veth between them will do. Martin, if you have the cycles please test this out. TLDR: Suspect is the vlan part. ----------- #Sender side egress setup. We are going to encode #all outgoing pings(icmp) to carry an skbmark 17 on the wire #and set the ethertype to 0xDEAD sudo $TC qdisc del dev $ETH root sudo $TC qdisc add dev $ETH root handle 1: prio sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 u32 \ match ip protocol 1 0xff \ flowid 1:2 \ action skbedit mark 17 \ action ife encode type 0xDEAD allow mark #receiver side ingress setup sudo $TC qdisc del dev $ETH ingress sudo $TC qdisc add dev $ETH ingress #decapsulate based on skb proto and then #reclassify sudo $TC filter add dev $ETH parent ffff: prio 2 protocol 0xdead u32 \ match u32 0 0 flowid 1:1 action ife decode reclassify #match icmp sudo $TC filter add dev $ETH parent ffff: prio 4 protocol ip u32 \ match ip protocol 1 0xff flowid 1:1 action continue # #now classify based on mark 17 (hex 11). # sudo $TC filter add dev $ETH parent ffff: prio 5 protocol ip \ handle 0x11 fw flowid 1:1 \ action ok On sender side, send 10 pings to receiver. Now lets see the receiver stats: --------------- root@jhs-foobar:# uname -a Linux jhs-foobar 4.20.0-rc7+ #2 SMP Fri Jan 12 9:29:35 EST 2019 x86_64 x86_64 x86_64 GNU/Linux root@jhs-foobar:# $TC -s filter ls dev $ETH parent ffff: filter protocol [57005] pref 2 u32 filter protocol [57005] pref 2 u32 fh 800: ht divisor 1 filter protocol [57005] pref 2 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1 (rule hit 10 success 10) match 00000000/00000000 at 0 (success 10 ) action order 1: ife decode action reclassify type 0x0 allow mark allow prio index 1 ref 1 bind 1 installed 40 sec used 19 sec Action statistics: Sent 1080 bytes 10 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 filter protocol ip pref 4 u32 filter protocol ip pref 4 u32 fh 801: ht divisor 1 filter protocol ip pref 4 u32 fh 801::800 order 2048 key ht 801 bkt 0 flowid 1:1 (rule hit 33 success 10) match 00010000/00ff0000 at 8 (success 10 ) action order 1: gact action continue random type none pass val 0 index 1 ref 1 bind 1 installed 40 sec used 19 sec Action statistics: Sent 1080 bytes 10 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 filter protocol ip pref 5 fw filter protocol ip pref 5 fw handle 0x11 classid 1:1 action order 1: gact action pass random type none pass val 0 index 2 ref 1 bind 1 installed 39 sec used 19 sec Action statistics: Sent 1080 bytes 10 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 --------------- As can be observed all the rules are hit as expected. cheers, jamal
On Sat, Jan 12, 2019 at 4:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > Cong, > I am a little worried about the impact of this change. Smells > more like it has to do with Vlan action or related issues > than with reclassifying. You can verify this patch with Martin's test case. I verified it with the script below, it works as expected. Not sure if it is feasible to add it to tc-tests as it needs a vlan device on top of veth. ip netns add test1 ip netns add test2 ip link add dev veth1 type veth peer name veth2 ip link set dev veth1 netns test1 up ip link set dev veth2 netns test2 up ip netns exec test1 ifconfig veth1 192.168.1.1 netmask 255.255.255.0 ip netns exec test2 ifconfig veth2 192.168.1.2 netmask 255.255.255.0 ip netns exec test1 ip link add link veth1 name veth1.100 type vlan id 100 ip netns exec test2 ip link add link veth2 name veth2.100 type vlan id 100 ip netns exec test1 ip link set link dev veth1.100 up ip netns exec test2 ip link set link dev veth2.100 up ip netns exec test1 ifconfig veth1.100 192.168.100.1 netmask 255.255.255.0 ip netns exec test2 ifconfig veth2.100 192.168.100.2 netmask 255.255.255.0 ip netns exec test2 ip link set dev veth2 address 00:c0:7b:7d:00:c8 ip netns exec test1 ip neighbor add 192.168.100.2 lladdr 00:c0:7b:7d:00:c8 dev veth1.100 nud permanent ip netns exec test1 tc qdisc add dev veth1 clsact ip netns exec test1 tc filter add dev veth1 egress prio 100 protocol 802.1Q matchall action vlan pop continue #reclassify ip netns exec test1 tc filter add dev veth1 egress prio 200 protocol ip u32 match ip src 192.168.1.0/24 action drop ip netns exec test1 tc filter add dev veth1 egress prio 201 protocol ip u32 match ip dst 192.168.100.0/24 action drop Thanks.
On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote: > > > Do we have a test case for a setup like this in tdc? > > i.e incoming tagged and then vlan popped by action? > > Otherwise a test with IFE which resets the ethertype > > would be sufficient i.e just something that will messup > > with skb->protocol. > > And here is a slightly complex test with IFE. > Wanted to show both reclassify and continue in play > as well as something that change skb->protocol. > I don't know why you need a complex one, Martin's test case is pretty simple (as I already sent to you). Also, you can add two printk()'s around the skb_vlan_pop() in tcf_vlan_act() to see the difference of tc_skb_protocol() return values before and after. I tried, it clearly shows ETH_P_8021Q and ETH_P_IP. Of course, it could be tc_skb_protocol() which is wrong, as skb->protocol stays same. This patch is always correct despite of tc_skb_protocol(): 1. If tc_skb_protocol() is wrong, this patch fixes nothing, and harms nothing. 2. If tc_skb_protocol() is correct, this patch fixes a bug. Changing tc_skb_protocol() is much more risky than this patch. You fixed a very similar bug before: commit 619fe32640b4b01f370574d50344ae0f62689816 Author: Jamal Hadi Salim <jhs@mojatatu.com> Date: Thu Feb 18 07:38:04 2016 -0500 net_sched fix: reclassification needs to consider ether protocol changes which also implies tc_skb_protocol() is correct. Thanks.
> "did this setup use to work before? If yes, rough idea of which > kernels?" Unknown. I haven't tried it before. /Martin On Sat, 12 Jan 2019, Jamal Hadi Salim wrote: > On 2019-01-11 9:55 p.m., Cong Wang wrote: >> Martin reported a set of filters don't work after changing >> from reclassify to continue. Looking into the code, it >> looks like skb protocol is not always fetched for each >> iteration of the filters. But, as demonstrated by Martin, >> TC actions could modify skb->protocol, for example act_vlan, >> this means we have to refetch skb protocol in each iteration, >> rather than using the one we fetch in the beginning of the loop. >> >> This bug is _not_ introduced by commit 3b3ae880266d >> ("net: sched: consolidate tc_classify{,_compat}"), technically, >> if act_vlan is the only action that modifies skb protocol, then >> it is commit c7e2b9689ef8 ("sched: introduce vlan action") which >> introduced this bug. >> > > +Cc Lucas > Do we have a test case for a setup like this in tdc? > i.e incoming tagged and then vlan popped by action? > Otherwise a test with IFE which resets the ethertype > would be sufficient i.e just something that will messup > with skb->protocol. > Cong, > I am a little worried about the impact of this change. Smells > more like it has to do with Vlan action or related issues > than with reclassifying. > Martin, > did this setup use to work before? If yes, rough idea of > which kernels? > > cheers, > jamal > >
On 01/12/2019 03:55 AM, Cong Wang wrote: > Martin reported a set of filters don't work after changing > from reclassify to continue. Looking into the code, it > looks like skb protocol is not always fetched for each > iteration of the filters. But, as demonstrated by Martin, > TC actions could modify skb->protocol, for example act_vlan, > this means we have to refetch skb protocol in each iteration, > rather than using the one we fetch in the beginning of the loop. > > This bug is _not_ introduced by commit 3b3ae880266d > ("net: sched: consolidate tc_classify{,_compat}"), technically, > if act_vlan is the only action that modifies skb protocol, then > it is commit c7e2b9689ef8 ("sched: introduce vlan action") which > introduced this bug. > > Reported-by: Martin Olsson <martin.olsson+netdev@sentorsecurity.com> > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Jiri Pirko <jiri@resnulli.us> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/sched/cls_api.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 8ce2a0507970..e2b5cb2eb34e 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -1277,7 +1277,6 @@ EXPORT_SYMBOL(tcf_block_cb_unregister); > int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, > struct tcf_result *res, bool compat_mode) > { > - __be16 protocol = tc_skb_protocol(skb); > #ifdef CONFIG_NET_CLS_ACT > const int max_reclassify_loop = 4; > const struct tcf_proto *orig_tp = tp; > @@ -1287,6 +1286,7 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, > reclassify: > #endif > for (; tp; tp = rcu_dereference_bh(tp->next)) { > + __be16 protocol = tc_skb_protocol(skb); > int err; > > if (tp->protocol != protocol && Can't we do something like the below instead? Otherwise we'll needlessly refetch protocol every time there is a mismatch in above tp->protocol check as well. diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 8ce2a05..dc725a1 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -1305,6 +1305,11 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, #endif if (err >= 0) return err; + + /* We also need to refetch protocol from here as e.g. + * act_vlan could have changed it. + */ + protocol = tc_skb_protocol(skb); } return TC_ACT_UNSPEC; /* signal: continue lookup */ Thanks, Daniel
On Mon, Jan 14, 2019 at 1:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > Can't we do something like the below instead? Otherwise we'll needlessly refetch > protocol every time there is a mismatch in above tp->protocol check as well. Does this save anything given the fact we simply return if err>=0? The protocol must be fetched, either before the loop or inside the loop, before this "if err>=0". If we return, it won't be refetched even with my patch. If we don't, it has to be refetched again. So, I don't see any difference here.
On Sat, Jan 12, 2019 at 7:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On 2019-01-11 9:55 p.m., Cong Wang wrote: > > Martin reported a set of filters don't work after changing > > from reclassify to continue. Looking into the code, it > > looks like skb protocol is not always fetched for each > > iteration of the filters. But, as demonstrated by Martin, > > TC actions could modify skb->protocol, for example act_vlan, > > this means we have to refetch skb protocol in each iteration, > > rather than using the one we fetch in the beginning of the loop. > > > > This bug is _not_ introduced by commit 3b3ae880266d > > ("net: sched: consolidate tc_classify{,_compat}"), technically, > > if act_vlan is the only action that modifies skb protocol, then > > it is commit c7e2b9689ef8 ("sched: introduce vlan action") which > > introduced this bug. > > > > +Cc Lucas > Do we have a test case for a setup like this in tdc? > i.e incoming tagged and then vlan popped by action? > Otherwise a test with IFE which resets the ethertype > would be sufficient i.e just something that will messup > with skb->protocol. Sorry, I've been a little caught up in things around here. There isn't currently a test case like this, but I can look at the example Cong provided and see if I can make it tdc-friendly. - Lucas
On 2019-01-13 3:58 p.m., Cong Wang wrote: > On Sat, Jan 12, 2019 at 4:23 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> Cong, >> I am a little worried about the impact of this change. Smells >> more like it has to do with Vlan action or related issues >> than with reclassifying. > > You can verify this patch with Martin's test case. I verified it > with the script below, it works as expected. > Your script is slightly different from his. Probably doesnt matter in the bigger picture - but lets give Lucas a chance to test. So a ping from test1->test2 should be sufficient here, yes? cheers, jamal
On 2019-01-14 4:15 a.m., Martin Olsson wrote: > >> "did this setup use to work before? If yes, rough idea of which kernels?" > > Unknown. I haven't tried it before. > Ok, IMO we are dealing with vlan breakage. cheers, jamal
On 2019-01-13 4:08 p.m., Cong Wang wrote: > On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> >> On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote: >> >>> Do we have a test case for a setup like this in tdc? >>> i.e incoming tagged and then vlan popped by action? >>> Otherwise a test with IFE which resets the ethertype >>> would be sufficient i.e just something that will messup >>> with skb->protocol. >> >> And here is a slightly complex test with IFE. >> Wanted to show both reclassify and continue in play >> as well as something that change skb->protocol. >> > > I don't know why you need a complex one, Martin's > test case is pretty simple (as I already sent to you). > Martin example had a "reclassify" with the vlan pop. Your example was showing a "continue" Note: "Reclassify" is typical how we have explained to users to deal with skb->protocol changes. I wanted to demonstrate that if i have a rule that followed that rule (as Martin did) with something other than vlan it would work: match u32 protocol someethertype .... action change vl reclassify match u32 protocol ip ... action continue match u32 protocol ip ... action blah infact that works before your patch. Basically it was an exercise to reduce _the variables_ from knowing that reclassify/continue do/nt work in presence of changing skb->protocol. IMO that example demonstrated that the current skb->protocol logic works and the vlan action maybe the guilty party. > Also, you can add two printk()'s around the skb_vlan_pop() > in tcf_vlan_act() to see the difference of tc_skb_protocol() > return values before and after. I tried, it clearly shows > ETH_P_8021Q and ETH_P_IP. > > Of course, it could be tc_skb_protocol() which is wrong, > as skb->protocol stays same. > > This patch is always correct despite of tc_skb_protocol(): > > 1. If tc_skb_protocol() is wrong, this patch fixes nothing, > and harms nothing. > > 2. If tc_skb_protocol() is correct, this patch fixes a bug. > > Changing tc_skb_protocol() is much more risky than this > patch. You fixed a very similar bug before: > > commit 619fe32640b4b01f370574d50344ae0f62689816 > Author: Jamal Hadi Salim <jhs@mojatatu.com> > Date: Thu Feb 18 07:38:04 2016 -0500 > > net_sched fix: reclassification needs to consider ether protocol changes > > which also implies tc_skb_protocol() is correct. I agree with your rationale. Lets have Lucas give this a run and we can move forward. cheers, jamal
On 2019-01-15 10:59 a.m., Jamal Hadi Salim wrote: > On 2019-01-13 4:08 p.m., Cong Wang wrote: >> On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> [..] > I agree with your rationale. > Lets have Lucas give this a run and we can move > forward. And as an addendum - Martin corrected me, he did have a "continue" after the vlan pop (not illegal to do). So i think your patch is fine; lets not mix it up with vlan not correctly matching (two separate issues). Lets have Lucas create the tdc test, then add my Ack. cheers, jamal
On Tue, Jan 15, 2019 at 7:59 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On 2019-01-13 4:08 p.m., Cong Wang wrote: > > On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > >> > >> On 2019-01-12 7:23 a.m., Jamal Hadi Salim wrote: > >> > >>> Do we have a test case for a setup like this in tdc? > >>> i.e incoming tagged and then vlan popped by action? > >>> Otherwise a test with IFE which resets the ethertype > >>> would be sufficient i.e just something that will messup > >>> with skb->protocol. > >> > >> And here is a slightly complex test with IFE. > >> Wanted to show both reclassify and continue in play > >> as well as something that change skb->protocol. > >> > > > > I don't know why you need a complex one, Martin's > > test case is pretty simple (as I already sent to you). > > > > Martin example had a "reclassify" with the vlan pop. > Your example was showing a "continue" > Note: "Reclassify" is typical how we have explained > to users to deal with skb->protocol changes. You have to re-read Martin's example, he clearly showed "reclassify" works but not "continue". This is exactly why I have "continue" in my script, as I need to test what I fix. > Lets have Lucas give this a run and we can move > forward. Sure. Thanks.
On Tue, Jan 15, 2019 at 8:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > On 2019-01-15 10:59 a.m., Jamal Hadi Salim wrote: > > On 2019-01-13 4:08 p.m., Cong Wang wrote: > >> On Sat, Jan 12, 2019 at 7:41 AM Jamal Hadi Salim <jhs@mojatatu.com> > > [..] > > I agree with your rationale. > > Lets have Lucas give this a run and we can move > > forward. > > And as an addendum - Martin corrected me, he did have a "continue" > after the vlan pop (not illegal to do). So i think your patch > is fine; lets not mix it up with vlan not correctly matching > (two separate issues). > > Lets have Lucas create the tdc test, then add my Ack. As I said, this case is a bit hard to add to tdc test, because: 1. It requires a vlan device on top. This is not hard to do, but still needs some extra work. 2. It requires a run-time test, like ping. This is different from other test cases which test setup-time config's. This is a bit hard because we have to parse tc filter stats to learn if some filter is hit or not. I am sure Lucas can do it, just it is not as easy as other cases. Thanks.
On 2019-01-11 9:55 p.m., Cong Wang wrote: > Martin reported a set of filters don't work after changing > from reclassify to continue. Looking into the code, it > looks like skb protocol is not always fetched for each > iteration of the filters. But, as demonstrated by Martin, > TC actions could modify skb->protocol, for example act_vlan, > this means we have to refetch skb protocol in each iteration, > rather than using the one we fetch in the beginning of the loop. > > This bug is _not_ introduced by commit 3b3ae880266d > ("net: sched: consolidate tc_classify{,_compat}"), technically, > if act_vlan is the only action that modifies skb protocol, then > it is commit c7e2b9689ef8 ("sched: introduce vlan action") which > introduced this bug. > > Reported-by: Martin Olsson <martin.olsson+netdev@sentorsecurity.com> > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Jiri Pirko <jiri@resnulli.us> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
On 2019-01-15 12:57 p.m., Cong Wang wrote: > On Tue, Jan 15, 2019 at 8:21 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> >> >> Lets have Lucas create the tdc test, then add my Ack. > > As I said, this case is a bit hard to add to tdc test, because: > > 1. It requires a vlan device on top. This is not hard to do, but still > needs some extra work. > > 2. It requires a run-time test, like ping. This is different from other > test cases which test setup-time config's. This is a bit hard because > we have to parse tc filter stats to learn if some filter is hit or not. > > I am sure Lucas can do it, just it is not as easy as other cases. Lucas has confirmed to me he manually tested it (I just Acked). Yes, it is tricky to add this test case but he will spend time to build the infrastructure and submit this test as first user. We discussed and the stats scrapping is doable with python and the -j tc option. cheers, jamal
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 8ce2a0507970..e2b5cb2eb34e 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -1277,7 +1277,6 @@ EXPORT_SYMBOL(tcf_block_cb_unregister); int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, struct tcf_result *res, bool compat_mode) { - __be16 protocol = tc_skb_protocol(skb); #ifdef CONFIG_NET_CLS_ACT const int max_reclassify_loop = 4; const struct tcf_proto *orig_tp = tp; @@ -1287,6 +1286,7 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, reclassify: #endif for (; tp; tp = rcu_dereference_bh(tp->next)) { + __be16 protocol = tc_skb_protocol(skb); int err; if (tp->protocol != protocol && @@ -1319,7 +1319,6 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp, } tp = first_tp; - protocol = tc_skb_protocol(skb); goto reclassify; #endif }
Martin reported a set of filters don't work after changing from reclassify to continue. Looking into the code, it looks like skb protocol is not always fetched for each iteration of the filters. But, as demonstrated by Martin, TC actions could modify skb->protocol, for example act_vlan, this means we have to refetch skb protocol in each iteration, rather than using the one we fetch in the beginning of the loop. This bug is _not_ introduced by commit 3b3ae880266d ("net: sched: consolidate tc_classify{,_compat}"), technically, if act_vlan is the only action that modifies skb protocol, then it is commit c7e2b9689ef8 ("sched: introduce vlan action") which introduced this bug. Reported-by: Martin Olsson <martin.olsson+netdev@sentorsecurity.com> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/sched/cls_api.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)