* [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing
@ 2020-02-11 7:32 Hangbin Liu
2020-02-11 17:50 ` Petr Machata
2020-02-17 2:32 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Hangbin Liu @ 2020-02-11 7:32 UTC (permalink / raw)
To: netdev; +Cc: Petr Machata, Jiri Pirko, David S . Miller, Hangbin Liu
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(-)
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()
--
2.19.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing
2020-02-11 7:32 [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing Hangbin Liu
@ 2020-02-11 17:50 ` Petr Machata
2020-02-12 2:23 ` Hangbin Liu
2020-02-17 2:32 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: Petr Machata @ 2020-02-11 17:50 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, Jiri Pirko, David S . Miller
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()
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing
2020-02-11 17:50 ` Petr Machata
@ 2020-02-12 2:23 ` Hangbin Liu
0 siblings, 0 replies; 4+ messages in thread
From: Hangbin Liu @ 2020-02-12 2:23 UTC (permalink / raw)
To: Petr Machata; +Cc: netdev, Jiri Pirko, David S . Miller, Tom Herbert
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()
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing
2020-02-11 7:32 [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing Hangbin Liu
2020-02-11 17:50 ` Petr Machata
@ 2020-02-17 2:32 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2020-02-17 2:32 UTC (permalink / raw)
To: liuhangbin; +Cc: netdev, petrm, jiri
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-17 2:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-11 7:32 [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing Hangbin Liu
2020-02-11 17:50 ` Petr Machata
2020-02-12 2:23 ` Hangbin Liu
2020-02-17 2:32 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).