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@xxxxxxxxxxxxxxxx>
> - 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
>
