Re: [PATCH v7 bpf-next 4/4] selftests/bpf: Add geneve with bpf_skb_set_tunnel_opt_dynptr test-case to test_progs

[Yonghong Song <yhs@fb.com>]

On 9/11/22 5:23 AM, Shmulik Ladkani wrote:
> Add geneve test to test_tunnel. The test setup and scheme resembles the
> existing vxlan test.
> 
> The test also exercises tunnel option assignment using
> bpf_skb_set_tunnel_opt_dynptr.
> 
> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> 
> ---
> v6:
> - Fix missing retcodes in progs/test_tunnel_kern.c
>    spotted by John Fastabend <john.fastabend@gmail.com>
> - Simplify bpf_skb_set_tunnel_opt_dynptr's interface, removing the
>    superfluous 'len' parameter
>    suggested by Andrii Nakryiko <andrii.nakryiko@gmail.com>
> ---
>   .../selftests/bpf/prog_tests/test_tunnel.c    | 108 ++++++++++++++
>   .../selftests/bpf/progs/test_tunnel_kern.c    | 138 ++++++++++++++++++
>   2 files changed, 246 insertions(+)
> 
[...]
>   
> diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> index b11f6952b0c8..cb901b76a547 100644
> --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> @@ -24,6 +24,20 @@
>   
>   #define log_err(__ret) bpf_printk("ERROR line:%d ret:%d\n", __LINE__, __ret)
>   
> +#define GENEVE_OPTS_LEN0 12
> +#define GENEVE_OPTS_LEN1 20
> +
> +struct tun_opts_raw {
> +	__u8 data[64];
> +};
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, __u32);
> +	__type(value, struct tun_opts_raw);
> +} geneve_opts SEC(".maps");
> +
>   struct geneve_opt {
>   	__be16	opt_class;
>   	__u8	type;
> @@ -286,6 +300,130 @@ int ip4ip6erspan_get_tunnel(struct __sk_buff *skb)
>   	return TC_ACT_OK;
>   }
>   
> +SEC("tc")
> +int geneve_set_tunnel_dst(struct __sk_buff *skb)
> +{
> +	int ret;
> +	struct bpf_tunnel_key key;
> +	struct tun_opts_raw *opts;
> +	struct bpf_dynptr dptr;
> +	__u32 index = 0;
> +	__u32 *local_ip = NULL;
> +
> +	local_ip = bpf_map_lookup_elem(&local_ip_map, &index);
> +	if (!local_ip) {
> +		log_err(-1);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	index = 0;
> +	opts = bpf_map_lookup_elem(&geneve_opts, &index);
> +	if (!opts) {
> +		log_err(-1);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	__builtin_memset(&key, 0x0, sizeof(key));
> +	key.local_ipv4 = 0xac100164; /* 172.16.1.100 */
> +	key.remote_ipv4 = *local_ip;
> +	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) {
> +		log_err(ret);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	/* set empty geneve options (of runtime length) using a dynptr */
> +	__builtin_memset(opts, 0x0, sizeof(*opts));
> +	if (*local_ip % 2)
> +		bpf_dynptr_from_mem(opts, GENEVE_OPTS_LEN1, 0, &dptr);
> +	else
> +		bpf_dynptr_from_mem(opts, GENEVE_OPTS_LEN0, 0, &dptr);
> +	ret = bpf_skb_set_tunnel_opt_dynptr(skb, &dptr);

I think the above example is not good. since it can write as
	if (*local_ip % 2)
		ret = bpf_skb_set_tunnel_opt(skb, opts, GENEVE_OPTS_LEN1);
	else
		ret = bpf_skb_set_tunnel_opt(skb, opts,	GENEVE_OPTS_LEN0);

In the commit message of Patch 2, we have

===
For example, we have an ebpf program that gets geneve options on
incoming packets, stores them into a map (using a key representing
the incoming flow), and later needs to assign *same* options to
reply packets (belonging to same flow).
===

It would be great if you can create a test case for the above
use case.


> +	if (ret < 0) {
> +		log_err(ret);
> +		return TC_ACT_SHOT;
> +	}
> +
> +	return TC_ACT_OK;
> +}
> +
[...]