netdev - Re: [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87eev1rp8g.fsf@mellanox.com>
Date:   Tue, 11 Feb 2020 18:50:38 +0100
From:   Petr Machata <pmachata@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] selftests: forwarding: use proto icmp for {gretap,ip6gretap}_mac testing


Hangbin Liu <liuhangbin@...il.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@...il.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@...il.com>
Tested-by: Petr Machata <pmachata@...il.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()