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: <20200212022302.GL2159@dhcp-12-139.nay.redhat.com>
Date:   Wed, 12 Feb 2020 10:23:02 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Petr Machata <pmachata@...il.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [PATCH net] selftests: forwarding: use proto icmp for
 {gretap,ip6gretap}_mac testing

On Tue, Feb 11, 2020 at 06:50:38PM +0100, Petr Machata wrote:
> 
> 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.

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