[net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing
diff mbox series

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
Related show

Commit Message

Hangbin Liu Feb. 11, 2020, 7:32 a.m. UTC
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(-)

Comments

Petr Machata Feb. 11, 2020, 5:50 p.m. UTC | #1
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()
Hangbin Liu Feb. 12, 2020, 2:23 a.m. UTC | #2
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()
David Miller Feb. 17, 2020, 2:32 a.m. UTC | #3
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.

Patch
diff mbox series

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()