netdev - Re: [PATCHv2 net 1/2] selftest/bpf: add missed ip6ip6 test back

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: <20201107021544.tajvaxcxnc3pmppe@kafai-mbp.dhcp.thefacebook.com>
Date:   Fri, 6 Nov 2020 18:15:44 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Hangbin Liu <liuhangbin@...il.com>
CC:     <netdev@...r.kernel.org>, William Tu <u9012063@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>, <bpf@...r.kernel.org>
Subject: Re: [PATCHv2 net 1/2] selftest/bpf: add missed ip6ip6 test back

On Fri, Nov 06, 2020 at 05:01:16PM +0800, Hangbin Liu wrote:
> In comment 173ca26e9b51 ("samples/bpf: add comprehensive ipip, ipip6,
> ip6ip6 test") we added ip6ip6 test for bpf tunnel testing. But in commit
> 933a741e3b82 ("selftests/bpf: bpf tunnel test.") when we moved it to
> the current folder, we didn't add it.
> 
> This patch add the ip6ip6 test back to bpf tunnel test. Update the ipip6's
> topology for both IPv4 and IPv6 testing. Since iperf test is removed as
> currect framework simplified it in purpose, I also removed unused tcp
> checkings in test_tunnel_kern.c.
> 
> Fixes: 933a741e3b82 ("selftests/bpf: bpf tunnel test.")
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
> 
> v2:
> update add_ipip6tnl_tunnel() to add_ip6tnl_tunnel()
> keep the _ip6ip6_set_tunnel() section.
> ---
>  .../selftests/bpf/progs/test_tunnel_kern.c    | 44 +++----------------
>  tools/testing/selftests/bpf/test_tunnel.sh    | 43 ++++++++++++++++--
>  2 files changed, 44 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> index f48dbfe24ddc..7fd95befef56 100644
> --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> @@ -15,7 +15,6 @@
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
>  #include <linux/types.h>
> -#include <linux/tcp.h>
>  #include <linux/socket.h>
>  #include <linux/pkt_cls.h>
>  #include <linux/erspan.h>
> @@ -528,30 +527,17 @@ int _ipip_set_tunnel(struct __sk_buff *skb)
>  	struct bpf_tunnel_key key = {};
>  	void *data = (void *)(long)skb->data;
>  	struct iphdr *iph = data;
> -	struct tcphdr *tcp = data + sizeof(*iph);
>  	void *data_end = (void *)(long)skb->data_end;
>  	int ret;
>  
>  	/* single length check */
> -	if (data + sizeof(*iph) + sizeof(*tcp) > data_end) {
> +	if (data + sizeof(*iph) > data_end) {
>  		ERROR(1);
>  		return TC_ACT_SHOT;
>  	}
>  
> +	key.remote_ipv4 = 0xac100164; /* 172.16.1.100 */
>  	key.tunnel_ttl = 64;
> -	if (iph->protocol == IPPROTO_ICMP) {
> -		key.remote_ipv4 = 0xac100164; /* 172.16.1.100 */
I think it is still good to check IPPROTO_ICMP
even ping is the only test.

> -	} else {
> -		if (iph->protocol != IPPROTO_TCP || iph->ihl != 5)
> -			return TC_ACT_SHOT;
> -
> -		if (tcp->dest == bpf_htons(5200))
> -			key.remote_ipv4 = 0xac100164; /* 172.16.1.100 */
> -		else if (tcp->dest == bpf_htons(5201))
> -			key.remote_ipv4 = 0xac100165; /* 172.16.1.101 */
> -		else
> -			return TC_ACT_SHOT;
> -	}
>  
>  	ret = bpf_skb_set_tunnel_key(skb, &key, sizeof(key), 0);
>  	if (ret < 0) {
> @@ -585,12 +571,11 @@ int _ipip6_set_tunnel(struct __sk_buff *skb)
>  	struct bpf_tunnel_key key = {};
>  	void *data = (void *)(long)skb->data;
>  	struct iphdr *iph = data;
> -	struct tcphdr *tcp = data + sizeof(*iph);
>  	void *data_end = (void *)(long)skb->data_end;
>  	int ret;
>  
>  	/* single length check */
> -	if (data + sizeof(*iph) + sizeof(*tcp) > data_end) {
> +	if (data + sizeof(*iph) > data_end) {
>  		ERROR(1);
>  		return TC_ACT_SHOT;
>  	}
> @@ -634,37 +619,18 @@ int _ip6ip6_set_tunnel(struct __sk_buff *skb)
>  	struct bpf_tunnel_key key = {};
>  	void *data = (void *)(long)skb->data;
>  	struct ipv6hdr *iph = data;
> -	struct tcphdr *tcp = data + sizeof(*iph);
>  	void *data_end = (void *)(long)skb->data_end;
>  	int ret;
>  
>  	/* single length check */
> -	if (data + sizeof(*iph) + sizeof(*tcp) > data_end) {
> +	if (data + sizeof(*iph) > data_end) {
>  		ERROR(1);
>  		return TC_ACT_SHOT;
>  	}
>  
> -	key.remote_ipv6[0] = bpf_htonl(0x2401db00);
> +	key.remote_ipv6[3] = bpf_htonl(0x11); /* ::11 */
>  	key.tunnel_ttl = 64;
>  
> -	if (iph->nexthdr == 58 /* NEXTHDR_ICMP */) {
Same here. Can this check be kept?

Others LGTM.

> -		key.remote_ipv6[3] = bpf_htonl(1);
> -	} else {
> -		if (iph->nexthdr != 6 /* NEXTHDR_TCP */) {
> -			ERROR(iph->nexthdr);
> -			return TC_ACT_SHOT;
> -		}
> -
> -		if (tcp->dest == bpf_htons(5200)) {
> -			key.remote_ipv6[3] = bpf_htonl(1);
> -		} else if (tcp->dest == bpf_htons(5201)) {
> -			key.remote_ipv6[3] = bpf_htonl(2);
> -		} else {
> -			ERROR(tcp->dest);
> -			return TC_ACT_SHOT;
> -		}
> -	}
> -
>  	ret = bpf_skb_set_tunnel_key(skb, &key, sizeof(key),
>  				     BPF_F_TUNINFO_IPV6);
>  	if (ret < 0) {