[bpf-next,v3,0/4] xdp: recycle Page Pool backed skbs built from XDP frames

  Yeah, I still remember that "Who needs cpumap nowadays" (c), but anyway.

__xdp_build_skb_from_frame() missed the moment when the networking stack
became able to recycle skb pages backed by a page_pool. This was making
e.g. cpumap redirect even less effective than simple %XDP_PASS. veth was
also affected in some scenarios.
A lot of drivers use skb_mark_for_recycle() already, it's been almost
two years and seems like there are no issues in using it in the generic
code too. {__,}xdp_release_frame() can be then removed as it losts its
last user.
Page Pool becomes then zero-alloc (or almost) in the abovementioned
cases, too. Other memory type models (who needs them at this point)
have no changes.

Some numbers on 1 Xeon Platinum core bombed with 27 Mpps of 64-byte
IPv6 UDP, iavf w/XDP[0] (CONFIG_PAGE_POOL_STATS is enabled):

Plain %XDP_PASS on baseline, Page Pool driver:

src cpu Rx     drops  dst cpu Rx
  2.1 Mpps       N/A    2.1 Mpps

cpumap redirect (cross-core, w/o leaving its NUMA node) on baseline:

  6.8 Mpps  5.0 Mpps    1.8 Mpps

cpumap redirect with skb PP recycling:

  7.9 Mpps  5.7 Mpps    2.2 Mpps
                       +22% (from cpumap redir on baseline)

[0] https://github.com/alobakin/linux/commits/iavf-xdp

Alexander Lobakin (4):
  selftests/bpf: robustify test_xdp_do_redirect with more payload magics
  net: page_pool, skbuff: make skb_mark_for_recycle() always available
  xdp: recycle Page Pool backed skbs built from XDP frames
  xdp: remove unused {__,}xdp_release_frame()

 include/linux/skbuff.h                        |  4 +--
 include/net/xdp.h                             | 29 ---------------
 net/core/xdp.c                                | 19 ++--------
 .../bpf/progs/test_xdp_do_redirect.c          | 36 +++++++++++++------
 4 files changed, 30 insertions(+), 58 deletions(-)

---
From v2[1]:
* fix the test_xdp_do_redirect selftest failing after the series: it was
  relying on that %XDP_PASS frames can't be recycled on veth
  (BPF CI, Alexei);
* explain "w/o leaving its node" in the cover letter (Jesper).

From v1[2]:
* make skb_mark_for_recycle() always available, otherwise there are build
  failures on non-PP systems (kbuild bot);
* 'Page Pool' -> 'page_pool' when it's about a page_pool instance, not
  API (Jesper);
* expanded test system info a bit in the cover letter (Jesper).

[1] https://lore.kernel.org/bpf/20230303133232.2546004-1-aleksander.lobakin@intel.com
[2] https://lore.kernel.org/bpf/20230301160315.1022488-1-aleksander.lobakin@intel.com
  

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Mon, 13 Mar 2023 22:55:49 +0100

> Yeah, I still remember that "Who needs cpumap nowadays" (c), but anyway.
> 
> __xdp_build_skb_from_frame() missed the moment when the networking stack
> became able to recycle skb pages backed by a page_pool. This was making
> e.g. cpumap redirect even less effective than simple %XDP_PASS. veth was
> also affected in some scenarios.
[...]

Regarding failing tests, here's a piece of logs:

  #288     xdp_devmap_attach:OK
  [  156.324473] IPv6: ADDRCONF(NETDEV_CHANGE): veth_src: link becomes ready
  [  156.362859] bond2 (unregistering): Released all slaves
  #289     xdp_do_redirect:OK
  #290     xdp_info:OK

[...]

  #297/1   xfrm_info/xfrm_info:OK
  #297     xfrm_info:OK

  All error logs:
  libbpf: prog 'trace_virtqueue_add_sgs': BPF program load failed: Bad
address
  libbpf: prog 'trace_virtqueue_add_sgs': -- BEGIN PROG LOAD LOG --
  The sequence of 8193 jumps is too complex.
  verification time 77808 usec
  stack depth 64
  processed 156616 insns (limit 1000000) max_states_per_insn 8
total_states 1754 peak_states 1712 mark_read 12
  -- END PROG LOAD LOG --
  libbpf: prog 'trace_virtqueue_add_sgs': failed to load: -14
  libbpf: failed to load object 'loop6.bpf.o'
  scale_test:FAIL:expect_success unexpected error: -14 (errno 14)
  #257     verif_scale_loop6:FAIL
  Summary: 288/1766 PASSED, 21 SKIPPED, 1 FAILED

So, xdp_do_redirect, which was previously failing, now works fine. OTOH,
"verif_scale_loop6" now fails, but from what I understand from the log,
it has nothing with the series ("8193 jumps is too complex" -- I don't
even touch program-related stuff). I don't know what's the reason of it
failing, can it be some CI issues or maybe some recent commits?

Thanks,
Olek
  

On Tue, Mar 14, 2023 at 4:58 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
>   All error logs:
>   libbpf: prog 'trace_virtqueue_add_sgs': BPF program load failed: Bad
> address
>   libbpf: prog 'trace_virtqueue_add_sgs': -- BEGIN PROG LOAD LOG --
>   The sequence of 8193 jumps is too complex.
>   verification time 77808 usec
>   stack depth 64
>   processed 156616 insns (limit 1000000) max_states_per_insn 8
> total_states 1754 peak_states 1712 mark_read 12
>   -- END PROG LOAD LOG --
>   libbpf: prog 'trace_virtqueue_add_sgs': failed to load: -14
>   libbpf: failed to load object 'loop6.bpf.o'
>   scale_test:FAIL:expect_success unexpected error: -14 (errno 14)
>   #257     verif_scale_loop6:FAIL
>   Summary: 288/1766 PASSED, 21 SKIPPED, 1 FAILED
>
> So, xdp_do_redirect, which was previously failing, now works fine. OTOH,
> "verif_scale_loop6" now fails, but from what I understand from the log,
> it has nothing with the series ("8193 jumps is too complex" -- I don't
> even touch program-related stuff). I don't know what's the reason of it
> failing, can it be some CI issues or maybe some recent commits?

Yeah. It's an issue with the latest clang.
We don't have a workaround for this yet.
It's not a blocker for your patchset.
We didn't have time to look at it closely.
  

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 13 Mar 2023 22:55:49 +0100 you wrote:
> Yeah, I still remember that "Who needs cpumap nowadays" (c), but anyway.
> 
> __xdp_build_skb_from_frame() missed the moment when the networking stack
> became able to recycle skb pages backed by a page_pool. This was making
> e.g. cpumap redirect even less effective than simple %XDP_PASS. veth was
> also affected in some scenarios.
> A lot of drivers use skb_mark_for_recycle() already, it's been almost
> two years and seems like there are no issues in using it in the generic
> code too. {__,}xdp_release_frame() can be then removed as it losts its
> last user.
> Page Pool becomes then zero-alloc (or almost) in the abovementioned
> cases, too. Other memory type models (who needs them at this point)
> have no changes.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v3,1/4] selftests/bpf: robustify test_xdp_do_redirect with more payload magics
    https://git.kernel.org/bpf/bpf-next/c/487deb3e3393
  - [bpf-next,v3,2/4] net: page_pool, skbuff: make skb_mark_for_recycle() always available
    https://git.kernel.org/bpf/bpf-next/c/2c854e5fcd7e
  - [bpf-next,v3,3/4] xdp: recycle Page Pool backed skbs built from XDP frames
    https://git.kernel.org/bpf/bpf-next/c/9c94bbf9a87b
  - [bpf-next,v3,4/4] xdp: remove unused {__,}xdp_release_frame()
    https://git.kernel.org/bpf/bpf-next/c/d4e492338d11

You are awesome, thank you!
  

On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Mar 14, 2023 at 4:58 AM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
> >
> >   All error logs:
> >   libbpf: prog 'trace_virtqueue_add_sgs': BPF program load failed: Bad
> > address
> >   libbpf: prog 'trace_virtqueue_add_sgs': -- BEGIN PROG LOAD LOG --
> >   The sequence of 8193 jumps is too complex.
> >   verification time 77808 usec
> >   stack depth 64
> >   processed 156616 insns (limit 1000000) max_states_per_insn 8
> > total_states 1754 peak_states 1712 mark_read 12
> >   -- END PROG LOAD LOG --
> >   libbpf: prog 'trace_virtqueue_add_sgs': failed to load: -14
> >   libbpf: failed to load object 'loop6.bpf.o'
> >   scale_test:FAIL:expect_success unexpected error: -14 (errno 14)
> >   #257     verif_scale_loop6:FAIL
> >   Summary: 288/1766 PASSED, 21 SKIPPED, 1 FAILED
> >
> > So, xdp_do_redirect, which was previously failing, now works fine. OTOH,
> > "verif_scale_loop6" now fails, but from what I understand from the log,
> > it has nothing with the series ("8193 jumps is too complex" -- I don't
> > even touch program-related stuff). I don't know what's the reason of it
> > failing, can it be some CI issues or maybe some recent commits?
>
> Yeah. It's an issue with the latest clang.
> We don't have a workaround for this yet.
> It's not a blocker for your patchset.
> We didn't have time to look at it closely.

I applied the workaround for this test.
It's all green now except s390 where it fails with

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

Alex,
could you please take a look at why it's happening?

I suspect it's an endianness issue in:
        if (*metadata != 0x42)
                return XDP_ABORTED;
but your patch didn't change that,
so I'm not sure why it worked before.
  

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 14 Mar 2023 16:54:25 -0700

> On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:

[...]

> 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
> 
> Alex,
> could you please take a look at why it's happening?
> 
> I suspect it's an endianness issue in:
>         if (*metadata != 0x42)
>                 return XDP_ABORTED;
> but your patch didn't change that,
> so I'm not sure why it worked before.

Sure, lemme fix it real quick.

Thanks,
Olek
  

From: Alexander Lobakin <aleksander.lobakin@intel.com>
Date: Wed, 15 Mar 2023 10:56:25 +0100

> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Date: Tue, 14 Mar 2023 16:54:25 -0700
> 
>> On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
> 
> [...]
> 
>> 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
>>
>> Alex,
>> could you please take a look at why it's happening?
>>
>> I suspect it's an endianness issue in:
>>         if (*metadata != 0x42)
>>                 return XDP_ABORTED;
>> but your patch didn't change that,
>> so I'm not sure why it worked before.
> 
> Sure, lemme fix it real quick.

Hi Ilya,

Do you have s390 testing setups? Maybe you could take a look, since I
don't have one and can't debug it? Doesn't seem to be Endianness issue.
I mean, I have this (the below patch), but not sure it will fix
anything -- IIRC eBPF arch always matches the host arch ._.
I can't figure out from the code what does happen wrongly :s And it
happens only on s390.

Thanks,
Olek
---
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 662b6c6c5ed7..b21371668447 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
@@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
 			    .attach_point = BPF_TC_INGRESS);
 
 	memcpy(&data[sizeof(__u32)], &pkt_udp, sizeof(pkt_udp));
-	*((__u32 *)data) = 0x42; /* metadata test value */
+	*((__u32 *)data) = htonl(0x42); /* metadata test value */
 
 	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..2475bc30ced2 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <vmlinux.h>
+#include <bpf/bpf_endian.h>
 #include <bpf/bpf_helpers.h>
 
 #define ETH_ALEN 6
@@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
 SEC("xdp")
 int xdp_redirect(struct xdp_md *xdp)
 {
-	__u32 *metadata = (void *)(long)xdp->data_meta;
+	__be32 *metadata = (void *)(long)xdp->data_meta;
 	void *data_end = (void *)(long)xdp->data_end;
 	void *data = (void *)(long)xdp->data;
 
@@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
 	if (metadata + 1 > data)
 		return XDP_ABORTED;
 
-	if (*metadata != 0x42)
+	if (*metadata != __bpf_htonl(0x42))
 		return XDP_ABORTED;
 
 	if (*payload == MARK_XMIT)
  

On Wed, 2023-03-15 at 11:54 +0100, Alexander Lobakin wrote:
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> Date: Wed, 15 Mar 2023 10:56:25 +0100
> 
> > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Date: Tue, 14 Mar 2023 16:54:25 -0700
> > 
> > > On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > 
> > [...]
> > 
> > > 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
> > > 
> > > Alex,
> > > could you please take a look at why it's happening?
> > > 
> > > I suspect it's an endianness issue in:
> > >         if (*metadata != 0x42)
> > >                 return XDP_ABORTED;
> > > but your patch didn't change that,
> > > so I'm not sure why it worked before.
> > 
> > Sure, lemme fix it real quick.
> 
> Hi Ilya,
> 
> Do you have s390 testing setups? Maybe you could take a look, since I
> don't have one and can't debug it? Doesn't seem to be Endianness
> issue.
> I mean, I have this (the below patch), but not sure it will fix
> anything -- IIRC eBPF arch always matches the host arch ._.
> I can't figure out from the code what does happen wrongly :s And it
> happens only on s390.
> 
> Thanks,
> Olek
> ---
> 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 662b6c6c5ed7..b21371668447 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> @@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
>                             .attach_point = BPF_TC_INGRESS);
>  
>         memcpy(&data[sizeof(__u32)], &pkt_udp, sizeof(pkt_udp));
> -       *((__u32 *)data) = 0x42; /* metadata test value */
> +       *((__u32 *)data) = htonl(0x42); /* metadata test value */
>  
>         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..2475bc30ced2 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <vmlinux.h>
> +#include <bpf/bpf_endian.h>
>  #include <bpf/bpf_helpers.h>
>  
>  #define ETH_ALEN 6
> @@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
>  SEC("xdp")
>  int xdp_redirect(struct xdp_md *xdp)
>  {
> -       __u32 *metadata = (void *)(long)xdp->data_meta;
> +       __be32 *metadata = (void *)(long)xdp->data_meta;
>         void *data_end = (void *)(long)xdp->data_end;
>         void *data = (void *)(long)xdp->data;
>  
> @@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
>         if (metadata + 1 > data)
>                 return XDP_ABORTED;
>  
> -       if (*metadata != 0x42)
> +       if (*metadata != __bpf_htonl(0x42))
>                 return XDP_ABORTED;
>  
>         if (*payload == MARK_XMIT)

Okay, I'll take a look. Two quick observations for now:

- Unfortunately the above patch does not help.

- In dmesg I see:

    Driver unsupported XDP return value 0 on prog xdp_redirect (id 23)
    dev N/A, expect packet loss!
  

On Wed, Mar 15, 2023 at 3:55 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Alexander Lobakin <aleksander.lobakin@intel.com>
> Date: Wed, 15 Mar 2023 10:56:25 +0100
>
> > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Date: Tue, 14 Mar 2023 16:54:25 -0700
> >
> >> On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
> >> <alexei.starovoitov@gmail.com> wrote:
> >
> > [...]
> >
> >> 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
> >>
> >> Alex,
> >> could you please take a look at why it's happening?
> >>
> >> I suspect it's an endianness issue in:
> >>         if (*metadata != 0x42)
> >>                 return XDP_ABORTED;
> >> but your patch didn't change that,
> >> so I'm not sure why it worked before.
> >
> > Sure, lemme fix it real quick.
>
> Hi Ilya,
>
> Do you have s390 testing setups? Maybe you could take a look, since I
> don't have one and can't debug it? Doesn't seem to be Endianness issue.
> I mean, I have this (the below patch), but not sure it will fix
> anything -- IIRC eBPF arch always matches the host arch ._.
> I can't figure out from the code what does happen wrongly :s And it
> happens only on s390.
>
> Thanks,
> Olek
> ---
> 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 662b6c6c5ed7..b21371668447 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> @@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
>                             .attach_point = BPF_TC_INGRESS);
>
>         memcpy(&data[sizeof(__u32)], &pkt_udp, sizeof(pkt_udp));
> -       *((__u32 *)data) = 0x42; /* metadata test value */
> +       *((__u32 *)data) = htonl(0x42); /* metadata test value */
>
>         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..2475bc30ced2 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <vmlinux.h>
> +#include <bpf/bpf_endian.h>
>  #include <bpf/bpf_helpers.h>
>
>  #define ETH_ALEN 6
> @@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
>  SEC("xdp")
>  int xdp_redirect(struct xdp_md *xdp)
>  {
> -       __u32 *metadata = (void *)(long)xdp->data_meta;
> +       __be32 *metadata = (void *)(long)xdp->data_meta;
>         void *data_end = (void *)(long)xdp->data_end;
>         void *data = (void *)(long)xdp->data;
>
> @@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
>         if (metadata + 1 > data)
>                 return XDP_ABORTED;
>
> -       if (*metadata != 0x42)
> +       if (*metadata != __bpf_htonl(0x42))
>                 return XDP_ABORTED;

Looks sane to me.
I'd probably use 'u8 * metadata' instead. Both in bpf and user space
just not to worry about endianness.
Could you please submit an official patch and let CI judge?
  

On Wed, 2023-03-15 at 15:54 +0100, Ilya Leoshkevich wrote:
> On Wed, 2023-03-15 at 11:54 +0100, Alexander Lobakin wrote:
> > From: Alexander Lobakin <aleksander.lobakin@intel.com>
> > Date: Wed, 15 Mar 2023 10:56:25 +0100
> > 
> > > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > Date: Tue, 14 Mar 2023 16:54:25 -0700
> > > 
> > > > On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > 
> > > [...]
> > > 
> > > > 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
> > > > 
> > > > Alex,
> > > > could you please take a look at why it's happening?
> > > > 
> > > > I suspect it's an endianness issue in:
> > > >         if (*metadata != 0x42)
> > > >                 return XDP_ABORTED;
> > > > but your patch didn't change that,
> > > > so I'm not sure why it worked before.
> > > 
> > > Sure, lemme fix it real quick.
> > 
> > Hi Ilya,
> > 
> > Do you have s390 testing setups? Maybe you could take a look, since
> > I
> > don't have one and can't debug it? Doesn't seem to be Endianness
> > issue.
> > I mean, I have this (the below patch), but not sure it will fix
> > anything -- IIRC eBPF arch always matches the host arch ._.
> > I can't figure out from the code what does happen wrongly :s And it
> > happens only on s390.
> > 
> > Thanks,
> > Olek
> > ---
> > 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 662b6c6c5ed7..b21371668447 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> > @@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
> >                             .attach_point = BPF_TC_INGRESS);
> >  
> >         memcpy(&data[sizeof(__u32)], &pkt_udp, sizeof(pkt_udp));
> > -       *((__u32 *)data) = 0x42; /* metadata test value */
> > +       *((__u32 *)data) = htonl(0x42); /* metadata test value */
> >  
> >         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..2475bc30ced2 100644
> > --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> > +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> > @@ -1,5 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <vmlinux.h>
> > +#include <bpf/bpf_endian.h>
> >  #include <bpf/bpf_helpers.h>
> >  
> >  #define ETH_ALEN 6
> > @@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
> >  SEC("xdp")
> >  int xdp_redirect(struct xdp_md *xdp)
> >  {
> > -       __u32 *metadata = (void *)(long)xdp->data_meta;
> > +       __be32 *metadata = (void *)(long)xdp->data_meta;
> >         void *data_end = (void *)(long)xdp->data_end;
> >         void *data = (void *)(long)xdp->data;
> >  
> > @@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
> >         if (metadata + 1 > data)
> >                 return XDP_ABORTED;
> >  
> > -       if (*metadata != 0x42)
> > +       if (*metadata != __bpf_htonl(0x42))
> >                 return XDP_ABORTED;
> >  
> >         if (*payload == MARK_XMIT)
> 
> Okay, I'll take a look. Two quick observations for now:
> 
> - Unfortunately the above patch does not help.
> 
> - In dmesg I see:
> 
>     Driver unsupported XDP return value 0 on prog xdp_redirect (id
> 23)
>     dev N/A, expect packet loss!

I haven't identified the issue yet, but I have found a couple more
things that might be helpful:

- In problematic cases metadata contains 0, so this is not an
  endianness issue. data is still reasonable though. I'm trying to
  understand what is causing this.

- Applying the following diff:

--- 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 (false && bpf_xdp_adjust_meta(xdp, 4))
                return XDP_ABORTED;
 
        if (retcode > XDP_PASS)

causes a kernel panic even on x86_64:

BUG: kernel NULL pointer dereference, address: 0000000000000d28       
...
Call Trace:            
 <TASK>                                                               
 build_skb_around+0x22/0xb0
 __xdp_build_skb_from_frame+0x4e/0x130
 bpf_test_run_xdp_live+0x65f/0x7c0
 ? __pfx_xdp_test_run_init_page+0x10/0x10
 bpf_prog_test_run_xdp+0x2ba/0x480
 bpf_prog_test_run+0xeb/0x110
 __sys_bpf+0x2b9/0x570
 __x64_sys_bpf+0x1c/0x30
 do_syscall_64+0x48/0xa0
 entry_SYSCALL_64_after_hwframe+0x72/0xdc

I haven't looked into this at all, but I believe this needs to be
fixed - BPF should never cause kernel panics.
  

From: Ilya Leoshkevich <iii@linux.ibm.com>
Date: Wed, 15 Mar 2023 19:00:47 +0100

> On Wed, 2023-03-15 at 15:54 +0100, Ilya Leoshkevich wrote:
>> On Wed, 2023-03-15 at 11:54 +0100, Alexander Lobakin wrote:
>>> From: Alexander Lobakin <aleksander.lobakin@intel.com>
>>> Date: Wed, 15 Mar 2023 10:56:25 +0100
>>>
>>>> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>>>> Date: Tue, 14 Mar 2023 16:54:25 -0700
>>>>
>>>>> On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> [...]
>>>>
>>>>> 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
>>>>>
>>>>> Alex,
>>>>> could you please take a look at why it's happening?
>>>>>
>>>>> I suspect it's an endianness issue in:
>>>>>         if (*metadata != 0x42)
>>>>>                 return XDP_ABORTED;
>>>>> but your patch didn't change that,
>>>>> so I'm not sure why it worked before.
>>>>
>>>> Sure, lemme fix it real quick.
>>>
>>> Hi Ilya,
>>>
>>> Do you have s390 testing setups? Maybe you could take a look, since
>>> I
>>> don't have one and can't debug it? Doesn't seem to be Endianness
>>> issue.
>>> I mean, I have this (the below patch), but not sure it will fix
>>> anything -- IIRC eBPF arch always matches the host arch ._.
>>> I can't figure out from the code what does happen wrongly :s And it
>>> happens only on s390.
>>>
>>> Thanks,
>>> Olek
>>> ---
>>> 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 662b6c6c5ed7..b21371668447 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
>>> @@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
>>>                             .attach_point = BPF_TC_INGRESS);
>>>  
>>>         memcpy(&data[sizeof(__u32)], &pkt_udp, sizeof(pkt_udp));
>>> -       *((__u32 *)data) = 0x42; /* metadata test value */
>>> +       *((__u32 *)data) = htonl(0x42); /* metadata test value */
>>>  
>>>         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..2475bc30ced2 100644
>>> --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
>>> +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
>>> @@ -1,5 +1,6 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>  #include <vmlinux.h>
>>> +#include <bpf/bpf_endian.h>
>>>  #include <bpf/bpf_helpers.h>
>>>  
>>>  #define ETH_ALEN 6
>>> @@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
>>>  SEC("xdp")
>>>  int xdp_redirect(struct xdp_md *xdp)
>>>  {
>>> -       __u32 *metadata = (void *)(long)xdp->data_meta;
>>> +       __be32 *metadata = (void *)(long)xdp->data_meta;
>>>         void *data_end = (void *)(long)xdp->data_end;
>>>         void *data = (void *)(long)xdp->data;
>>>  
>>> @@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
>>>         if (metadata + 1 > data)
>>>                 return XDP_ABORTED;
>>>  
>>> -       if (*metadata != 0x42)
>>> +       if (*metadata != __bpf_htonl(0x42))
>>>                 return XDP_ABORTED;
>>>  
>>>         if (*payload == MARK_XMIT)
>>
>> Okay, I'll take a look. Two quick observations for now:
>>
>> - Unfortunately the above patch does not help.
>>
>> - In dmesg I see:
>>
>>     Driver unsupported XDP return value 0 on prog xdp_redirect (id
>> 23)
>>     dev N/A, expect packet loss!
> 
> I haven't identified the issue yet, but I have found a couple more
> things that might be helpful:
> 
> - In problematic cases metadata contains 0, so this is not an
>   endianness issue. data is still reasonable though. I'm trying to
>   understand what is causing this.
> 
> - Applying the following diff:
> 
> --- 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 (false && bpf_xdp_adjust_meta(xdp, 4))
>                 return XDP_ABORTED;
>  
>         if (retcode > XDP_PASS)
> 
> causes a kernel panic even on x86_64:
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000d28       
> ...
> Call Trace:            
>  <TASK>                                                               
>  build_skb_around+0x22/0xb0
>  __xdp_build_skb_from_frame+0x4e/0x130
>  bpf_test_run_xdp_live+0x65f/0x7c0
>  ? __pfx_xdp_test_run_init_page+0x10/0x10
>  bpf_prog_test_run_xdp+0x2ba/0x480
>  bpf_prog_test_run+0xeb/0x110
>  __sys_bpf+0x2b9/0x570
>  __x64_sys_bpf+0x1c/0x30
>  do_syscall_64+0x48/0xa0
>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> 
> I haven't looked into this at all, but I believe this needs to be
> fixed - BPF should never cause kernel panics.

This one is basically the same issue as syzbot mentioned today (separate
subthread). I'm waiting for a feedback from Toke on which way of fixing
he'd prefer (I proposed 2). If those zeroed metadata magics that you
observe have the same roots with the panic, one fix will smash 2 issues.

Thanks,
Olek
  

On Wed, 2023-03-15 at 19:12 +0100, Alexander Lobakin wrote:
> From: Ilya Leoshkevich <iii@linux.ibm.com>
> Date: Wed, 15 Mar 2023 19:00:47 +0100
> 
> > On Wed, 2023-03-15 at 15:54 +0100, Ilya Leoshkevich wrote:
> > > On Wed, 2023-03-15 at 11:54 +0100, Alexander Lobakin wrote:
> > > > From: Alexander Lobakin <aleksander.lobakin@intel.com>
> > > > Date: Wed, 15 Mar 2023 10:56:25 +0100
> > > > 
> > > > > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > > > Date: Tue, 14 Mar 2023 16:54:25 -0700
> > > > > 
> > > > > > On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
> > > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > 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
> > > > > > 
> > > > > > Alex,
> > > > > > could you please take a look at why it's happening?
> > > > > > 
> > > > > > I suspect it's an endianness issue in:
> > > > > >         if (*metadata != 0x42)
> > > > > >                 return XDP_ABORTED;
> > > > > > but your patch didn't change that,
> > > > > > so I'm not sure why it worked before.
> > > > > 
> > > > > Sure, lemme fix it real quick.
> > > > 
> > > > Hi Ilya,
> > > > 
> > > > Do you have s390 testing setups? Maybe you could take a look,
> > > > since
> > > > I
> > > > don't have one and can't debug it? Doesn't seem to be
> > > > Endianness
> > > > issue.
> > > > I mean, I have this (the below patch), but not sure it will fix
> > > > anything -- IIRC eBPF arch always matches the host arch ._.
> > > > I can't figure out from the code what does happen wrongly :s
> > > > And it
> > > > happens only on s390.
> > > > 
> > > > Thanks,
> > > > Olek
> > > > ---
> > > > 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 662b6c6c5ed7..b21371668447 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> > > > @@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
> > > >                             .attach_point = BPF_TC_INGRESS);
> > > >  
> > > >         memcpy(&data[sizeof(__u32)], &pkt_udp,
> > > > sizeof(pkt_udp));
> > > > -       *((__u32 *)data) = 0x42; /* metadata test value */
> > > > +       *((__u32 *)data) = htonl(0x42); /* metadata test value
> > > > */
> > > >  
> > > >         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..2475bc30ced2 100644
> > > > --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> > > > +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
> > > > @@ -1,5 +1,6 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  #include <vmlinux.h>
> > > > +#include <bpf/bpf_endian.h>
> > > >  #include <bpf/bpf_helpers.h>
> > > >  
> > > >  #define ETH_ALEN 6
> > > > @@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
> > > >  SEC("xdp")
> > > >  int xdp_redirect(struct xdp_md *xdp)
> > > >  {
> > > > -       __u32 *metadata = (void *)(long)xdp->data_meta;
> > > > +       __be32 *metadata = (void *)(long)xdp->data_meta;
> > > >         void *data_end = (void *)(long)xdp->data_end;
> > > >         void *data = (void *)(long)xdp->data;
> > > >  
> > > > @@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
> > > >         if (metadata + 1 > data)
> > > >                 return XDP_ABORTED;
> > > >  
> > > > -       if (*metadata != 0x42)
> > > > +       if (*metadata != __bpf_htonl(0x42))
> > > >                 return XDP_ABORTED;
> > > >  
> > > >         if (*payload == MARK_XMIT)
> > > 
> > > Okay, I'll take a look. Two quick observations for now:
> > > 
> > > - Unfortunately the above patch does not help.
> > > 
> > > - In dmesg I see:
> > > 
> > >     Driver unsupported XDP return value 0 on prog xdp_redirect
> > > (id
> > > 23)
> > >     dev N/A, expect packet loss!
> > 
> > I haven't identified the issue yet, but I have found a couple more
> > things that might be helpful:
> > 
> > - In problematic cases metadata contains 0, so this is not an
> >   endianness issue. data is still reasonable though. I'm trying to
> >   understand what is causing this.
> > 
> > - Applying the following diff:
> > 
> > --- 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 (false && bpf_xdp_adjust_meta(xdp, 4))
> >                 return XDP_ABORTED;
> >  
> >         if (retcode > XDP_PASS)
> > 
> > causes a kernel panic even on x86_64:
> > 
> > BUG: kernel NULL pointer dereference, address:
> > 0000000000000d28       
> > ...
> > Call Trace:            
> >  <TASK>                                                            
> >    
> >  build_skb_around+0x22/0xb0
> >  __xdp_build_skb_from_frame+0x4e/0x130
> >  bpf_test_run_xdp_live+0x65f/0x7c0
> >  ? __pfx_xdp_test_run_init_page+0x10/0x10
> >  bpf_prog_test_run_xdp+0x2ba/0x480
> >  bpf_prog_test_run+0xeb/0x110
> >  __sys_bpf+0x2b9/0x570
> >  __x64_sys_bpf+0x1c/0x30
> >  do_syscall_64+0x48/0xa0
> >  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> > 
> > I haven't looked into this at all, but I believe this needs to be
> > fixed - BPF should never cause kernel panics.
> 
> This one is basically the same issue as syzbot mentioned today
> (separate
> subthread). I'm waiting for a feedback from Toke on which way of
> fixing
> he'd prefer (I proposed 2). If those zeroed metadata magics that you
> observe have the same roots with the panic, one fix will smash 2
> issues.
> 
> Thanks,
> Olek

Sounds good, I will wait for an update then.

In the meantime, I found the code that overwrites the metadata:

#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
#5  0x0000000000ab5cbc in dst_input (skb=<optimized out>) at
linux/include/net/dst.h:454
#6  ip6_sublist_rcv_finish (head=head@entry=0x38000dbf520) at
linux/net/ipv6/ip6_input.c:88
#7  0x0000000000ab6104 in ip6_list_rcv_finish (net=<optimized out>,
head=<optimized out>, sk=0x0) at linux/net/ipv6/ip6_input.c:145
#8  0x0000000000ab72bc in ipv6_list_rcv (head=0x38000dbf638,
pt=<optimized out>, orig_dev=<optimized out>) at
linux/net/ipv6/ip6_input.c:354
#9  0x00000000008b3710 in __netif_receive_skb_list_ptype
(orig_dev=0x880b8000, pt_prev=0x176b7f8 <ipv6_packet_type>,
head=0x38000dbf638) at linux/net/core/dev.c:5520
#10 __netif_receive_skb_list_core (head=head@entry=0x38000dbf7b8,
pfmemalloc=pfmemalloc@entry=false) at linux/net/core/dev.c:5568
#11 0x00000000008b4390 in __netif_receive_skb_list (head=0x38000dbf7b8)
at linux/net/core/dev.c:5620
#12 netif_receive_skb_list_internal (head=head@entry=0x38000dbf7b8) at
linux/net/core/dev.c:5711
#13 0x00000000008b45ce in netif_receive_skb_list
(head=head@entry=0x38000dbf7b8) at linux/net/core/dev.c:5763
#14 0x0000000000950782 in xdp_recv_frames (dev=<optimized out>,
skbs=<optimized out>, nframes=62, frames=0x8587c600) at
linux/net/bpf/test_run.c:256
#15 xdp_test_run_batch (xdp=xdp@entry=0x38000dbf900,
prog=prog@entry=0x37fffe75000, repeat=<optimized out>) at
linux/net/bpf/test_run.c:334

namely:

static inline int neigh_hh_output(const struct hh_cache *hh, struct
sk_buff *skb)
   ...
   memcpy(skb->data - HH_DATA_MOD, hh->hh_data, HH_DATA_MOD);

It's hard for me to see what is going on here, since I'm not familiar
with the networking code - since XDP metadata is located at the end of
headroom, should not there be something that prevents the network stack
from overwriting it? Or can it be that netif_receive_skb_list() is
free to do whatever it wants with that memory and we cannot expect to
receive it back intact?
  

From: Ilya Leoshkevich <iii@linux.ibm.com>
Date: Wed, 15 Mar 2023 19:26:00 +0100

> On Wed, 2023-03-15 at 19:12 +0100, Alexander Lobakin wrote:
>> From: Ilya Leoshkevich <iii@linux.ibm.com>
>> Date: Wed, 15 Mar 2023 19:00:47 +0100
>>
>>> On Wed, 2023-03-15 at 15:54 +0100, Ilya Leoshkevich wrote:
>>>> On Wed, 2023-03-15 at 11:54 +0100, Alexander Lobakin wrote:
>>>>> From: Alexander Lobakin <aleksander.lobakin@intel.com>
>>>>> Date: Wed, 15 Mar 2023 10:56:25 +0100
>>>>>
>>>>>> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>>>>>> Date: Tue, 14 Mar 2023 16:54:25 -0700
>>>>>>
>>>>>>> On Tue, Mar 14, 2023 at 11:52 AM Alexei Starovoitov
>>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>> Alex,
>>>>>>> could you please take a look at why it's happening?
>>>>>>>
>>>>>>> I suspect it's an endianness issue in:
>>>>>>>         if (*metadata != 0x42)
>>>>>>>                 return XDP_ABORTED;
>>>>>>> but your patch didn't change that,
>>>>>>> so I'm not sure why it worked before.
>>>>>>
>>>>>> Sure, lemme fix it real quick.
>>>>>
>>>>> Hi Ilya,
>>>>>
>>>>> Do you have s390 testing setups? Maybe you could take a look,
>>>>> since
>>>>> I
>>>>> don't have one and can't debug it? Doesn't seem to be
>>>>> Endianness
>>>>> issue.
>>>>> I mean, I have this (the below patch), but not sure it will fix
>>>>> anything -- IIRC eBPF arch always matches the host arch ._.
>>>>> I can't figure out from the code what does happen wrongly :s
>>>>> And it
>>>>> happens only on s390.
>>>>>
>>>>> Thanks,
>>>>> Olek
>>>>> ---
>>>>> 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 662b6c6c5ed7..b21371668447 100644
>>>>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
>>>>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
>>>>> @@ -107,7 +107,7 @@ void test_xdp_do_redirect(void)
>>>>>                             .attach_point = BPF_TC_INGRESS);
>>>>>  
>>>>>         memcpy(&data[sizeof(__u32)], &pkt_udp,
>>>>> sizeof(pkt_udp));
>>>>> -       *((__u32 *)data) = 0x42; /* metadata test value */
>>>>> +       *((__u32 *)data) = htonl(0x42); /* metadata test value
>>>>> */
>>>>>  
>>>>>         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..2475bc30ced2 100644
>>>>> --- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
>>>>> +++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
>>>>> @@ -1,5 +1,6 @@
>>>>>  // SPDX-License-Identifier: GPL-2.0
>>>>>  #include <vmlinux.h>
>>>>> +#include <bpf/bpf_endian.h>
>>>>>  #include <bpf/bpf_helpers.h>
>>>>>  
>>>>>  #define ETH_ALEN 6
>>>>> @@ -28,7 +29,7 @@ volatile int retcode = XDP_REDIRECT;
>>>>>  SEC("xdp")
>>>>>  int xdp_redirect(struct xdp_md *xdp)
>>>>>  {
>>>>> -       __u32 *metadata = (void *)(long)xdp->data_meta;
>>>>> +       __be32 *metadata = (void *)(long)xdp->data_meta;
>>>>>         void *data_end = (void *)(long)xdp->data_end;
>>>>>         void *data = (void *)(long)xdp->data;
>>>>>  
>>>>> @@ -44,7 +45,7 @@ int xdp_redirect(struct xdp_md *xdp)
>>>>>         if (metadata + 1 > data)
>>>>>                 return XDP_ABORTED;
>>>>>  
>>>>> -       if (*metadata != 0x42)
>>>>> +       if (*metadata != __bpf_htonl(0x42))
>>>>>                 return XDP_ABORTED;
>>>>>  
>>>>>         if (*payload == MARK_XMIT)
>>>>
>>>> Okay, I'll take a look. Two quick observations for now:
>>>>
>>>> - Unfortunately the above patch does not help.
>>>>
>>>> - In dmesg I see:
>>>>
>>>>     Driver unsupported XDP return value 0 on prog xdp_redirect
>>>> (id
>>>> 23)
>>>>     dev N/A, expect packet loss!
>>>
>>> I haven't identified the issue yet, but I have found a couple more
>>> things that might be helpful:
>>>
>>> - In problematic cases metadata contains 0, so this is not an
>>>   endianness issue. data is still reasonable though. I'm trying to
>>>   understand what is causing this.
>>>
>>> - Applying the following diff:
>>>
>>> --- 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 (false && bpf_xdp_adjust_meta(xdp, 4))
>>>                 return XDP_ABORTED;
>>>  
>>>         if (retcode > XDP_PASS)
>>>
>>> causes a kernel panic even on x86_64:
>>>
>>> BUG: kernel NULL pointer dereference, address:
>>> 0000000000000d28       
>>> ...
>>> Call Trace:            
>>>  <TASK>                                                            
>>>    
>>>  build_skb_around+0x22/0xb0
>>>  __xdp_build_skb_from_frame+0x4e/0x130
>>>  bpf_test_run_xdp_live+0x65f/0x7c0
>>>  ? __pfx_xdp_test_run_init_page+0x10/0x10
>>>  bpf_prog_test_run_xdp+0x2ba/0x480
>>>  bpf_prog_test_run+0xeb/0x110
>>>  __sys_bpf+0x2b9/0x570
>>>  __x64_sys_bpf+0x1c/0x30
>>>  do_syscall_64+0x48/0xa0
>>>  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>
>>> I haven't looked into this at all, but I believe this needs to be
>>> fixed - BPF should never cause kernel panics.
>>
>> This one is basically the same issue as syzbot mentioned today
>> (separate
>> subthread). I'm waiting for a feedback from Toke on which way of
>> fixing
>> he'd prefer (I proposed 2). If those zeroed metadata magics that you
>> observe have the same roots with the panic, one fix will smash 2
>> issues.
>>
>> Thanks,
>> Olek
> 
> Sounds good, I will wait for an update then.
> 
> In the meantime, I found the code that overwrites the metadata:
> 
> #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
> #5  0x0000000000ab5cbc in dst_input (skb=<optimized out>) at
> linux/include/net/dst.h:454
> #6  ip6_sublist_rcv_finish (head=head@entry=0x38000dbf520) at
> linux/net/ipv6/ip6_input.c:88
> #7  0x0000000000ab6104 in ip6_list_rcv_finish (net=<optimized out>,
> head=<optimized out>, sk=0x0) at linux/net/ipv6/ip6_input.c:145
> #8  0x0000000000ab72bc in ipv6_list_rcv (head=0x38000dbf638,
> pt=<optimized out>, orig_dev=<optimized out>) at
> linux/net/ipv6/ip6_input.c:354
> #9  0x00000000008b3710 in __netif_receive_skb_list_ptype
> (orig_dev=0x880b8000, pt_prev=0x176b7f8 <ipv6_packet_type>,
> head=0x38000dbf638) at linux/net/core/dev.c:5520
> #10 __netif_receive_skb_list_core (head=head@entry=0x38000dbf7b8,
> pfmemalloc=pfmemalloc@entry=false) at linux/net/core/dev.c:5568
> #11 0x00000000008b4390 in __netif_receive_skb_list (head=0x38000dbf7b8)
> at linux/net/core/dev.c:5620
> #12 netif_receive_skb_list_internal (head=head@entry=0x38000dbf7b8) at
> linux/net/core/dev.c:5711
> #13 0x00000000008b45ce in netif_receive_skb_list
> (head=head@entry=0x38000dbf7b8) at linux/net/core/dev.c:5763
> #14 0x0000000000950782 in xdp_recv_frames (dev=<optimized out>,
> skbs=<optimized out>, nframes=62, frames=0x8587c600) at
> linux/net/bpf/test_run.c:256
> #15 xdp_test_run_batch (xdp=xdp@entry=0x38000dbf900,
> prog=prog@entry=0x37fffe75000, repeat=<optimized out>) at
> linux/net/bpf/test_run.c:334
> 
> namely:
> 
> static inline int neigh_hh_output(const struct hh_cache *hh, struct
> sk_buff *skb)
>    ...
>    memcpy(skb->data - HH_DATA_MOD, hh->hh_data, HH_DATA_MOD);
> 
> It's hard for me to see what is going on here, since I'm not familiar
> with the networking code - since XDP metadata is located at the end of
> headroom, should not there be something that prevents the network stack
> from overwriting it? Or can it be that netif_receive_skb_list() is

Ok I got it. Much thanks for the debugging! It gets overwritten when
neigh puts the MAC addr back when xmitting. It works on LE, because 0x42
fits into one bit and it's stored in `mac_addr - 4`. On BE this byte is
`mac_addr - 1`. Neigh rounds 14 bytes of Ethernet header to 16, so two
bytes of metadata get wiped.

This is not the same bug as the one that syzbot reported, but they are
provoked by the same: assumptions that %XDP_PASS guarantees the page
won't come back to the pool.
So there are two ways: the first one is to fix those two bugs (two
oneliners basically), the second one is to turn off the recycling for
bpf_test_run at all. I like the first more as it theoretically keeps the
perf boost for bpf_test_run gained from enabling the recycling, but it
can't guarantee similar stuff won't happen soon :D Something else might
get overwritten somewhere else and so on. The second one will
effectively revert the logics for bpf_test_run to the pre-recycling state.
I'll submit the first way today, it will be a series of two. Will see
then how it goes, I'm fine with both ways.

> free to do whatever it wants with that memory and we cannot expect to
> receive it back intact?
Ideally, yes, after a frame is passed outside the driver, you can't
assume anything on its page won't be changed. But that's one of the
bpf_test_run's "features" that gives it nice perf :D So as long as
assumptions don't get broken by some change, like this one with the
recycling, it just works.

Thanks,
Olek