| Message ID | 8ec1b885d2e13fcd20944cce9edc0340d993d044.1702325874.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add bpf_xdp_get_xfrm_state() kfunc | expand |
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 >
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
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.
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
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.
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
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.
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
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. [...]
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. > > [...]
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
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.
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.
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).
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.
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
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 --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";