Re: [External] [PATCH bpf-next v2 2/3] selftests/bpf: add ipv4 vxlan tunnel source testcase

[Yonghong Song <yhs@fb.com>]

On 3/22/22 8:42 AM, fankaixi.li@bytedance.com wrote:
> From: "kaixi.fan" <fankaixi.li@bytedance.com>
> 
> Vxlan tunnel is chosen to test bpf code could configure tunnel
> source ipv4 address.

The added test configures tunnel source ipv4 address.

 >It's sufficient to prove that other types
> tunnels could also do it.

Could you be more specific what other types will also use source ipv4 
address. It is too vague to claim "it's sufficient to prove ...".


> In the vxlan tunnel testcase, two underlay ipv4 addresses
> are configured on veth device in root namespace. Test bpf kernel
> code would configure the secondary ipv4 address as the tunnel
> source ip.
> 
> Signed-off-by: kaixi.fan <fankaixi.li@bytedance.com>

Again, please use proper name in Signed-off-by tag.

> ---
>   .../selftests/bpf/progs/test_tunnel_kern.c    | 64 +++++++++++++++++++
>   tools/testing/selftests/bpf/test_tunnel.sh    | 37 ++++++++++-
>   2 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> index ef0dde83b85a..ab635c55ae9b 100644
> --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> @@ -676,4 +676,68 @@ int _xfrm_get_state(struct __sk_buff *skb)
>   	return TC_ACT_OK;
>   }
>   
> +SEC("vxlan_set_tunnel_src")
> +int _vxlan_set_tunnel_src(struct __sk_buff *skb)
> +{
> +	int ret;
> +	struct bpf_tunnel_key key;
> +	struct vxlan_metadata md;
> +
> +	__builtin_memset(&key, 0x0, sizeof(key));
> +	key.local_ipv4 = 0xac100114; /* 172.16.1.20 */
> +	key.remote_ipv4 = 0xac100164; /* 172.16.1.100 */
> +	key.tunnel_id = 2;
> +	key.tunnel_tos = 0;
> +	key.tunnel_ttl = 64;
> +
> +	ret = bpf_skb_set_tunnel_key(skb, &key, sizeof(key),
> +				     BPF_F_ZERO_CSUM_TX);
> +	if (ret < 0) {
> +		ERROR(ret);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	md.gbp = 0x800FF; /* Set VXLAN Group Policy extension */
> +	ret = bpf_skb_set_tunnel_opt(skb, &md, sizeof(md));
> +	if (ret < 0) {
> +		ERROR(ret);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	return TC_ACT_OK;
> +}
> +
> +SEC("vxlan_get_tunnel_src")
> +int _vxlan_get_tunnel_src(struct __sk_buff *skb)
> +{
> +	int ret;
> +	struct bpf_tunnel_key key;
> +	struct vxlan_metadata md;
> +	char fmt[] = "key %d remote ip 0x%x source ip 0x%x\n";
> +	char fmt2[] = "vxlan gbp 0x%x\n";
> +
> +	ret = bpf_skb_get_tunnel_key(skb, &key, sizeof(key), 0);
> +	if (ret < 0) {
> +		ERROR(ret);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	ret = bpf_skb_get_tunnel_opt(skb, &md, sizeof(md));
> +	if (ret < 0) {
> +		ERROR(ret);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	bpf_trace_printk(fmt, sizeof(fmt),
> +			 key.tunnel_id, key.remote_ipv4, key.local_ipv4);
> +	bpf_trace_printk(fmt2, sizeof(fmt2),

In bpf_helpers.h, bpf_printk can be used instead of bpf_trace_printk.

> +			 md.gbp);
> +
> +	if (key.local_ipv4 != 0xac100114) {

I would suggest to make 0xac100114 a macro, so both set_* and get_* 
programs can use the same macro, which makes it easier to understand
and check.

> +		ERROR(ret);
> +		return TC_ACT_SHOT;
> +	}
> +	return TC_ACT_OK;
> +}
> +
[...]