| Message ID | 20200211073256.32652-1-liuhangbin@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing | expand |
Hangbin Liu <liuhangbin@gmail.com> writes: > For tc ip_proto filter, when we extract the flow via __skb_flow_dissect() > without flag FLOW_DISSECTOR_F_STOP_AT_ENCAP, we will continue extract to > the inner proto. > > So for GRE + ICMP messages, we should not track GRE proto, but inner ICMP > proto. > > For test mirror_gre.sh, it may make user confused if we capture ICMP > message on $h3(since the flow is GRE message). So I move the capture > dev to h3-gt{4,6}, and only capture ICMP message. [...] > Fixes: ba8d39871a10 ("selftests: forwarding: Add test for mirror to gretap") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> This looks correct. The reason we never saw this internally is that the ASIC puts the outer protocol to ACL ip_proto. Thus the flower rule 77 actually only matched in HW, not in both HW and SW like it should, given the missing skip_sw. Reviewed-by: Petr Machata <pmachata@gmail.com> Tested-by: Petr Machata <pmachata@gmail.com> Thanks! > --- > .../selftests/net/forwarding/mirror_gre.sh | 25 ++++++++++--------- > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/net/forwarding/mirror_gre.sh b/tools/testing/selftests/net/forwarding/mirror_gre.sh > index e6fd7a18c655..0266443601bc 100755 > --- a/tools/testing/selftests/net/forwarding/mirror_gre.sh > +++ b/tools/testing/selftests/net/forwarding/mirror_gre.sh > @@ -63,22 +63,23 @@ test_span_gre_mac() > { > local tundev=$1; shift > local direction=$1; shift > - local prot=$1; shift > local what=$1; shift > > - local swp3mac=$(mac_get $swp3) > - local h3mac=$(mac_get $h3) > + case "$direction" in > + ingress) local src_mac=$(mac_get $h1); local dst_mac=$(mac_get $h2) > + ;; > + egress) local src_mac=$(mac_get $h2); local dst_mac=$(mac_get $h1) > + ;; > + esac > > RET=0 > > mirror_install $swp1 $direction $tundev "matchall $tcflags" > - tc filter add dev $h3 ingress pref 77 prot $prot \ > - flower ip_proto 0x2f src_mac $swp3mac dst_mac $h3mac \ > - action pass > + icmp_capture_install h3-${tundev} "src_mac $src_mac dst_mac $dst_mac" > > - mirror_test v$h1 192.0.2.1 192.0.2.2 $h3 77 10 > + mirror_test v$h1 192.0.2.1 192.0.2.2 h3-${tundev} 100 10 > > - tc filter del dev $h3 ingress pref 77 > + icmp_capture_uninstall h3-${tundev} > mirror_uninstall $swp1 $direction > > log_test "$direction $what: envelope MAC ($tcflags)" > @@ -120,14 +121,14 @@ test_ip6gretap() > > test_gretap_mac() > { > - test_span_gre_mac gt4 ingress ip "mirror to gretap" > - test_span_gre_mac gt4 egress ip "mirror to gretap" > + test_span_gre_mac gt4 ingress "mirror to gretap" > + test_span_gre_mac gt4 egress "mirror to gretap" > } > > test_ip6gretap_mac() > { > - test_span_gre_mac gt6 ingress ipv6 "mirror to ip6gretap" > - test_span_gre_mac gt6 egress ipv6 "mirror to ip6gretap" > + test_span_gre_mac gt6 ingress "mirror to ip6gretap" > + test_span_gre_mac gt6 egress "mirror to ip6gretap" > } > > test_all()
On Tue, Feb 11, 2020 at 06:50:38PM +0100, Petr Machata wrote: > > Hangbin Liu <liuhangbin@gmail.com> writes: > > > For tc ip_proto filter, when we extract the flow via __skb_flow_dissect() > > without flag FLOW_DISSECTOR_F_STOP_AT_ENCAP, we will continue extract to > > the inner proto. > > > > So for GRE + ICMP messages, we should not track GRE proto, but inner ICMP > > proto. > > > > For test mirror_gre.sh, it may make user confused if we capture ICMP > > message on $h3(since the flow is GRE message). So I move the capture > > dev to h3-gt{4,6}, and only capture ICMP message. > > [...] > > > Fixes: ba8d39871a10 ("selftests: forwarding: Add test for mirror to gretap") > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > This looks correct. The reason we never saw this internally is that the > ASIC puts the outer protocol to ACL ip_proto. Thus the flower rule 77 > actually only matched in HW, not in both HW and SW like it should, given > the missing skip_sw. Hi Petr, Thanks for the review and testing. I also got the same issue with test_ttl in mirror_gre_changes.sh, because the actual ttl it captured is inner ip header's ttl, not gretap's ttl. But that test is hard to fix as it is for gretap ttl testing, we need mirror the ttl on lower level interface(i.e. interface $h3). What I thought is to fix it on kernel side. First I thought is to add a new flag FLOW_DIS_STOP_AT_ENCAP for struct flow_dissector_key_control. But that looks not useful as we do skb_flow_dissect() first, and do fl_lookup() later. Now I'm thinking about setting flag STOP_AT_ENCAP directly for tc flower, like static int fl_classify() { [...] skb_flow_dissect(skb, &mask->dissector, &skb_key, FLOW_DIS_STOP_AT_ENCAP); [...] } Because when we check the ip proto on the interface, most time we only care about the outer ip proto. If we care about the inner ip proto, why don't we set the tc rule on inner interface? Hi Tom, what do you think? Thanks Hangbin > > Reviewed-by: Petr Machata <pmachata@gmail.com> > Tested-by: Petr Machata <pmachata@gmail.com> > > Thanks! > > > --- > > .../selftests/net/forwarding/mirror_gre.sh | 25 ++++++++++--------- > > 1 file changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/tools/testing/selftests/net/forwarding/mirror_gre.sh b/tools/testing/selftests/net/forwarding/mirror_gre.sh > > index e6fd7a18c655..0266443601bc 100755 > > --- a/tools/testing/selftests/net/forwarding/mirror_gre.sh > > +++ b/tools/testing/selftests/net/forwarding/mirror_gre.sh > > @@ -63,22 +63,23 @@ test_span_gre_mac() > > { > > local tundev=$1; shift > > local direction=$1; shift > > - local prot=$1; shift > > local what=$1; shift > > > > - local swp3mac=$(mac_get $swp3) > > - local h3mac=$(mac_get $h3) > > + case "$direction" in > > + ingress) local src_mac=$(mac_get $h1); local dst_mac=$(mac_get $h2) > > + ;; > > + egress) local src_mac=$(mac_get $h2); local dst_mac=$(mac_get $h1) > > + ;; > > + esac > > > > RET=0 > > > > mirror_install $swp1 $direction $tundev "matchall $tcflags" > > - tc filter add dev $h3 ingress pref 77 prot $prot \ > > - flower ip_proto 0x2f src_mac $swp3mac dst_mac $h3mac \ > > - action pass > > + icmp_capture_install h3-${tundev} "src_mac $src_mac dst_mac $dst_mac" > > > > - mirror_test v$h1 192.0.2.1 192.0.2.2 $h3 77 10 > > + mirror_test v$h1 192.0.2.1 192.0.2.2 h3-${tundev} 100 10 > > > > - tc filter del dev $h3 ingress pref 77 > > + icmp_capture_uninstall h3-${tundev} > > mirror_uninstall $swp1 $direction > > > > log_test "$direction $what: envelope MAC ($tcflags)" > > @@ -120,14 +121,14 @@ test_ip6gretap() > > > > test_gretap_mac() > > { > > - test_span_gre_mac gt4 ingress ip "mirror to gretap" > > - test_span_gre_mac gt4 egress ip "mirror to gretap" > > + test_span_gre_mac gt4 ingress "mirror to gretap" > > + test_span_gre_mac gt4 egress "mirror to gretap" > > } > > > > test_ip6gretap_mac() > > { > > - test_span_gre_mac gt6 ingress ipv6 "mirror to ip6gretap" > > - test_span_gre_mac gt6 egress ipv6 "mirror to ip6gretap" > > + test_span_gre_mac gt6 ingress "mirror to ip6gretap" > > + test_span_gre_mac gt6 egress "mirror to ip6gretap" > > } > > > > test_all()
From: Hangbin Liu <liuhangbin@gmail.com> Date: Tue, 11 Feb 2020 15:32:56 +0800 > For tc ip_proto filter, when we extract the flow via __skb_flow_dissect() > without flag FLOW_DISSECTOR_F_STOP_AT_ENCAP, we will continue extract to > the inner proto. > > So for GRE + ICMP messages, we should not track GRE proto, but inner ICMP > proto. > > For test mirror_gre.sh, it may make user confused if we capture ICMP > message on $h3(since the flow is GRE message). So I move the capture > dev to h3-gt{4,6}, and only capture ICMP message. ... > Fixes: ba8d39871a10 ("selftests: forwarding: Add test for mirror to gretap") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> Applied, thank you.
diff --git a/tools/testing/selftests/net/forwarding/mirror_gre.sh b/tools/testing/selftests/net/forwarding/mirror_gre.sh index e6fd7a18c655..0266443601bc 100755 --- a/tools/testing/selftests/net/forwarding/mirror_gre.sh +++ b/tools/testing/selftests/net/forwarding/mirror_gre.sh @@ -63,22 +63,23 @@ test_span_gre_mac() { local tundev=$1; shift local direction=$1; shift - local prot=$1; shift local what=$1; shift - local swp3mac=$(mac_get $swp3) - local h3mac=$(mac_get $h3) + case "$direction" in + ingress) local src_mac=$(mac_get $h1); local dst_mac=$(mac_get $h2) + ;; + egress) local src_mac=$(mac_get $h2); local dst_mac=$(mac_get $h1) + ;; + esac RET=0 mirror_install $swp1 $direction $tundev "matchall $tcflags" - tc filter add dev $h3 ingress pref 77 prot $prot \ - flower ip_proto 0x2f src_mac $swp3mac dst_mac $h3mac \ - action pass + icmp_capture_install h3-${tundev} "src_mac $src_mac dst_mac $dst_mac" - mirror_test v$h1 192.0.2.1 192.0.2.2 $h3 77 10 + mirror_test v$h1 192.0.2.1 192.0.2.2 h3-${tundev} 100 10 - tc filter del dev $h3 ingress pref 77 + icmp_capture_uninstall h3-${tundev} mirror_uninstall $swp1 $direction log_test "$direction $what: envelope MAC ($tcflags)" @@ -120,14 +121,14 @@ test_ip6gretap() test_gretap_mac() { - test_span_gre_mac gt4 ingress ip "mirror to gretap" - test_span_gre_mac gt4 egress ip "mirror to gretap" + test_span_gre_mac gt4 ingress "mirror to gretap" + test_span_gre_mac gt4 egress "mirror to gretap" } test_ip6gretap_mac() { - test_span_gre_mac gt6 ingress ipv6 "mirror to ip6gretap" - test_span_gre_mac gt6 egress ipv6 "mirror to ip6gretap" + test_span_gre_mac gt6 ingress "mirror to ip6gretap" + test_span_gre_mac gt6 egress "mirror to ip6gretap" } test_all()
For tc ip_proto filter, when we extract the flow via __skb_flow_dissect() without flag FLOW_DISSECTOR_F_STOP_AT_ENCAP, we will continue extract to the inner proto. So for GRE + ICMP messages, we should not track GRE proto, but inner ICMP proto. For test mirror_gre.sh, it may make user confused if we capture ICMP message on $h3(since the flow is GRE message). So I move the capture dev to h3-gt{4,6}, and only capture ICMP message. Before the fix: ]# ./mirror_gre.sh TEST: ingress mirror to gretap (skip_hw) [ OK ] TEST: egress mirror to gretap (skip_hw) [ OK ] TEST: ingress mirror to ip6gretap (skip_hw) [ OK ] TEST: egress mirror to ip6gretap (skip_hw) [ OK ] TEST: ingress mirror to gretap: envelope MAC (skip_hw) [FAIL] Expected to capture 10 packets, got 0. TEST: egress mirror to gretap: envelope MAC (skip_hw) [FAIL] Expected to capture 10 packets, got 0. TEST: ingress mirror to ip6gretap: envelope MAC (skip_hw) [FAIL] Expected to capture 10 packets, got 0. TEST: egress mirror to ip6gretap: envelope MAC (skip_hw) [FAIL] Expected to capture 10 packets, got 0. TEST: two simultaneously configured mirrors (skip_hw) [ OK ] WARN: Could not test offloaded functionality After fix: ]# ./mirror_gre.sh TEST: ingress mirror to gretap (skip_hw) [ OK ] TEST: egress mirror to gretap (skip_hw) [ OK ] TEST: ingress mirror to ip6gretap (skip_hw) [ OK ] TEST: egress mirror to ip6gretap (skip_hw) [ OK ] TEST: ingress mirror to gretap: envelope MAC (skip_hw) [ OK ] TEST: egress mirror to gretap: envelope MAC (skip_hw) [ OK ] TEST: ingress mirror to ip6gretap: envelope MAC (skip_hw) [ OK ] TEST: egress mirror to ip6gretap: envelope MAC (skip_hw) [ OK ] TEST: two simultaneously configured mirrors (skip_hw) [ OK ] WARN: Could not test offloaded functionality Fixes: ba8d39871a10 ("selftests: forwarding: Add test for mirror to gretap") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- .../selftests/net/forwarding/mirror_gre.sh | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-)