On Thu, 2023-03-16 at 18:50 +0100, Alexander Lobakin wrote:
> Alexei noticed xdp_do_redirect test on BPF CI started failing on
> BE systems after skb PP recycling was enabled:
>
> test_xdp_do_redirect:PASS:prog_run 0 nsec
> test_xdp_do_redirect:PASS:pkt_count_xdp 0 nsec
> test_xdp_do_redirect:PASS:pkt_count_zero 0 nsec
> test_xdp_do_redirect:FAIL:pkt_count_tc unexpected pkt_count_tc:
> actual
> 220 != expected 9998
> test_max_pkt_size:PASS:prog_run_max_size 0 nsec
> test_max_pkt_size:PASS:prog_run_too_big 0 nsec
> close_netns:PASS:setns 0 nsec
> #289 xdp_do_redirect:FAIL
> Summary: 270/1674 PASSED, 30 SKIPPED, 1 FAILED
>
> and it doesn't happen on LE systems.
> Ilya then hunted it down to:
>
> #0 0x0000000000aaeee6 in neigh_hh_output (hh=0x83258df0,
> skb=0x88142200) at linux/include/net/neighbour.h:503
> #1 0x0000000000ab2cda in neigh_output (skip_cache=false,
> skb=0x88142200, n=<optimized out>) at
> linux/include/net/neighbour.h:544
> #2 ip6_finish_output2 (net=net@entry=0x88edba00, sk=sk@entry=0x0,
> skb=skb@entry=0x88142200) at linux/net/ipv6/ip6_output.c:134
> #3 0x0000000000ab4cbc in __ip6_finish_output (skb=0x88142200,
> sk=0x0,
> net=0x88edba00) at linux/net/ipv6/ip6_output.c:195
> #4 ip6_finish_output (net=0x88edba00, sk=0x0, skb=0x88142200) at
> linux/net/ipv6/ip6_output.c:206
>
> xdp_do_redirect test places a u32 marker (0x42) right before the
> Ethernet
> header to check it then in the XDP program and return %XDP_ABORTED if
> it's
> not there. Neigh xmit code likes to round up hard header length to
> speed
> up copying the header, so it overwrites two bytes in front of the Eth
> header. On LE systems, 0x42 is one byte at `data - 4`, while on BE
> it's
> `data - 1`, what explains why it happens only there.
> It didn't happen previously due to that %XDP_PASS meant the page will
> be
> discarded and replaced by a new one, but now it can be recycled as
> well,
> while bpf_test_run code doesn't reinitialize the content of recycled
> pages. This mark is limited to this particular test and its setup
> though,
> so there's no need to predict 1000 different possible cases. Just
> move
> it 4 bytes to the left, still keeping it 32 bit to match on more
> bytes.
>
> Fixes: 9c94bbf9a87b ("xdp: recycle Page Pool backed skbs built from
> XDP frames")
> Reported-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> Link:
> https://lore.kernel.org/bpf/CAADnVQ+B_JOU+EpP=DKhbY9yXdN6GiRPnpTTXfEZ9sNkUeb-yQ@xxxxxxxxxxxxxx
> Reported-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx> # + debugging
> Link:
> https://lore.kernel.org/bpf/8341c1d9f935f410438e79d3bd8a9cc50aefe105.camel@xxxxxxxxxxxxx
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c | 7 ++++---
> tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c | 2 +-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> index 856cbc29e6a1..4eaa3dcaebc8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> @@ -86,12 +86,12 @@ static void test_max_pkt_size(int fd)
> void test_xdp_do_redirect(void)
> {
> int err, xdp_prog_fd, tc_prog_fd, ifindex_src, ifindex_dst;
> - char data[sizeof(pkt_udp) + sizeof(__u32)];
> + char data[sizeof(pkt_udp) + sizeof(__u64)];
> struct test_xdp_do_redirect *skel = NULL;
> struct nstoken *nstoken = NULL;
> struct bpf_link *link;
> LIBBPF_OPTS(bpf_xdp_query_opts, query_opts);
> - struct xdp_md ctx_in = { .data = sizeof(__u32),
> + struct xdp_md ctx_in = { .data = sizeof(__u64),
> .data_end = sizeof(data) };
> DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
> .data_in = &data,
> @@ -105,8 +105,9 @@ void test_xdp_do_redirect(void)
> DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> .attach_point = BPF_TC_INGRESS);
>
> - memcpy(&data[sizeof(__u32)], &pkt_udp, sizeof(pkt_udp));
> + memcpy(&data[sizeof(__u64)], &pkt_udp, sizeof(pkt_udp));
> *((__u32 *)data) = 0x42; /* metadata test value */
> + *((__u32 *)data + 4) = 0;
>
> skel = test_xdp_do_redirect__open();
> if (!ASSERT_OK_PTR(skel, "skel"))
> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> index cd2d4e3258b8..5baaafed0d2d 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> @@ -52,7 +52,7 @@ int xdp_redirect(struct xdp_md *xdp)
>
> *payload = MARK_IN;
>
> - if (bpf_xdp_adjust_meta(xdp, 4))
> + if (bpf_xdp_adjust_meta(xdp, sizeof(__u64)))
> return XDP_ABORTED;
>
> if (retcode > XDP_PASS)
Tested-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
