diff mbox series

[bpf-next,v5,9/9] bpf: xfrm: Add selftest for bpf_xdp_get_xfrm_state()

Message ID 8ec1b885d2e13fcd20944cce9edc0340d993d044.1702325874.git.dxu@dxuuu.xyz (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Add bpf_xdp_get_xfrm_state() kfunc | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 2 maintainers not CCed: liuhangbin@gmail.com cehrig@cloudflare.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: Possible repeated word: 'Antony' WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst WARNING: quoted string split across lines
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-10 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-11 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / veristat

Commit Message

Daniel Xu Dec. 11, 2023, 8:20 p.m. UTC 
This commit extends test_tunnel selftest to test the new XDP xfrm state
lookup kfunc.

Co-developed-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
 .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
 2 files changed, 67 insertions(+), 4 deletions(-)

Comments

Eyal Birger Dec. 11, 2023, 9:39 p.m. UTC | #1 
Hi Daniel,

Tiny nits below in case you respin this for other reasons:

On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> This commit extends test_tunnel selftest to test the new XDP xfrm state
> lookup kfunc.
>
> Co-developed-by: Antony Antony <antony.antony@secunet.com>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
>  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
>  2 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> index 2d7f8fa82ebd..fc804095d578 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
>         SYS(fail,
>             "ip netns exec at_ns0 "
>                 "ip xfrm state add src %s dst %s proto esp "
> -                       "spi %d reqid 1 mode tunnel "
> +                       "spi %d reqid 1 mode tunnel replay-window 42 "
>                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
>             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
>         SYS(fail,
> @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
>         SYS(fail,
>             "ip netns exec at_ns0 "
>                 "ip xfrm state add src %s dst %s proto esp "
> -                       "spi %d reqid 2 mode tunnel "
> +                       "spi %d reqid 2 mode tunnel replay-window 42 "

nit: why do you need to set the replay-window in both directions?

>                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
>             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
>         SYS(fail,
> @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
>          */
>         SYS(fail,
>             "ip xfrm state add src %s dst %s proto esp "
> -                   "spi %d reqid 1 mode tunnel "
> +                   "spi %d reqid 1 mode tunnel replay-window 42 "
>                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
>             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
>         SYS(fail,
> @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
>         /* root -> at_ns0 */
>         SYS(fail,
>             "ip xfrm state add src %s dst %s proto esp "
> -                   "spi %d reqid 2 mode tunnel "
> +                   "spi %d reqid 2 mode tunnel replay-window 42 "
>                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
>             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
>         SYS(fail,
> @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
>  {
>         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
>                             .attach_point = BPF_TC_INGRESS);
> +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
>         struct test_tunnel_kern *skel = NULL;
>         struct nstoken *nstoken;
> +       int xdp_prog_fd;
>         int tc_prog_fd;
>         int ifindex;
>         int err;
> @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
>         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
>                 goto done;
>
> +       /* attach xdp prog to tunnel dev */
> +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> +               goto done;
> +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> +               goto done;
> +
>         /* ping from at_ns0 namespace test */
>         nstoken = open_netns("at_ns0");
>         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
>                 goto done;
>         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
>                 goto done;
> +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> +               goto done;
>
>  done:
>         delete_xfrm_tunnel();
> diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> index 3a59eb9c34de..c0dd38616562 100644
> --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
>                           struct bpf_fou_encap *encap, int type) __ksym;
>  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
>                           struct bpf_fou_encap *encap) __ksym;
> +struct xfrm_state *
> +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> +                      u32 opts__sz) __ksym;
> +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
>
>  struct {
>         __uint(type, BPF_MAP_TYPE_ARRAY);
> @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
>         return TC_ACT_OK;
>  }
>
> +volatile int xfrm_replay_window = 0;
> +
> +SEC("xdp")
> +int xfrm_get_state_xdp(struct xdp_md *xdp)
> +{
> +       struct bpf_xfrm_state_opts opts = {};
> +       struct xfrm_state *x = NULL;
> +       struct ip_esp_hdr *esph;
> +       struct bpf_dynptr ptr;
> +       u8 esph_buf[8] = {};
> +       u8 iph_buf[20] = {};
> +       struct iphdr *iph;
> +       u32 off;
> +
> +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> +               goto out;
> +
> +       off = sizeof(struct ethhdr);
> +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> +       if (!iph || iph->protocol != IPPROTO_ESP)
> +               goto out;
> +
> +       off += sizeof(struct iphdr);
> +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> +       if (!esph)
> +               goto out;
> +
> +       opts.netns_id = BPF_F_CURRENT_NETNS;
> +       opts.daddr.a4 = iph->daddr;
> +       opts.spi = esph->spi;
> +       opts.proto = IPPROTO_ESP;
> +       opts.family = AF_INET;
> +
> +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> +       if (!x || opts.error)

nit: how can opts.error be non zero if x == NULL?


Eyal.


> +               goto out;
> +
> +       if (!x->replay_esn)
> +               goto out;
> +
> +       xfrm_replay_window = x->replay_esn->replay_window;
> +out:
> +       if (x)
> +               bpf_xdp_xfrm_state_release(x);
> +       return XDP_PASS;
> +}
> +
>  char _license[] SEC("license") = "GPL";
> --
> 2.42.1
>
Daniel Xu Dec. 11, 2023, 10:31 p.m. UTC | #2 
On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> Hi Daniel,
> 
> Tiny nits below in case you respin this for other reasons:
> 
> On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > lookup kfunc.
> >
> > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> >  2 files changed, 67 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > index 2d7f8fa82ebd..fc804095d578 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> >         SYS(fail,
> >             "ip netns exec at_ns0 "
> >                 "ip xfrm state add src %s dst %s proto esp "
> > -                       "spi %d reqid 1 mode tunnel "
> > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> >         SYS(fail,
> > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> >         SYS(fail,
> >             "ip netns exec at_ns0 "
> >                 "ip xfrm state add src %s dst %s proto esp "
> > -                       "spi %d reqid 2 mode tunnel "
> > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> 
> nit: why do you need to set the replay-window in both directions?

No reason - probably just careless here.

> 
> >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> >         SYS(fail,
> > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> >          */
> >         SYS(fail,
> >             "ip xfrm state add src %s dst %s proto esp "
> > -                   "spi %d reqid 1 mode tunnel "
> > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> >         SYS(fail,
> > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> >         /* root -> at_ns0 */
> >         SYS(fail,
> >             "ip xfrm state add src %s dst %s proto esp "
> > -                   "spi %d reqid 2 mode tunnel "
> > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> >         SYS(fail,
> > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> >  {
> >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> >                             .attach_point = BPF_TC_INGRESS);
> > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> >         struct test_tunnel_kern *skel = NULL;
> >         struct nstoken *nstoken;
> > +       int xdp_prog_fd;
> >         int tc_prog_fd;
> >         int ifindex;
> >         int err;
> > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> >                 goto done;
> >
> > +       /* attach xdp prog to tunnel dev */
> > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > +               goto done;
> > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > +               goto done;
> > +
> >         /* ping from at_ns0 namespace test */
> >         nstoken = open_netns("at_ns0");
> >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> >                 goto done;
> >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> >                 goto done;
> > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > +               goto done;
> >
> >  done:
> >         delete_xfrm_tunnel();
> > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > index 3a59eb9c34de..c0dd38616562 100644
> > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> >                           struct bpf_fou_encap *encap, int type) __ksym;
> >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> >                           struct bpf_fou_encap *encap) __ksym;
> > +struct xfrm_state *
> > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > +                      u32 opts__sz) __ksym;
> > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> >
> >  struct {
> >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> >         return TC_ACT_OK;
> >  }
> >
> > +volatile int xfrm_replay_window = 0;
> > +
> > +SEC("xdp")
> > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > +{
> > +       struct bpf_xfrm_state_opts opts = {};
> > +       struct xfrm_state *x = NULL;
> > +       struct ip_esp_hdr *esph;
> > +       struct bpf_dynptr ptr;
> > +       u8 esph_buf[8] = {};
> > +       u8 iph_buf[20] = {};
> > +       struct iphdr *iph;
> > +       u32 off;
> > +
> > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > +               goto out;
> > +
> > +       off = sizeof(struct ethhdr);
> > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > +               goto out;
> > +
> > +       off += sizeof(struct iphdr);
> > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > +       if (!esph)
> > +               goto out;
> > +
> > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > +       opts.daddr.a4 = iph->daddr;
> > +       opts.spi = esph->spi;
> > +       opts.proto = IPPROTO_ESP;
> > +       opts.family = AF_INET;
> > +
> > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > +       if (!x || opts.error)
> 
> nit: how can opts.error be non zero if x == NULL?

Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
behavior here.

[...]

Thanks,
Daniel
Eyal Birger Dec. 11, 2023, 11:13 p.m. UTC | #3 
On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > Hi Daniel,
> >
> > Tiny nits below in case you respin this for other reasons:
> >
> > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > >
> > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > lookup kfunc.
> > >
> > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > ---
> > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > index 2d7f8fa82ebd..fc804095d578 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > >         SYS(fail,
> > >             "ip netns exec at_ns0 "
> > >                 "ip xfrm state add src %s dst %s proto esp "
> > > -                       "spi %d reqid 1 mode tunnel "
> > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > >         SYS(fail,
> > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > >         SYS(fail,
> > >             "ip netns exec at_ns0 "
> > >                 "ip xfrm state add src %s dst %s proto esp "
> > > -                       "spi %d reqid 2 mode tunnel "
> > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> >
> > nit: why do you need to set the replay-window in both directions?
>
> No reason - probably just careless here.
>
> >
> > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > >         SYS(fail,
> > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > >          */
> > >         SYS(fail,
> > >             "ip xfrm state add src %s dst %s proto esp "
> > > -                   "spi %d reqid 1 mode tunnel "
> > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > >         SYS(fail,
> > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > >         /* root -> at_ns0 */
> > >         SYS(fail,
> > >             "ip xfrm state add src %s dst %s proto esp "
> > > -                   "spi %d reqid 2 mode tunnel "
> > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > >         SYS(fail,
> > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > >  {
> > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > >                             .attach_point = BPF_TC_INGRESS);
> > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > >         struct test_tunnel_kern *skel = NULL;
> > >         struct nstoken *nstoken;
> > > +       int xdp_prog_fd;
> > >         int tc_prog_fd;
> > >         int ifindex;
> > >         int err;
> > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > >                 goto done;
> > >
> > > +       /* attach xdp prog to tunnel dev */
> > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > +               goto done;
> > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > +               goto done;
> > > +
> > >         /* ping from at_ns0 namespace test */
> > >         nstoken = open_netns("at_ns0");
> > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > >                 goto done;
> > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > >                 goto done;
> > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > +               goto done;
> > >
> > >  done:
> > >         delete_xfrm_tunnel();
> > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > index 3a59eb9c34de..c0dd38616562 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > >                           struct bpf_fou_encap *encap) __ksym;
> > > +struct xfrm_state *
> > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > +                      u32 opts__sz) __ksym;
> > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > >
> > >  struct {
> > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > >         return TC_ACT_OK;
> > >  }
> > >
> > > +volatile int xfrm_replay_window = 0;
> > > +
> > > +SEC("xdp")
> > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > +{
> > > +       struct bpf_xfrm_state_opts opts = {};
> > > +       struct xfrm_state *x = NULL;
> > > +       struct ip_esp_hdr *esph;
> > > +       struct bpf_dynptr ptr;
> > > +       u8 esph_buf[8] = {};
> > > +       u8 iph_buf[20] = {};
> > > +       struct iphdr *iph;
> > > +       u32 off;
> > > +
> > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > +               goto out;
> > > +
> > > +       off = sizeof(struct ethhdr);
> > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > +               goto out;
> > > +
> > > +       off += sizeof(struct iphdr);
> > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > +       if (!esph)
> > > +               goto out;
> > > +
> > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > +       opts.daddr.a4 = iph->daddr;
> > > +       opts.spi = esph->spi;
> > > +       opts.proto = IPPROTO_ESP;
> > > +       opts.family = AF_INET;
> > > +
> > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > +       if (!x || opts.error)
> >
> > nit: how can opts.error be non zero if x == NULL?
>
> Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> behavior here.

I'm sorry, I don't understand.

AFAICT, regardless of the -ENOENT change, I don't see
how (!x) is false and (opt.error) is true, and so
"if (!x || opts.error)" is always equivalent to "if (!x)".

What am I missing?
Eyal.
Daniel Xu Dec. 11, 2023, 11:49 p.m. UTC | #4 
On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > Hi Daniel,
> > >
> > > Tiny nits below in case you respin this for other reasons:
> > >
> > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > >
> > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > lookup kfunc.
> > > >
> > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > ---
> > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > >         SYS(fail,
> > > >             "ip netns exec at_ns0 "
> > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > -                       "spi %d reqid 1 mode tunnel "
> > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > >         SYS(fail,
> > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > >         SYS(fail,
> > > >             "ip netns exec at_ns0 "
> > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > -                       "spi %d reqid 2 mode tunnel "
> > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > >
> > > nit: why do you need to set the replay-window in both directions?
> >
> > No reason - probably just careless here.
> >
> > >
> > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > >         SYS(fail,
> > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > >          */
> > > >         SYS(fail,
> > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > -                   "spi %d reqid 1 mode tunnel "
> > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > >         SYS(fail,
> > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > >         /* root -> at_ns0 */
> > > >         SYS(fail,
> > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > -                   "spi %d reqid 2 mode tunnel "
> > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > >         SYS(fail,
> > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > >  {
> > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > >                             .attach_point = BPF_TC_INGRESS);
> > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > >         struct test_tunnel_kern *skel = NULL;
> > > >         struct nstoken *nstoken;
> > > > +       int xdp_prog_fd;
> > > >         int tc_prog_fd;
> > > >         int ifindex;
> > > >         int err;
> > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > >                 goto done;
> > > >
> > > > +       /* attach xdp prog to tunnel dev */
> > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > +               goto done;
> > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > +               goto done;
> > > > +
> > > >         /* ping from at_ns0 namespace test */
> > > >         nstoken = open_netns("at_ns0");
> > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > >                 goto done;
> > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > >                 goto done;
> > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > +               goto done;
> > > >
> > > >  done:
> > > >         delete_xfrm_tunnel();
> > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > +struct xfrm_state *
> > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > +                      u32 opts__sz) __ksym;
> > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > >
> > > >  struct {
> > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > >         return TC_ACT_OK;
> > > >  }
> > > >
> > > > +volatile int xfrm_replay_window = 0;
> > > > +
> > > > +SEC("xdp")
> > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > +{
> > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > +       struct xfrm_state *x = NULL;
> > > > +       struct ip_esp_hdr *esph;
> > > > +       struct bpf_dynptr ptr;
> > > > +       u8 esph_buf[8] = {};
> > > > +       u8 iph_buf[20] = {};
> > > > +       struct iphdr *iph;
> > > > +       u32 off;
> > > > +
> > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > +               goto out;
> > > > +
> > > > +       off = sizeof(struct ethhdr);
> > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > +               goto out;
> > > > +
> > > > +       off += sizeof(struct iphdr);
> > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > +       if (!esph)
> > > > +               goto out;
> > > > +
> > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > +       opts.daddr.a4 = iph->daddr;
> > > > +       opts.spi = esph->spi;
> > > > +       opts.proto = IPPROTO_ESP;
> > > > +       opts.family = AF_INET;
> > > > +
> > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > +       if (!x || opts.error)
> > >
> > > nit: how can opts.error be non zero if x == NULL?
> >
> > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > behavior here.
> 
> I'm sorry, I don't understand.
> 
> AFAICT, regardless of the -ENOENT change, I don't see
> how (!x) is false and (opt.error) is true, and so
> "if (!x || opts.error)" is always equivalent to "if (!x)".
> 
> What am I missing?
> Eyal.

The selftests are tests so my intention was to check edge cases here.
In normal operation it shouldn't be possible that
bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
another way of writing this would be:

        if (!x)
                goto out;
        assert(opts.error == 0);

If I'm trying to be too clever (or maybe just wrong) or it's pointless,
I can remove the `opts.error` condition.

Thanks,
Daniel
Eyal Birger Dec. 12, 2023, 12:25 a.m. UTC | #5 
On Mon, Dec 11, 2023 at 3:49 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> > On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > >
> > > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > > Hi Daniel,
> > > >
> > > > Tiny nits below in case you respin this for other reasons:
> > > >
> > > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > >
> > > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > > lookup kfunc.
> > > > >
> > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > > ---
> > > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > > >         SYS(fail,
> > > > >             "ip netns exec at_ns0 "
> > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > -                       "spi %d reqid 1 mode tunnel "
> > > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > >         SYS(fail,
> > > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > > >         SYS(fail,
> > > > >             "ip netns exec at_ns0 "
> > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > -                       "spi %d reqid 2 mode tunnel "
> > > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > > >
> > > > nit: why do you need to set the replay-window in both directions?
> > >
> > > No reason - probably just careless here.
> > >
> > > >
> > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > >         SYS(fail,
> > > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > > >          */
> > > > >         SYS(fail,
> > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > -                   "spi %d reqid 1 mode tunnel "
> > > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > >         SYS(fail,
> > > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > > >         /* root -> at_ns0 */
> > > > >         SYS(fail,
> > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > -                   "spi %d reqid 2 mode tunnel "
> > > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > >         SYS(fail,
> > > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > > >  {
> > > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > > >                             .attach_point = BPF_TC_INGRESS);
> > > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > > >         struct test_tunnel_kern *skel = NULL;
> > > > >         struct nstoken *nstoken;
> > > > > +       int xdp_prog_fd;
> > > > >         int tc_prog_fd;
> > > > >         int ifindex;
> > > > >         int err;
> > > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > > >                 goto done;
> > > > >
> > > > > +       /* attach xdp prog to tunnel dev */
> > > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > > +               goto done;
> > > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > > +               goto done;
> > > > > +
> > > > >         /* ping from at_ns0 namespace test */
> > > > >         nstoken = open_netns("at_ns0");
> > > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > > >                 goto done;
> > > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > > >                 goto done;
> > > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > > +               goto done;
> > > > >
> > > > >  done:
> > > > >         delete_xfrm_tunnel();
> > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > > +struct xfrm_state *
> > > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > > +                      u32 opts__sz) __ksym;
> > > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > > >
> > > > >  struct {
> > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > > >         return TC_ACT_OK;
> > > > >  }
> > > > >
> > > > > +volatile int xfrm_replay_window = 0;
> > > > > +
> > > > > +SEC("xdp")
> > > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > +{
> > > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > > +       struct xfrm_state *x = NULL;
> > > > > +       struct ip_esp_hdr *esph;
> > > > > +       struct bpf_dynptr ptr;
> > > > > +       u8 esph_buf[8] = {};
> > > > > +       u8 iph_buf[20] = {};
> > > > > +       struct iphdr *iph;
> > > > > +       u32 off;
> > > > > +
> > > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > > +               goto out;
> > > > > +
> > > > > +       off = sizeof(struct ethhdr);
> > > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > > +               goto out;
> > > > > +
> > > > > +       off += sizeof(struct iphdr);
> > > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > > +       if (!esph)
> > > > > +               goto out;
> > > > > +
> > > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > > +       opts.daddr.a4 = iph->daddr;
> > > > > +       opts.spi = esph->spi;
> > > > > +       opts.proto = IPPROTO_ESP;
> > > > > +       opts.family = AF_INET;
> > > > > +
> > > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > +       if (!x || opts.error)
> > > >
> > > > nit: how can opts.error be non zero if x == NULL?
> > >
> > > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > > behavior here.
> >
> > I'm sorry, I don't understand.
> >
> > AFAICT, regardless of the -ENOENT change, I don't see
> > how (!x) is false and (opt.error) is true, and so
> > "if (!x || opts.error)" is always equivalent to "if (!x)".
> >
> > What am I missing?
> > Eyal.
>
> The selftests are tests so my intention was to check edge cases here.
> In normal operation it shouldn't be possible that
> bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
> another way of writing this would be:
>
>         if (!x)
>                 goto out;
>         assert(opts.error == 0);

I think this would convey the "edge case testing" notion better.

>
> If I'm trying to be too clever (or maybe just wrong) or it's pointless,
> I can remove the `opts.error` condition.

At least for me the tests also serve as references as to how the
API is expected to be used, so I think it'd be clearer without
signaling that opts.error could potentially be nonzero on success.

An assertion would indeed make that clear.

Thanks for the explanation,
Eyal.
Daniel Xu Dec. 12, 2023, 4:17 p.m. UTC | #6 
On Mon, Dec 11, 2023 at 04:25:06PM -0800, Eyal Birger wrote:
> On Mon, Dec 11, 2023 at 3:49 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> > > On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > >
> > > > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > > > Hi Daniel,
> > > > >
> > > > > Tiny nits below in case you respin this for other reasons:
> > > > >
> > > > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > >
> > > > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > > > lookup kfunc.
> > > > > >
> > > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > > > ---
> > > > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > > > >         SYS(fail,
> > > > > >             "ip netns exec at_ns0 "
> > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > -                       "spi %d reqid 1 mode tunnel "
> > > > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > >         SYS(fail,
> > > > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > > > >         SYS(fail,
> > > > > >             "ip netns exec at_ns0 "
> > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > -                       "spi %d reqid 2 mode tunnel "
> > > > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > >
> > > > > nit: why do you need to set the replay-window in both directions?
> > > >
> > > > No reason - probably just careless here.
> > > >
> > > > >
> > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > >         SYS(fail,
> > > > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > > > >          */
> > > > > >         SYS(fail,
> > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > -                   "spi %d reqid 1 mode tunnel "
> > > > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > >         SYS(fail,
> > > > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > > > >         /* root -> at_ns0 */
> > > > > >         SYS(fail,
> > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > -                   "spi %d reqid 2 mode tunnel "
> > > > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > >         SYS(fail,
> > > > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > > > >  {
> > > > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > > > >                             .attach_point = BPF_TC_INGRESS);
> > > > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > > > >         struct test_tunnel_kern *skel = NULL;
> > > > > >         struct nstoken *nstoken;
> > > > > > +       int xdp_prog_fd;
> > > > > >         int tc_prog_fd;
> > > > > >         int ifindex;
> > > > > >         int err;
> > > > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > > > >                 goto done;
> > > > > >
> > > > > > +       /* attach xdp prog to tunnel dev */
> > > > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > > > +               goto done;
> > > > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > > > +               goto done;
> > > > > > +
> > > > > >         /* ping from at_ns0 namespace test */
> > > > > >         nstoken = open_netns("at_ns0");
> > > > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > > > >                 goto done;
> > > > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > > > >                 goto done;
> > > > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > > > +               goto done;
> > > > > >
> > > > > >  done:
> > > > > >         delete_xfrm_tunnel();
> > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > > > +struct xfrm_state *
> > > > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > > > +                      u32 opts__sz) __ksym;
> > > > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > > > >
> > > > > >  struct {
> > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > > > >         return TC_ACT_OK;
> > > > > >  }
> > > > > >
> > > > > > +volatile int xfrm_replay_window = 0;
> > > > > > +
> > > > > > +SEC("xdp")
> > > > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > > +{
> > > > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > > > +       struct xfrm_state *x = NULL;
> > > > > > +       struct ip_esp_hdr *esph;
> > > > > > +       struct bpf_dynptr ptr;
> > > > > > +       u8 esph_buf[8] = {};
> > > > > > +       u8 iph_buf[20] = {};
> > > > > > +       struct iphdr *iph;
> > > > > > +       u32 off;
> > > > > > +
> > > > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > > > +               goto out;
> > > > > > +
> > > > > > +       off = sizeof(struct ethhdr);
> > > > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > > > +               goto out;
> > > > > > +
> > > > > > +       off += sizeof(struct iphdr);
> > > > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > > > +       if (!esph)
> > > > > > +               goto out;
> > > > > > +
> > > > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > > > +       opts.daddr.a4 = iph->daddr;
> > > > > > +       opts.spi = esph->spi;
> > > > > > +       opts.proto = IPPROTO_ESP;
> > > > > > +       opts.family = AF_INET;
> > > > > > +
> > > > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > +       if (!x || opts.error)
> > > > >
> > > > > nit: how can opts.error be non zero if x == NULL?
> > > >
> > > > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > > > behavior here.
> > >
> > > I'm sorry, I don't understand.
> > >
> > > AFAICT, regardless of the -ENOENT change, I don't see
> > > how (!x) is false and (opt.error) is true, and so
> > > "if (!x || opts.error)" is always equivalent to "if (!x)".
> > >
> > > What am I missing?
> > > Eyal.
> >
> > The selftests are tests so my intention was to check edge cases here.
> > In normal operation it shouldn't be possible that
> > bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
> > another way of writing this would be:
> >
> >         if (!x)
> >                 goto out;
> >         assert(opts.error == 0);
> 
> I think this would convey the "edge case testing" notion better.
> 
> >
> > If I'm trying to be too clever (or maybe just wrong) or it's pointless,
> > I can remove the `opts.error` condition.
> 
> At least for me the tests also serve as references as to how the
> API is expected to be used, so I think it'd be clearer without
> signaling that opts.error could potentially be nonzero on success.
> 
> An assertion would indeed make that clear.

Sure, sounds good. I will check on the new bpf assert infra.

> 
> Thanks for the explanation,
> Eyal.

Np!

If you don't mind (and there no more comments), I would prefer to send a
follow up fixing the nits in this revision. So that I stop blasting the
list (as well as people who may not be as concerned with these details).

Thanks,
Daniel
Alexei Starovoitov Dec. 12, 2023, 4:44 p.m. UTC | #7 
On Tue, Dec 12, 2023 at 8:17 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
>
> If you don't mind (and there no more comments), I would prefer to send a
> follow up fixing the nits in this revision. So that I stop blasting the
> list (as well as people who may not be as concerned with these details).

Resending patches is little effort while follow up patches
double the commits, more code churn, increase in code reviews, etc.
Always address feedback by resending.
Daniel Xu Dec. 12, 2023, 7 p.m. UTC | #8 
On Tue, Dec 12, 2023 at 08:44:42AM -0800, Alexei Starovoitov wrote:
> On Tue, Dec 12, 2023 at 8:17 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> >
> > If you don't mind (and there no more comments), I would prefer to send a
> > follow up fixing the nits in this revision. So that I stop blasting the
> > list (as well as people who may not be as concerned with these details).
> 
> Resending patches is little effort while follow up patches
> double the commits, more code churn, increase in code reviews, etc.
> Always address feedback by resending.

Got it; will keep that in mind.

Thanks,
Daniel
Daniel Xu Dec. 12, 2023, 7:52 p.m. UTC | #9 
cc Kumar

On Tue, Dec 12, 2023 at 09:17:02AM -0700, Daniel Xu wrote:
> On Mon, Dec 11, 2023 at 04:25:06PM -0800, Eyal Birger wrote:
> > On Mon, Dec 11, 2023 at 3:49 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > >
> > > On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> > > > On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > >
> > > > > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > > > > Hi Daniel,
> > > > > >
> > > > > > Tiny nits below in case you respin this for other reasons:
> > > > > >
> > > > > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > >
> > > > > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > > > > lookup kfunc.
> > > > > > >
> > > > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > > > > ---
> > > > > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > > > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > > > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > > > > >         SYS(fail,
> > > > > > >             "ip netns exec at_ns0 "
> > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > -                       "spi %d reqid 1 mode tunnel "
> > > > > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > >         SYS(fail,
> > > > > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > > > > >         SYS(fail,
> > > > > > >             "ip netns exec at_ns0 "
> > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > -                       "spi %d reqid 2 mode tunnel "
> > > > > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > >
> > > > > > nit: why do you need to set the replay-window in both directions?
> > > > >
> > > > > No reason - probably just careless here.
> > > > >
> > > > > >
> > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > >         SYS(fail,
> > > > > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > > > > >          */
> > > > > > >         SYS(fail,
> > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > -                   "spi %d reqid 1 mode tunnel "
> > > > > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > >         SYS(fail,
> > > > > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > > > > >         /* root -> at_ns0 */
> > > > > > >         SYS(fail,
> > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > -                   "spi %d reqid 2 mode tunnel "
> > > > > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > >         SYS(fail,
> > > > > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > > > > >  {
> > > > > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > > > > >                             .attach_point = BPF_TC_INGRESS);
> > > > > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > > > > >         struct test_tunnel_kern *skel = NULL;
> > > > > > >         struct nstoken *nstoken;
> > > > > > > +       int xdp_prog_fd;
> > > > > > >         int tc_prog_fd;
> > > > > > >         int ifindex;
> > > > > > >         int err;
> > > > > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > > > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > > > > >                 goto done;
> > > > > > >
> > > > > > > +       /* attach xdp prog to tunnel dev */
> > > > > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > > > > +               goto done;
> > > > > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > > > > +               goto done;
> > > > > > > +
> > > > > > >         /* ping from at_ns0 namespace test */
> > > > > > >         nstoken = open_netns("at_ns0");
> > > > > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > > > > >                 goto done;
> > > > > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > > > > >                 goto done;
> > > > > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > > > > +               goto done;
> > > > > > >
> > > > > > >  done:
> > > > > > >         delete_xfrm_tunnel();
> > > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > > > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > > > > +struct xfrm_state *
> > > > > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > > > > +                      u32 opts__sz) __ksym;
> > > > > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > > > > >
> > > > > > >  struct {
> > > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > > > > >         return TC_ACT_OK;
> > > > > > >  }
> > > > > > >
> > > > > > > +volatile int xfrm_replay_window = 0;
> > > > > > > +
> > > > > > > +SEC("xdp")
> > > > > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > > > +{
> > > > > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > > > > +       struct xfrm_state *x = NULL;
> > > > > > > +       struct ip_esp_hdr *esph;
> > > > > > > +       struct bpf_dynptr ptr;
> > > > > > > +       u8 esph_buf[8] = {};
> > > > > > > +       u8 iph_buf[20] = {};
> > > > > > > +       struct iphdr *iph;
> > > > > > > +       u32 off;
> > > > > > > +
> > > > > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > > > > +               goto out;
> > > > > > > +
> > > > > > > +       off = sizeof(struct ethhdr);
> > > > > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > > > > +               goto out;
> > > > > > > +
> > > > > > > +       off += sizeof(struct iphdr);
> > > > > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > > > > +       if (!esph)
> > > > > > > +               goto out;
> > > > > > > +
> > > > > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > > > > +       opts.daddr.a4 = iph->daddr;
> > > > > > > +       opts.spi = esph->spi;
> > > > > > > +       opts.proto = IPPROTO_ESP;
> > > > > > > +       opts.family = AF_INET;
> > > > > > > +
> > > > > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > > +       if (!x || opts.error)
> > > > > >
> > > > > > nit: how can opts.error be non zero if x == NULL?
> > > > >
> > > > > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > > > > behavior here.
> > > >
> > > > I'm sorry, I don't understand.
> > > >
> > > > AFAICT, regardless of the -ENOENT change, I don't see
> > > > how (!x) is false and (opt.error) is true, and so
> > > > "if (!x || opts.error)" is always equivalent to "if (!x)".
> > > >
> > > > What am I missing?
> > > > Eyal.
> > >
> > > The selftests are tests so my intention was to check edge cases here.
> > > In normal operation it shouldn't be possible that
> > > bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
> > > another way of writing this would be:
> > >
> > >         if (!x)
> > >                 goto out;
> > >         assert(opts.error == 0);
> > 
> > I think this would convey the "edge case testing" notion better.
> > 
> > >
> > > If I'm trying to be too clever (or maybe just wrong) or it's pointless,
> > > I can remove the `opts.error` condition.
> > 
> > At least for me the tests also serve as references as to how the
> > API is expected to be used, so I think it'd be clearer without
> > signaling that opts.error could potentially be nonzero on success.
> > 
> > An assertion would indeed make that clear.
> 
> Sure, sounds good. I will check on the new bpf assert infra.

Couldn't quite get bpf_assert() working. The following diff:

diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
index c0dd38616562..f00dba85ac5d 100644
--- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
@@ -8,8 +8,9 @@
  */
 #include "vmlinux.h"
 #include <bpf/bpf_core_read.h>
-#include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_experimental.h"
 #include "bpf_kfuncs.h"
 #include "bpf_tracing_net.h"

@@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
        opts.family = AF_INET;

        x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
-       if (!x || opts.error)
+       if (!x)
                goto out;
+       bpf_assert_with(opts.error == 0, XDP_PASS);

        if (!x->replay_esn)
                goto out;

results in:

57: (b7) r1 = 2                       ; R1_w=2 refs=5
58: (85) call bpf_throw#115436
calling kernel function bpf_throw is not allowed

It looks like the above error comes from verifier.c:fetch_kfunc_meta,
but I can run the exceptions selftests just fine with the same bzImage.
So I'm thinking it's not a kfunc registration or BTF issue.

Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.

So for now I think I'll drop checking opts.error.

[...]
Kumar Kartikeya Dwivedi Dec. 12, 2023, 11:13 p.m. UTC | #10 
On Tue, 12 Dec 2023 at 20:52, Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> cc Kumar
>
> On Tue, Dec 12, 2023 at 09:17:02AM -0700, Daniel Xu wrote:
> > On Mon, Dec 11, 2023 at 04:25:06PM -0800, Eyal Birger wrote:
> > > On Mon, Dec 11, 2023 at 3:49 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > >
> > > > On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> > > > > On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > >
> > > > > > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > > > > > Hi Daniel,
> > > > > > >
> > > > > > > Tiny nits below in case you respin this for other reasons:
> > > > > > >
> > > > > > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > > >
> > > > > > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > > > > > lookup kfunc.
> > > > > > > >
> > > > > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > > > > > ---
> > > > > > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > > > > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > > > > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > > > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > >         SYS(fail,
> > > > > > > >             "ip netns exec at_ns0 "
> > > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > -                       "spi %d reqid 1 mode tunnel "
> > > > > > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > > >         SYS(fail,
> > > > > > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > >         SYS(fail,
> > > > > > > >             "ip netns exec at_ns0 "
> > > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > -                       "spi %d reqid 2 mode tunnel "
> > > > > > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > >
> > > > > > > nit: why do you need to set the replay-window in both directions?
> > > > > >
> > > > > > No reason - probably just careless here.
> > > > > >
> > > > > > >
> > > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > > >         SYS(fail,
> > > > > > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > >          */
> > > > > > > >         SYS(fail,
> > > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > -                   "spi %d reqid 1 mode tunnel "
> > > > > > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > > >         SYS(fail,
> > > > > > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > >         /* root -> at_ns0 */
> > > > > > > >         SYS(fail,
> > > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > -                   "spi %d reqid 2 mode tunnel "
> > > > > > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > > >         SYS(fail,
> > > > > > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > > > > > >  {
> > > > > > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > > > > > >                             .attach_point = BPF_TC_INGRESS);
> > > > > > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > > > > > >         struct test_tunnel_kern *skel = NULL;
> > > > > > > >         struct nstoken *nstoken;
> > > > > > > > +       int xdp_prog_fd;
> > > > > > > >         int tc_prog_fd;
> > > > > > > >         int ifindex;
> > > > > > > >         int err;
> > > > > > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > > > > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > > > > > >                 goto done;
> > > > > > > >
> > > > > > > > +       /* attach xdp prog to tunnel dev */
> > > > > > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > > > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > > > > > +               goto done;
> > > > > > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > > > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > > > > > +               goto done;
> > > > > > > > +
> > > > > > > >         /* ping from at_ns0 namespace test */
> > > > > > > >         nstoken = open_netns("at_ns0");
> > > > > > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > > > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > > > > > >                 goto done;
> > > > > > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > > > > > >                 goto done;
> > > > > > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > > > > > +               goto done;
> > > > > > > >
> > > > > > > >  done:
> > > > > > > >         delete_xfrm_tunnel();
> > > > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > > > > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > > > > > +struct xfrm_state *
> > > > > > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > > > > > +                      u32 opts__sz) __ksym;
> > > > > > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > > > > > >
> > > > > > > >  struct {
> > > > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > > > > > >         return TC_ACT_OK;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +volatile int xfrm_replay_window = 0;
> > > > > > > > +
> > > > > > > > +SEC("xdp")
> > > > > > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > > > > +{
> > > > > > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > > > > > +       struct xfrm_state *x = NULL;
> > > > > > > > +       struct ip_esp_hdr *esph;
> > > > > > > > +       struct bpf_dynptr ptr;
> > > > > > > > +       u8 esph_buf[8] = {};
> > > > > > > > +       u8 iph_buf[20] = {};
> > > > > > > > +       struct iphdr *iph;
> > > > > > > > +       u32 off;
> > > > > > > > +
> > > > > > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > > > > > +               goto out;
> > > > > > > > +
> > > > > > > > +       off = sizeof(struct ethhdr);
> > > > > > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > > > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > > > > > +               goto out;
> > > > > > > > +
> > > > > > > > +       off += sizeof(struct iphdr);
> > > > > > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > > > > > +       if (!esph)
> > > > > > > > +               goto out;
> > > > > > > > +
> > > > > > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > > > > > +       opts.daddr.a4 = iph->daddr;
> > > > > > > > +       opts.spi = esph->spi;
> > > > > > > > +       opts.proto = IPPROTO_ESP;
> > > > > > > > +       opts.family = AF_INET;
> > > > > > > > +
> > > > > > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > > > +       if (!x || opts.error)
> > > > > > >
> > > > > > > nit: how can opts.error be non zero if x == NULL?
> > > > > >
> > > > > > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > > > > > behavior here.
> > > > >
> > > > > I'm sorry, I don't understand.
> > > > >
> > > > > AFAICT, regardless of the -ENOENT change, I don't see
> > > > > how (!x) is false and (opt.error) is true, and so
> > > > > "if (!x || opts.error)" is always equivalent to "if (!x)".
> > > > >
> > > > > What am I missing?
> > > > > Eyal.
> > > >
> > > > The selftests are tests so my intention was to check edge cases here.
> > > > In normal operation it shouldn't be possible that
> > > > bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
> > > > another way of writing this would be:
> > > >
> > > >         if (!x)
> > > >                 goto out;
> > > >         assert(opts.error == 0);
> > >
> > > I think this would convey the "edge case testing" notion better.
> > >
> > > >
> > > > If I'm trying to be too clever (or maybe just wrong) or it's pointless,
> > > > I can remove the `opts.error` condition.
> > >
> > > At least for me the tests also serve as references as to how the
> > > API is expected to be used, so I think it'd be clearer without
> > > signaling that opts.error could potentially be nonzero on success.
> > >
> > > An assertion would indeed make that clear.
> >
> > Sure, sounds good. I will check on the new bpf assert infra.
>
> Couldn't quite get bpf_assert() working. The following diff:
>
> diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> index c0dd38616562..f00dba85ac5d 100644
> --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> @@ -8,8 +8,9 @@
>   */
>  #include "vmlinux.h"
>  #include <bpf/bpf_core_read.h>
> -#include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_endian.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_experimental.h"
>  #include "bpf_kfuncs.h"
>  #include "bpf_tracing_net.h"
>
> @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
>         opts.family = AF_INET;
>
>         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> -       if (!x || opts.error)
> +       if (!x)
>                 goto out;
> +       bpf_assert_with(opts.error == 0, XDP_PASS);
>
>         if (!x->replay_esn)
>                 goto out;
>
> results in:
>
> 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> 58: (85) call bpf_throw#115436
> calling kernel function bpf_throw is not allowed
>

I think this might be because bpf_throw is not registered for use by
BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
this program type as well, since it's already done for TC.

> It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> but I can run the exceptions selftests just fine with the same bzImage.
> So I'm thinking it's not a kfunc registration or BTF issue.
>
> Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
>

Yes, even once you enable this, this will fail for now. I am sending
out a series later this week that enables bpf_throw with acquired
references, but until then may I suggest the following:

#define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
&& !___j; bpf_throw(), ___i++)

This will allow you to insert some cleanup code with an assertion.
Then in my series, I will convert this temporary bpf_assert_if back to
the normal bpf_assert.

It would look like:
bpf_assert_if(opts.error == 0) {
  // Execute if assertion failed
  bpf_xdp_xfrm_state_release(x);
}

Likewise for bpf_assert_with_if, you get the idea.



> So for now I think I'll drop checking opts.error.
>
> [...]
Daniel Xu Dec. 13, 2023, 11:15 p.m. UTC | #11 
On Wed, Dec 13, 2023 at 12:13:51AM +0100, Kumar Kartikeya Dwivedi wrote:
> On Tue, 12 Dec 2023 at 20:52, Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > cc Kumar
> >
> > On Tue, Dec 12, 2023 at 09:17:02AM -0700, Daniel Xu wrote:
> > > On Mon, Dec 11, 2023 at 04:25:06PM -0800, Eyal Birger wrote:
> > > > On Mon, Dec 11, 2023 at 3:49 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > >
> > > > > On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> > > > > > On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > > > > > > Hi Daniel,
> > > > > > > >
> > > > > > > > Tiny nits below in case you respin this for other reasons:
> > > > > > > >
> > > > > > > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > > > >
> > > > > > > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > > > > > > lookup kfunc.
> > > > > > > > >
> > > > > > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > > > > > > ---
> > > > > > > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > > > > > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > > > > > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > > > > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > >         SYS(fail,
> > > > > > > > >             "ip netns exec at_ns0 "
> > > > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > -                       "spi %d reqid 1 mode tunnel "
> > > > > > > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > > > >         SYS(fail,
> > > > > > > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > >         SYS(fail,
> > > > > > > > >             "ip netns exec at_ns0 "
> > > > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > -                       "spi %d reqid 2 mode tunnel "
> > > > > > > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > > >
> > > > > > > > nit: why do you need to set the replay-window in both directions?
> > > > > > >
> > > > > > > No reason - probably just careless here.
> > > > > > >
> > > > > > > >
> > > > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > > > >         SYS(fail,
> > > > > > > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > >          */
> > > > > > > > >         SYS(fail,
> > > > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > -                   "spi %d reqid 1 mode tunnel "
> > > > > > > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > > > >         SYS(fail,
> > > > > > > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > >         /* root -> at_ns0 */
> > > > > > > > >         SYS(fail,
> > > > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > -                   "spi %d reqid 2 mode tunnel "
> > > > > > > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > > > >         SYS(fail,
> > > > > > > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > > > > > > >  {
> > > > > > > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > > > > > > >                             .attach_point = BPF_TC_INGRESS);
> > > > > > > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > > > > > > >         struct test_tunnel_kern *skel = NULL;
> > > > > > > > >         struct nstoken *nstoken;
> > > > > > > > > +       int xdp_prog_fd;
> > > > > > > > >         int tc_prog_fd;
> > > > > > > > >         int ifindex;
> > > > > > > > >         int err;
> > > > > > > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > > > > > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > > > > > > >                 goto done;
> > > > > > > > >
> > > > > > > > > +       /* attach xdp prog to tunnel dev */
> > > > > > > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > > > > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > > > > > > +               goto done;
> > > > > > > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > > > > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > > > > > > +               goto done;
> > > > > > > > > +
> > > > > > > > >         /* ping from at_ns0 namespace test */
> > > > > > > > >         nstoken = open_netns("at_ns0");
> > > > > > > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > > > > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > > > > > > >                 goto done;
> > > > > > > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > > > > > > >                 goto done;
> > > > > > > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > > > > > > +               goto done;
> > > > > > > > >
> > > > > > > > >  done:
> > > > > > > > >         delete_xfrm_tunnel();
> > > > > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > > > > > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > > > > > > +struct xfrm_state *
> > > > > > > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > > > > > > +                      u32 opts__sz) __ksym;
> > > > > > > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > > > > > > >
> > > > > > > > >  struct {
> > > > > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > > > > > > >         return TC_ACT_OK;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +volatile int xfrm_replay_window = 0;
> > > > > > > > > +
> > > > > > > > > +SEC("xdp")
> > > > > > > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > > > > > +{
> > > > > > > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > > > > > > +       struct xfrm_state *x = NULL;
> > > > > > > > > +       struct ip_esp_hdr *esph;
> > > > > > > > > +       struct bpf_dynptr ptr;
> > > > > > > > > +       u8 esph_buf[8] = {};
> > > > > > > > > +       u8 iph_buf[20] = {};
> > > > > > > > > +       struct iphdr *iph;
> > > > > > > > > +       u32 off;
> > > > > > > > > +
> > > > > > > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > > > > > > +               goto out;
> > > > > > > > > +
> > > > > > > > > +       off = sizeof(struct ethhdr);
> > > > > > > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > > > > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > > > > > > +               goto out;
> > > > > > > > > +
> > > > > > > > > +       off += sizeof(struct iphdr);
> > > > > > > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > > > > > > +       if (!esph)
> > > > > > > > > +               goto out;
> > > > > > > > > +
> > > > > > > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > > > > > > +       opts.daddr.a4 = iph->daddr;
> > > > > > > > > +       opts.spi = esph->spi;
> > > > > > > > > +       opts.proto = IPPROTO_ESP;
> > > > > > > > > +       opts.family = AF_INET;
> > > > > > > > > +
> > > > > > > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > > > > +       if (!x || opts.error)
> > > > > > > >
> > > > > > > > nit: how can opts.error be non zero if x == NULL?
> > > > > > >
> > > > > > > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > > > > > > behavior here.
> > > > > >
> > > > > > I'm sorry, I don't understand.
> > > > > >
> > > > > > AFAICT, regardless of the -ENOENT change, I don't see
> > > > > > how (!x) is false and (opt.error) is true, and so
> > > > > > "if (!x || opts.error)" is always equivalent to "if (!x)".
> > > > > >
> > > > > > What am I missing?
> > > > > > Eyal.
> > > > >
> > > > > The selftests are tests so my intention was to check edge cases here.
> > > > > In normal operation it shouldn't be possible that
> > > > > bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
> > > > > another way of writing this would be:
> > > > >
> > > > >         if (!x)
> > > > >                 goto out;
> > > > >         assert(opts.error == 0);
> > > >
> > > > I think this would convey the "edge case testing" notion better.
> > > >
> > > > >
> > > > > If I'm trying to be too clever (or maybe just wrong) or it's pointless,
> > > > > I can remove the `opts.error` condition.
> > > >
> > > > At least for me the tests also serve as references as to how the
> > > > API is expected to be used, so I think it'd be clearer without
> > > > signaling that opts.error could potentially be nonzero on success.
> > > >
> > > > An assertion would indeed make that clear.
> > >
> > > Sure, sounds good. I will check on the new bpf assert infra.
> >
> > Couldn't quite get bpf_assert() working. The following diff:
> >
> > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > index c0dd38616562..f00dba85ac5d 100644
> > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > @@ -8,8 +8,9 @@
> >   */
> >  #include "vmlinux.h"
> >  #include <bpf/bpf_core_read.h>
> > -#include <bpf/bpf_helpers.h>
> >  #include <bpf/bpf_endian.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_experimental.h"
> >  #include "bpf_kfuncs.h"
> >  #include "bpf_tracing_net.h"
> >
> > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> >         opts.family = AF_INET;
> >
> >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > -       if (!x || opts.error)
> > +       if (!x)
> >                 goto out;
> > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> >
> >         if (!x->replay_esn)
> >                 goto out;
> >
> > results in:
> >
> > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > 58: (85) call bpf_throw#115436
> > calling kernel function bpf_throw is not allowed
> >
> 
> I think this might be because bpf_throw is not registered for use by
> BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> this program type as well, since it's already done for TC.

Ah yeah, that was it.

> 
> > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > but I can run the exceptions selftests just fine with the same bzImage.
> > So I'm thinking it's not a kfunc registration or BTF issue.
> >
> > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> >
> 
> Yes, even once you enable this, this will fail for now. I am sending
> out a series later this week that enables bpf_throw with acquired
> references, but until then may I suggest the following:
> 
> #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> && !___j; bpf_throw(), ___i++)
> 
> This will allow you to insert some cleanup code with an assertion.
> Then in my series, I will convert this temporary bpf_assert_if back to
> the normal bpf_assert.
> 
> It would look like:
> bpf_assert_if(opts.error == 0) {
>   // Execute if assertion failed
>   bpf_xdp_xfrm_state_release(x);
> }
> 
> Likewise for bpf_assert_with_if, you get the idea.

I gave it a try and I'm getting this compile error:

        progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
                bpf_assert_with_if(opts.error == 0, XDP_PASS) {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
                for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
                                                    ^~~~      ~~~~
        1 error generated.
        make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1

Seems like the compiler is being clever.

Thanks,
Daniel
Eyal Birger Dec. 13, 2023, 11:49 p.m. UTC | #12 
On Wed, Dec 13, 2023 at 3:15 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> On Wed, Dec 13, 2023 at 12:13:51AM +0100, Kumar Kartikeya Dwivedi wrote:
> > On Tue, 12 Dec 2023 at 20:52, Daniel Xu <dxu@dxuuu.xyz> wrote:
> > >
> > > cc Kumar
> > >
> > > On Tue, Dec 12, 2023 at 09:17:02AM -0700, Daniel Xu wrote:
> > > > On Mon, Dec 11, 2023 at 04:25:06PM -0800, Eyal Birger wrote:
> > > > > On Mon, Dec 11, 2023 at 3:49 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > >
> > > > > > On Mon, Dec 11, 2023 at 03:13:07PM -0800, Eyal Birger wrote:
> > > > > > > On Mon, Dec 11, 2023 at 2:31 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > > >
> > > > > > > > On Mon, Dec 11, 2023 at 01:39:25PM -0800, Eyal Birger wrote:
> > > > > > > > > Hi Daniel,
> > > > > > > > >
> > > > > > > > > Tiny nits below in case you respin this for other reasons:
> > > > > > > > >
> > > > > > > > > On Mon, Dec 11, 2023 at 12:20 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > > > > >
> > > > > > > > > > This commit extends test_tunnel selftest to test the new XDP xfrm state
> > > > > > > > > > lookup kfunc.
> > > > > > > > > >
> > > > > > > > > > Co-developed-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > > > > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > > > > > > > ---
> > > > > > > > > >  .../selftests/bpf/prog_tests/test_tunnel.c    | 20 ++++++--
> > > > > > > > > >  .../selftests/bpf/progs/test_tunnel_kern.c    | 51 +++++++++++++++++++
> > > > > > > > > >  2 files changed, 67 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > > > index 2d7f8fa82ebd..fc804095d578 100644
> > > > > > > > > > --- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
> > > > > > > > > > @@ -278,7 +278,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > > >         SYS(fail,
> > > > > > > > > >             "ip netns exec at_ns0 "
> > > > > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > > -                       "spi %d reqid 1 mode tunnel "
> > > > > > > > > > +                       "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > > > > >         SYS(fail,
> > > > > > > > > > @@ -292,7 +292,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > > >         SYS(fail,
> > > > > > > > > >             "ip netns exec at_ns0 "
> > > > > > > > > >                 "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > > -                       "spi %d reqid 2 mode tunnel "
> > > > > > > > > > +                       "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > > > >
> > > > > > > > > nit: why do you need to set the replay-window in both directions?
> > > > > > > >
> > > > > > > > No reason - probably just careless here.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >                         "auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
> > > > > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > > > > >         SYS(fail,
> > > > > > > > > > @@ -313,7 +313,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > > >          */
> > > > > > > > > >         SYS(fail,
> > > > > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > > -                   "spi %d reqid 1 mode tunnel "
> > > > > > > > > > +                   "spi %d reqid 1 mode tunnel replay-window 42 "
> > > > > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > > > > >             IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
> > > > > > > > > >         SYS(fail,
> > > > > > > > > > @@ -325,7 +325,7 @@ static int add_xfrm_tunnel(void)
> > > > > > > > > >         /* root -> at_ns0 */
> > > > > > > > > >         SYS(fail,
> > > > > > > > > >             "ip xfrm state add src %s dst %s proto esp "
> > > > > > > > > > -                   "spi %d reqid 2 mode tunnel "
> > > > > > > > > > +                   "spi %d reqid 2 mode tunnel replay-window 42 "
> > > > > > > > > >                     "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
> > > > > > > > > >             IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
> > > > > > > > > >         SYS(fail,
> > > > > > > > > > @@ -628,8 +628,10 @@ static void test_xfrm_tunnel(void)
> > > > > > > > > >  {
> > > > > > > > > >         DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> > > > > > > > > >                             .attach_point = BPF_TC_INGRESS);
> > > > > > > > > > +       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > > > > > > > > >         struct test_tunnel_kern *skel = NULL;
> > > > > > > > > >         struct nstoken *nstoken;
> > > > > > > > > > +       int xdp_prog_fd;
> > > > > > > > > >         int tc_prog_fd;
> > > > > > > > > >         int ifindex;
> > > > > > > > > >         int err;
> > > > > > > > > > @@ -654,6 +656,14 @@ static void test_xfrm_tunnel(void)
> > > > > > > > > >         if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
> > > > > > > > > >                 goto done;
> > > > > > > > > >
> > > > > > > > > > +       /* attach xdp prog to tunnel dev */
> > > > > > > > > > +       xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
> > > > > > > > > > +       if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
> > > > > > > > > > +               goto done;
> > > > > > > > > > +       err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
> > > > > > > > > > +       if (!ASSERT_OK(err, "bpf_xdp_attach"))
> > > > > > > > > > +               goto done;
> > > > > > > > > > +
> > > > > > > > > >         /* ping from at_ns0 namespace test */
> > > > > > > > > >         nstoken = open_netns("at_ns0");
> > > > > > > > > >         err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
> > > > > > > > > > @@ -667,6 +677,8 @@ static void test_xfrm_tunnel(void)
> > > > > > > > > >                 goto done;
> > > > > > > > > >         if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
> > > > > > > > > >                 goto done;
> > > > > > > > > > +       if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
> > > > > > > > > > +               goto done;
> > > > > > > > > >
> > > > > > > > > >  done:
> > > > > > > > > >         delete_xfrm_tunnel();
> > > > > > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > > > index 3a59eb9c34de..c0dd38616562 100644
> > > > > > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > > > > @@ -30,6 +30,10 @@ int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > > > > >                           struct bpf_fou_encap *encap, int type) __ksym;
> > > > > > > > > >  int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
> > > > > > > > > >                           struct bpf_fou_encap *encap) __ksym;
> > > > > > > > > > +struct xfrm_state *
> > > > > > > > > > +bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
> > > > > > > > > > +                      u32 opts__sz) __ksym;
> > > > > > > > > > +void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
> > > > > > > > > >
> > > > > > > > > >  struct {
> > > > > > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > > > > > @@ -950,4 +954,51 @@ int xfrm_get_state(struct __sk_buff *skb)
> > > > > > > > > >         return TC_ACT_OK;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +volatile int xfrm_replay_window = 0;
> > > > > > > > > > +
> > > > > > > > > > +SEC("xdp")
> > > > > > > > > > +int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > > > > > > +{
> > > > > > > > > > +       struct bpf_xfrm_state_opts opts = {};
> > > > > > > > > > +       struct xfrm_state *x = NULL;
> > > > > > > > > > +       struct ip_esp_hdr *esph;
> > > > > > > > > > +       struct bpf_dynptr ptr;
> > > > > > > > > > +       u8 esph_buf[8] = {};
> > > > > > > > > > +       u8 iph_buf[20] = {};
> > > > > > > > > > +       struct iphdr *iph;
> > > > > > > > > > +       u32 off;
> > > > > > > > > > +
> > > > > > > > > > +       if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
> > > > > > > > > > +               goto out;
> > > > > > > > > > +
> > > > > > > > > > +       off = sizeof(struct ethhdr);
> > > > > > > > > > +       iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
> > > > > > > > > > +       if (!iph || iph->protocol != IPPROTO_ESP)
> > > > > > > > > > +               goto out;
> > > > > > > > > > +
> > > > > > > > > > +       off += sizeof(struct iphdr);
> > > > > > > > > > +       esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
> > > > > > > > > > +       if (!esph)
> > > > > > > > > > +               goto out;
> > > > > > > > > > +
> > > > > > > > > > +       opts.netns_id = BPF_F_CURRENT_NETNS;
> > > > > > > > > > +       opts.daddr.a4 = iph->daddr;
> > > > > > > > > > +       opts.spi = esph->spi;
> > > > > > > > > > +       opts.proto = IPPROTO_ESP;
> > > > > > > > > > +       opts.family = AF_INET;
> > > > > > > > > > +
> > > > > > > > > > +       x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > > > > > +       if (!x || opts.error)
> > > > > > > > >
> > > > > > > > > nit: how can opts.error be non zero if x == NULL?
> > > > > > > >
> > > > > > > > Ignoring the new -ENOENT case, it can't. Which is why I'm testing that
> > > > > > > > behavior here.
> > > > > > >
> > > > > > > I'm sorry, I don't understand.
> > > > > > >
> > > > > > > AFAICT, regardless of the -ENOENT change, I don't see
> > > > > > > how (!x) is false and (opt.error) is true, and so
> > > > > > > "if (!x || opts.error)" is always equivalent to "if (!x)".
> > > > > > >
> > > > > > > What am I missing?
> > > > > > > Eyal.
> > > > > >
> > > > > > The selftests are tests so my intention was to check edge cases here.
> > > > > > In normal operation it shouldn't be possible that
> > > > > > bpf_xdp_get_xfrm_state() returns non-NULL and also an error. Maybe
> > > > > > another way of writing this would be:
> > > > > >
> > > > > >         if (!x)
> > > > > >                 goto out;
> > > > > >         assert(opts.error == 0);
> > > > >
> > > > > I think this would convey the "edge case testing" notion better.
> > > > >
> > > > > >
> > > > > > If I'm trying to be too clever (or maybe just wrong) or it's pointless,
> > > > > > I can remove the `opts.error` condition.
> > > > >
> > > > > At least for me the tests also serve as references as to how the
> > > > > API is expected to be used, so I think it'd be clearer without
> > > > > signaling that opts.error could potentially be nonzero on success.
> > > > >
> > > > > An assertion would indeed make that clear.
> > > >
> > > > Sure, sounds good. I will check on the new bpf assert infra.
> > >
> > > Couldn't quite get bpf_assert() working. The following diff:
> > >
> > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > index c0dd38616562..f00dba85ac5d 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > @@ -8,8 +8,9 @@
> > >   */
> > >  #include "vmlinux.h"
> > >  #include <bpf/bpf_core_read.h>
> > > -#include <bpf/bpf_helpers.h>
> > >  #include <bpf/bpf_endian.h>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include "bpf_experimental.h"
> > >  #include "bpf_kfuncs.h"
> > >  #include "bpf_tracing_net.h"
> > >
> > > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> > >         opts.family = AF_INET;
> > >
> > >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > -       if (!x || opts.error)
> > > +       if (!x)
> > >                 goto out;
> > > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> > >
> > >         if (!x->replay_esn)
> > >                 goto out;
> > >
> > > results in:
> > >
> > > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > > 58: (85) call bpf_throw#115436
> > > calling kernel function bpf_throw is not allowed
> > >
> >
> > I think this might be because bpf_throw is not registered for use by
> > BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> > this program type as well, since it's already done for TC.
>
> Ah yeah, that was it.
>
> >
> > > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > > but I can run the exceptions selftests just fine with the same bzImage.
> > > So I'm thinking it's not a kfunc registration or BTF issue.
> > >
> > > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> > >
> >
> > Yes, even once you enable this, this will fail for now. I am sending
> > out a series later this week that enables bpf_throw with acquired
> > references, but until then may I suggest the following:
> >
> > #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> > && !___j; bpf_throw(), ___i++)
> >
> > This will allow you to insert some cleanup code with an assertion.
> > Then in my series, I will convert this temporary bpf_assert_if back to
> > the normal bpf_assert.
> >
> > It would look like:
> > bpf_assert_if(opts.error == 0) {
> >   // Execute if assertion failed
> >   bpf_xdp_xfrm_state_release(x);
> > }
> >
> > Likewise for bpf_assert_with_if, you get the idea.
>
> I gave it a try and I'm getting this compile error:
>
>         progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
>                 bpf_assert_with_if(opts.error == 0, XDP_PASS) {
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
>                 for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
>                                                     ^~~~      ~~~~
>         1 error generated.
>         make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1
>
> Seems like the compiler is being clever.

It looks like ___j is used twice - maybe it was meant to be ___i? i.e.:

   for (int ___i = 0, ___j = (cond); !(___j) && !___i; bpf_throw(value), ___i++)

Eyal.
Kumar Kartikeya Dwivedi Dec. 14, 2023, 4:08 p.m. UTC | #13 
On Thu, 14 Dec 2023 at 00:49, Eyal Birger <eyal.birger@gmail.com> wrote:
>
> On Wed, Dec 13, 2023 at 3:15 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > [...]
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > index c0dd38616562..f00dba85ac5d 100644
> > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > @@ -8,8 +8,9 @@
> > > >   */
> > > >  #include "vmlinux.h"
> > > >  #include <bpf/bpf_core_read.h>
> > > > -#include <bpf/bpf_helpers.h>
> > > >  #include <bpf/bpf_endian.h>
> > > > +#include <bpf/bpf_helpers.h>
> > > > +#include "bpf_experimental.h"
> > > >  #include "bpf_kfuncs.h"
> > > >  #include "bpf_tracing_net.h"
> > > >
> > > > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > >         opts.family = AF_INET;
> > > >
> > > >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > -       if (!x || opts.error)
> > > > +       if (!x)
> > > >                 goto out;
> > > > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> > > >
> > > >         if (!x->replay_esn)
> > > >                 goto out;
> > > >
> > > > results in:
> > > >
> > > > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > > > 58: (85) call bpf_throw#115436
> > > > calling kernel function bpf_throw is not allowed
> > > >
> > >
> > > I think this might be because bpf_throw is not registered for use by
> > > BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> > > this program type as well, since it's already done for TC.
> >
> > Ah yeah, that was it.
> >
> > >
> > > > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > > > but I can run the exceptions selftests just fine with the same bzImage.
> > > > So I'm thinking it's not a kfunc registration or BTF issue.
> > > >
> > > > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> > > >
> > >
> > > Yes, even once you enable this, this will fail for now. I am sending
> > > out a series later this week that enables bpf_throw with acquired
> > > references, but until then may I suggest the following:
> > >
> > > #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> > > && !___j; bpf_throw(), ___i++)
> > >
> > > This will allow you to insert some cleanup code with an assertion.
> > > Then in my series, I will convert this temporary bpf_assert_if back to
> > > the normal bpf_assert.
> > >
> > > It would look like:
> > > bpf_assert_if(opts.error == 0) {
> > >   // Execute if assertion failed
> > >   bpf_xdp_xfrm_state_release(x);
> > > }
> > >
> > > Likewise for bpf_assert_with_if, you get the idea.
> >
> > I gave it a try and I'm getting this compile error:
> >
> >         progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
> >                 bpf_assert_with_if(opts.error == 0, XDP_PASS) {
> >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >         /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
> >                 for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
> >                                                     ^~~~      ~~~~
> >         1 error generated.
> >         make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1
> >
> > Seems like the compiler is being clever.
>
> It looks like ___j is used twice - maybe it was meant to be ___i? i.e.:
>
>    for (int ___i = 0, ___j = (cond); !(___j) && !___i; bpf_throw(value), ___i++)
>

Ah, yes, that's a typo. Eyal is right, it should be ___i.
Kumar Kartikeya Dwivedi Dec. 14, 2023, 4:16 p.m. UTC | #14 
On Thu, 14 Dec 2023 at 17:08, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, 14 Dec 2023 at 00:49, Eyal Birger <eyal.birger@gmail.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 3:15 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > [...]
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > index c0dd38616562..f00dba85ac5d 100644
> > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > @@ -8,8 +8,9 @@
> > > > >   */
> > > > >  #include "vmlinux.h"
> > > > >  #include <bpf/bpf_core_read.h>
> > > > > -#include <bpf/bpf_helpers.h>
> > > > >  #include <bpf/bpf_endian.h>
> > > > > +#include <bpf/bpf_helpers.h>
> > > > > +#include "bpf_experimental.h"
> > > > >  #include "bpf_kfuncs.h"
> > > > >  #include "bpf_tracing_net.h"
> > > > >
> > > > > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > >         opts.family = AF_INET;
> > > > >
> > > > >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > -       if (!x || opts.error)
> > > > > +       if (!x)
> > > > >                 goto out;
> > > > > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> > > > >
> > > > >         if (!x->replay_esn)
> > > > >                 goto out;
> > > > >
> > > > > results in:
> > > > >
> > > > > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > > > > 58: (85) call bpf_throw#115436
> > > > > calling kernel function bpf_throw is not allowed
> > > > >
> > > >
> > > > I think this might be because bpf_throw is not registered for use by
> > > > BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> > > > this program type as well, since it's already done for TC.
> > >
> > > Ah yeah, that was it.
> > >
> > > >
> > > > > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > > > > but I can run the exceptions selftests just fine with the same bzImage.
> > > > > So I'm thinking it's not a kfunc registration or BTF issue.
> > > > >
> > > > > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> > > > >
> > > >
> > > > Yes, even once you enable this, this will fail for now. I am sending
> > > > out a series later this week that enables bpf_throw with acquired
> > > > references, but until then may I suggest the following:
> > > >
> > > > #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> > > > && !___j; bpf_throw(), ___i++)
> > > >
> > > > This will allow you to insert some cleanup code with an assertion.
> > > > Then in my series, I will convert this temporary bpf_assert_if back to
> > > > the normal bpf_assert.
> > > >
> > > > It would look like:
> > > > bpf_assert_if(opts.error == 0) {
> > > >   // Execute if assertion failed
> > > >   bpf_xdp_xfrm_state_release(x);
> > > > }
> > > >
> > > > Likewise for bpf_assert_with_if, you get the idea.
> > >
> > > I gave it a try and I'm getting this compile error:
> > >
> > >         progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
> > >                 bpf_assert_with_if(opts.error == 0, XDP_PASS) {
> > >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >         /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
> > >                 for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
> > >                                                     ^~~~      ~~~~
> > >         1 error generated.
> > >         make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1
> > >
> > > Seems like the compiler is being clever.
> >
> > It looks like ___j is used twice - maybe it was meant to be ___i? i.e.:
> >
> >    for (int ___i = 0, ___j = (cond); !(___j) && !___i; bpf_throw(value), ___i++)
> >
>
> Ah, yes, that's a typo. Eyal is right, it should be ___i.

Additionally, I would modify the macro to do ___j = !!(cond).
Daniel Xu Dec. 14, 2023, 6:23 p.m. UTC | #15 
On Thu, Dec 14, 2023 at 05:16:08PM +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 14 Dec 2023 at 17:08, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > On Thu, 14 Dec 2023 at 00:49, Eyal Birger <eyal.birger@gmail.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 3:15 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > [...]
> > > > > >
> > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > index c0dd38616562..f00dba85ac5d 100644
> > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > @@ -8,8 +8,9 @@
> > > > > >   */
> > > > > >  #include "vmlinux.h"
> > > > > >  #include <bpf/bpf_core_read.h>
> > > > > > -#include <bpf/bpf_helpers.h>
> > > > > >  #include <bpf/bpf_endian.h>
> > > > > > +#include <bpf/bpf_helpers.h>
> > > > > > +#include "bpf_experimental.h"
> > > > > >  #include "bpf_kfuncs.h"
> > > > > >  #include "bpf_tracing_net.h"
> > > > > >
> > > > > > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > >         opts.family = AF_INET;
> > > > > >
> > > > > >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > -       if (!x || opts.error)
> > > > > > +       if (!x)
> > > > > >                 goto out;
> > > > > > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> > > > > >
> > > > > >         if (!x->replay_esn)
> > > > > >                 goto out;
> > > > > >
> > > > > > results in:
> > > > > >
> > > > > > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > > > > > 58: (85) call bpf_throw#115436
> > > > > > calling kernel function bpf_throw is not allowed
> > > > > >
> > > > >
> > > > > I think this might be because bpf_throw is not registered for use by
> > > > > BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> > > > > this program type as well, since it's already done for TC.
> > > >
> > > > Ah yeah, that was it.
> > > >
> > > > >
> > > > > > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > > > > > but I can run the exceptions selftests just fine with the same bzImage.
> > > > > > So I'm thinking it's not a kfunc registration or BTF issue.
> > > > > >
> > > > > > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> > > > > >
> > > > >
> > > > > Yes, even once you enable this, this will fail for now. I am sending
> > > > > out a series later this week that enables bpf_throw with acquired
> > > > > references, but until then may I suggest the following:
> > > > >
> > > > > #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> > > > > && !___j; bpf_throw(), ___i++)
> > > > >
> > > > > This will allow you to insert some cleanup code with an assertion.
> > > > > Then in my series, I will convert this temporary bpf_assert_if back to
> > > > > the normal bpf_assert.
> > > > >
> > > > > It would look like:
> > > > > bpf_assert_if(opts.error == 0) {
> > > > >   // Execute if assertion failed
> > > > >   bpf_xdp_xfrm_state_release(x);
> > > > > }
> > > > >
> > > > > Likewise for bpf_assert_with_if, you get the idea.
> > > >
> > > > I gave it a try and I'm getting this compile error:
> > > >
> > > >         progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
> > > >                 bpf_assert_with_if(opts.error == 0, XDP_PASS) {
> > > >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >         /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
> > > >                 for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
> > > >                                                     ^~~~      ~~~~
> > > >         1 error generated.
> > > >         make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1
> > > >
> > > > Seems like the compiler is being clever.
> > >
> > > It looks like ___j is used twice - maybe it was meant to be ___i? i.e.:
> > >
> > >    for (int ___i = 0, ___j = (cond); !(___j) && !___i; bpf_throw(value), ___i++)
> > >
> >
> > Ah, yes, that's a typo. Eyal is right, it should be ___i.
> 
> Additionally, I would modify the macro to do ___j = !!(cond).

Makes sense. Will send out v6 with these fixes today.
Daniel Xu Dec. 14, 2023, 8:24 p.m. UTC | #16 
On Thu, Dec 14, 2023 at 11:23:02AM -0700, Daniel Xu wrote:
> On Thu, Dec 14, 2023 at 05:16:08PM +0100, Kumar Kartikeya Dwivedi wrote:
> > On Thu, 14 Dec 2023 at 17:08, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > On Thu, 14 Dec 2023 at 00:49, Eyal Birger <eyal.birger@gmail.com> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 3:15 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > > [...]
> > > > > > >
> > > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > index c0dd38616562..f00dba85ac5d 100644
> > > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > > @@ -8,8 +8,9 @@
> > > > > > >   */
> > > > > > >  #include "vmlinux.h"
> > > > > > >  #include <bpf/bpf_core_read.h>
> > > > > > > -#include <bpf/bpf_helpers.h>
> > > > > > >  #include <bpf/bpf_endian.h>
> > > > > > > +#include <bpf/bpf_helpers.h>
> > > > > > > +#include "bpf_experimental.h"
> > > > > > >  #include "bpf_kfuncs.h"
> > > > > > >  #include "bpf_tracing_net.h"
> > > > > > >
> > > > > > > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > > >         opts.family = AF_INET;
> > > > > > >
> > > > > > >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > > -       if (!x || opts.error)
> > > > > > > +       if (!x)
> > > > > > >                 goto out;
> > > > > > > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> > > > > > >
> > > > > > >         if (!x->replay_esn)
> > > > > > >                 goto out;
> > > > > > >
> > > > > > > results in:
> > > > > > >
> > > > > > > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > > > > > > 58: (85) call bpf_throw#115436
> > > > > > > calling kernel function bpf_throw is not allowed
> > > > > > >
> > > > > >
> > > > > > I think this might be because bpf_throw is not registered for use by
> > > > > > BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> > > > > > this program type as well, since it's already done for TC.
> > > > >
> > > > > Ah yeah, that was it.
> > > > >
> > > > > >
> > > > > > > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > > > > > > but I can run the exceptions selftests just fine with the same bzImage.
> > > > > > > So I'm thinking it's not a kfunc registration or BTF issue.
> > > > > > >
> > > > > > > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> > > > > > >
> > > > > >
> > > > > > Yes, even once you enable this, this will fail for now. I am sending
> > > > > > out a series later this week that enables bpf_throw with acquired
> > > > > > references, but until then may I suggest the following:
> > > > > >
> > > > > > #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> > > > > > && !___j; bpf_throw(), ___i++)
> > > > > >
> > > > > > This will allow you to insert some cleanup code with an assertion.
> > > > > > Then in my series, I will convert this temporary bpf_assert_if back to
> > > > > > the normal bpf_assert.
> > > > > >
> > > > > > It would look like:
> > > > > > bpf_assert_if(opts.error == 0) {
> > > > > >   // Execute if assertion failed
> > > > > >   bpf_xdp_xfrm_state_release(x);
> > > > > > }
> > > > > >
> > > > > > Likewise for bpf_assert_with_if, you get the idea.
> > > > >
> > > > > I gave it a try and I'm getting this compile error:
> > > > >
> > > > >         progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
> > > > >                 bpf_assert_with_if(opts.error == 0, XDP_PASS) {
> > > > >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >         /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
> > > > >                 for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
> > > > >                                                     ^~~~      ~~~~
> > > > >         1 error generated.
> > > > >         make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1
> > > > >
> > > > > Seems like the compiler is being clever.
> > > >
> > > > It looks like ___j is used twice - maybe it was meant to be ___i? i.e.:
> > > >
> > > >    for (int ___i = 0, ___j = (cond); !(___j) && !___i; bpf_throw(value), ___i++)
> > > >
> > >
> > > Ah, yes, that's a typo. Eyal is right, it should be ___i.
> > 
> > Additionally, I would modify the macro to do ___j = !!(cond).
> 
> Makes sense. Will send out v6 with these fixes today.
> 

Looks like only x86 supports exceptions (looking at
bpf_jit_supports_exceptions()).

This causes selftests in this patchset to fail on !x86, which is
unfortunate. We probably want to be running these tests on all the major
archs, so I will drop the assertion patches from this patchset.

But since they're generally useful and I've already written the
selftests for it, I could put them up in another patchset? Or maybe not
cuz you're gonna fix it later anyways. WDYT?

Thanks,
Daniel
Alexei Starovoitov Dec. 14, 2023, 8:53 p.m. UTC | #17 
On Thu, Dec 14, 2023 at 12:24 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
>
> Looks like only x86 supports exceptions (looking at
> bpf_jit_supports_exceptions()).
>
> This causes selftests in this patchset to fail on !x86, which is
> unfortunate. We probably want to be running these tests on all the major
> archs, so I will drop the assertion patches from this patchset.
>
> But since they're generally useful and I've already written the
> selftests for it, I could put them up in another patchset? Or maybe not
> cuz you're gonna fix it later anyways. WDYT?

Yeah. don't use bpf_assert in generic tests yet.
Only tests that test bpf_assert should use it.

Pls send the ones you wrote separately, so they stay in email archives
and we can pick them up later.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
index 2d7f8fa82ebd..fc804095d578 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_tunnel.c
@@ -278,7 +278,7 @@  static int add_xfrm_tunnel(void)
 	SYS(fail,
 	    "ip netns exec at_ns0 "
 		"ip xfrm state add src %s dst %s proto esp "
-			"spi %d reqid 1 mode tunnel "
+			"spi %d reqid 1 mode tunnel replay-window 42 "
 			"auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
 	    IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
 	SYS(fail,
@@ -292,7 +292,7 @@  static int add_xfrm_tunnel(void)
 	SYS(fail,
 	    "ip netns exec at_ns0 "
 		"ip xfrm state add src %s dst %s proto esp "
-			"spi %d reqid 2 mode tunnel "
+			"spi %d reqid 2 mode tunnel replay-window 42 "
 			"auth-trunc 'hmac(sha1)' %s 96 enc 'cbc(aes)' %s",
 	    IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
 	SYS(fail,
@@ -313,7 +313,7 @@  static int add_xfrm_tunnel(void)
 	 */
 	SYS(fail,
 	    "ip xfrm state add src %s dst %s proto esp "
-		    "spi %d reqid 1 mode tunnel "
+		    "spi %d reqid 1 mode tunnel replay-window 42 "
 		    "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
 	    IP4_ADDR_VETH0, IP4_ADDR1_VETH1, XFRM_SPI_IN_TO_OUT, XFRM_AUTH, XFRM_ENC);
 	SYS(fail,
@@ -325,7 +325,7 @@  static int add_xfrm_tunnel(void)
 	/* root -> at_ns0 */
 	SYS(fail,
 	    "ip xfrm state add src %s dst %s proto esp "
-		    "spi %d reqid 2 mode tunnel "
+		    "spi %d reqid 2 mode tunnel replay-window 42 "
 		    "auth-trunc 'hmac(sha1)' %s 96  enc 'cbc(aes)' %s",
 	    IP4_ADDR1_VETH1, IP4_ADDR_VETH0, XFRM_SPI_OUT_TO_IN, XFRM_AUTH, XFRM_ENC);
 	SYS(fail,
@@ -628,8 +628,10 @@  static void test_xfrm_tunnel(void)
 {
 	DECLARE_LIBBPF_OPTS(bpf_tc_hook, tc_hook,
 			    .attach_point = BPF_TC_INGRESS);
+	LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
 	struct test_tunnel_kern *skel = NULL;
 	struct nstoken *nstoken;
+	int xdp_prog_fd;
 	int tc_prog_fd;
 	int ifindex;
 	int err;
@@ -654,6 +656,14 @@  static void test_xfrm_tunnel(void)
 	if (attach_tc_prog(&tc_hook, tc_prog_fd, -1))
 		goto done;
 
+	/* attach xdp prog to tunnel dev */
+	xdp_prog_fd = bpf_program__fd(skel->progs.xfrm_get_state_xdp);
+	if (!ASSERT_GE(xdp_prog_fd, 0, "bpf_program__fd"))
+		goto done;
+	err = bpf_xdp_attach(ifindex, xdp_prog_fd, XDP_FLAGS_REPLACE, &opts);
+	if (!ASSERT_OK(err, "bpf_xdp_attach"))
+		goto done;
+
 	/* ping from at_ns0 namespace test */
 	nstoken = open_netns("at_ns0");
 	err = test_ping(AF_INET, IP4_ADDR_TUNL_DEV1);
@@ -667,6 +677,8 @@  static void test_xfrm_tunnel(void)
 		goto done;
 	if (!ASSERT_EQ(skel->bss->xfrm_remote_ip, 0xac100164, "remote_ip"))
 		goto done;
+	if (!ASSERT_EQ(skel->bss->xfrm_replay_window, 42, "replay_window"))
+		goto done;
 
 done:
 	delete_xfrm_tunnel();
diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
index 3a59eb9c34de..c0dd38616562 100644
--- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
@@ -30,6 +30,10 @@  int bpf_skb_set_fou_encap(struct __sk_buff *skb_ctx,
 			  struct bpf_fou_encap *encap, int type) __ksym;
 int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
 			  struct bpf_fou_encap *encap) __ksym;
+struct xfrm_state *
+bpf_xdp_get_xfrm_state(struct xdp_md *ctx, struct bpf_xfrm_state_opts *opts,
+		       u32 opts__sz) __ksym;
+void bpf_xdp_xfrm_state_release(struct xfrm_state *x) __ksym;
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
@@ -950,4 +954,51 @@  int xfrm_get_state(struct __sk_buff *skb)
 	return TC_ACT_OK;
 }
 
+volatile int xfrm_replay_window = 0;
+
+SEC("xdp")
+int xfrm_get_state_xdp(struct xdp_md *xdp)
+{
+	struct bpf_xfrm_state_opts opts = {};
+	struct xfrm_state *x = NULL;
+	struct ip_esp_hdr *esph;
+	struct bpf_dynptr ptr;
+	u8 esph_buf[8] = {};
+	u8 iph_buf[20] = {};
+	struct iphdr *iph;
+	u32 off;
+
+	if (bpf_dynptr_from_xdp(xdp, 0, &ptr))
+		goto out;
+
+	off = sizeof(struct ethhdr);
+	iph = bpf_dynptr_slice(&ptr, off, iph_buf, sizeof(iph_buf));
+	if (!iph || iph->protocol != IPPROTO_ESP)
+		goto out;
+
+	off += sizeof(struct iphdr);
+	esph = bpf_dynptr_slice(&ptr, off, esph_buf, sizeof(esph_buf));
+	if (!esph)
+		goto out;
+
+	opts.netns_id = BPF_F_CURRENT_NETNS;
+	opts.daddr.a4 = iph->daddr;
+	opts.spi = esph->spi;
+	opts.proto = IPPROTO_ESP;
+	opts.family = AF_INET;
+
+	x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
+	if (!x || opts.error)
+		goto out;
+
+	if (!x->replay_esn)
+		goto out;
+
+	xfrm_replay_window = x->replay_esn->replay_window;
+out:
+	if (x)
+		bpf_xdp_xfrm_state_release(x);
+	return XDP_PASS;
+}
+
 char _license[] SEC("license") = "GPL";