Re: [PATCH bpf-next v3 4/6] selftests/bpf: refactor xdp_context_functional test and bpf program

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Marcus Wichelmann wrote:
> The existing XDP metadata test works by creating a veth pair and
> attaching XDP & TC programs that drop the packet when the condition of
> the test isn't fulfilled. The test then pings through the veth pair and
> succeeds when the ping comes through.
> 
> While this test works great for a veth pair, it is hard to replicate for
> tap devices to test the XDP metadata support of them. A similar test for
> the tun driver would either involve logic to reply to the ping request,
> or would have to capture the packet to check if it was dropped or not.
> 
> To make the testing of other drivers easier while still maximizing code
> reuse, this commit refactors the existing xdp_context_functional test to
> use a test_result map. Instead of conditionally passing or dropping the
> packet, the TC program is changed to copy the received metadata into the
> value of that single-entry array map. Tests can then verify that the map
> value matches the expectation.
> 
> This testing logic is easy to adapt to other network drivers as the only
> remaining requirement is that there is some way to send a custom
> Ethernet packet through it that triggers the XDP & TC programs.
> 
> The payload of the Ethernet packet is used as the test data that is
> expected to be passed as metadata from the XDP to the TC program and
> written to the map. It has a fixed size of 32 bytes which is a
> reasonalbe size that should be supported by both drivers. Additional
> packet headers are not necessary for the test and were therefore skipped
> to keep the testing code short.
> 
> Signed-off-by: Marcus Wichelmann <marcus.wichelmann@hetzner-cloud.de>

> -	SYS(close, "ip addr add " RX_ADDR "/24 dev " RX_NAME);

Why remove the Rx and Tx addresses?

> +	/* By default, Linux sends IPv6 multicast listener reports which
> +	 * interfere with this test. Set the IFF_NOARP flag to ensure
> +	 * silence on the interface.
> +	 */
> +	SYS(close, "ip link set dev " RX_NAME " arp off");
>  	SYS(close, "ip link set dev " RX_NAME " up");
>  
>  	skel = test_xdp_meta__open_and_load();
> @@ -154,21 +215,12 @@ void test_xdp_context_functional(void)
>  	if (!ASSERT_OK(ret, "bpf_tc_hook_create"))
>  		goto close;
>  
> -	tc_prog = bpf_object__find_program_by_name(skel->obj, "ing_cls");
> -	if (!ASSERT_OK_PTR(tc_prog, "open ing_cls prog"))
> -		goto close;
> -
> -	tc_opts.prog_fd = bpf_program__fd(tc_prog);
> +	tc_opts.prog_fd = bpf_program__fd(skel->progs.ing_cls);

This refactor seems irrelevant to the goal of this patch? Same below.
>  	ret = bpf_tc_attach(&tc_hook, &tc_opts);
>  	if (!ASSERT_OK(ret, "bpf_tc_attach"))
>  		goto close;
>  
> -	xdp_prog = bpf_object__find_program_by_name(skel->obj, "ing_xdp");
> -	if (!ASSERT_OK_PTR(xdp_prog, "open ing_xdp prog"))
> -		goto close;
> -
> -	ret = bpf_xdp_attach(rx_ifindex,
> -			     bpf_program__fd(xdp_prog),
> +	ret = bpf_xdp_attach(rx_ifindex, bpf_program__fd(skel->progs.ing_xdp),
>  			     0, NULL);
>  	if (!ASSERT_GE(ret, 0, "bpf_xdp_attach"))
>  		goto close;
> @@ -179,9 +231,18 @@ void test_xdp_context_functional(void)
>  	if (!ASSERT_OK_PTR(nstoken, "setns tx_ns"))
>  		goto close;
>  
> -	SYS(close, "ip addr add " TX_ADDR "/24 dev " TX_NAME);
> +	SYS(close, "ip link set dev " TX_NAME " arp off");
>  	SYS(close, "ip link set dev " TX_NAME " up");

>  SEC("tc")
>  int ing_cls(struct __sk_buff *ctx)
>  {
> -	__u8 *data, *data_meta, *data_end;
> -	__u32 diff = 0;
> +	void *data_meta = (void *)(unsigned long)ctx->data_meta;
> +	void *data = (void *)(unsigned long)ctx->data;
>  
> -	data_meta = ctx_ptr(ctx, data_meta);
> -	data_end  = ctx_ptr(ctx, data_end);
> -	data      = ctx_ptr(ctx, data);

Similarly: why these changes?

In general, please minimize your patch to the core purpose. Avoid
including "cleanups" or other such refactors. Or justify the choice
explicitly in the commit message.

> -
> -	if (data + ETH_ALEN > data_end ||
> -	    data_meta + round_up(ETH_ALEN, 4) > data)
> +	if (data_meta + META_SIZE > data)
>  		return TC_ACT_SHOT;
>  
> -	diff |= ((__u32 *)data_meta)[0] ^ ((__u32 *)data)[0];
> -	diff |= ((__u16 *)data_meta)[2] ^ ((__u16 *)data)[2];
> +	int key = 0;
> +
> +	bpf_map_update_elem(&test_result, &key, data_meta, BPF_ANY);
>  
> -	return diff ? TC_ACT_SHOT : TC_ACT_OK;
> +	return TC_ACT_SHOT;
>  }
>  
>  SEC("xdp")
>  int ing_xdp(struct xdp_md *ctx)
>  {
> -	__u8 *data, *data_meta, *data_end;
> -	int ret;
> -
> -	ret = bpf_xdp_adjust_meta(ctx, -round_up(ETH_ALEN, 4));
> +	int ret = bpf_xdp_adjust_meta(ctx, -META_SIZE);
>  	if (ret < 0)
>  		return XDP_DROP;
>  
> -	data_meta = ctx_ptr(ctx, data_meta);
> -	data_end  = ctx_ptr(ctx, data_end);
> -	data      = ctx_ptr(ctx, data);
> +	void *data_meta = (void *)(unsigned long)ctx->data_meta;
> +	void *data = (void *)(unsigned long)ctx->data;
> +	void *data_end = (void *)(unsigned long)ctx->data_end;
>  
> -	if (data + ETH_ALEN > data_end ||
> -	    data_meta + round_up(ETH_ALEN, 4) > data)
> +	void *payload = data + sizeof(struct ethhdr);
> +
> +	if (data_meta + META_SIZE > data || payload + META_SIZE > data_end)
>  		return XDP_DROP;
>  
> -	__builtin_memcpy(data_meta, data, ETH_ALEN);
> +	__builtin_memcpy(data_meta, payload, META_SIZE);
> +
>  	return XDP_PASS;
>  }
>  
> -- 
> 2.43.0
>