Hi Martin,
On Wed, Nov 30, 2022 at 8:41 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 11/29/22 5:20 AM, Eyal Birger wrote:
> > Test the xfrm_info kfunc helpers.
> >
> > Note: the tests require support for xfrmi "external" mode in iproute2.
> >
> > The test setup creates three name spaces - NS0, NS1, NS2.
> >
> > XFRM tunnels are setup between NS0 and the two other NSs.
> >
> > The kfunc helpers are used to steer traffic from NS0 to the other
> > NSs based on a userspace populated map and validate that the
> > return traffic had arrived from the desired NS.
> >
> > Signed-off-by: Eyal Birger <eyal.birger@xxxxxxxxx>
> >
> > ---
> >
> > v2:
> > - use an lwt route in NS1 for testing that flow as well
> > - indendation fix
> > ---
> > tools/testing/selftests/bpf/config | 2 +
> > .../selftests/bpf/prog_tests/test_xfrm_info.c | 343 ++++++++++++++++++
> > .../selftests/bpf/progs/test_xfrm_info_kern.c | 74 ++++
> > 3 files changed, 419 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/test_xfrm_info.c
> > create mode 100644 tools/testing/selftests/bpf/progs/test_xfrm_info_kern.c
> >
> > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > index 9213565c0311..9f39943d6ebd 100644
> > --- a/tools/testing/selftests/bpf/config
> > +++ b/tools/testing/selftests/bpf/config
> > @@ -20,6 +20,7 @@ CONFIG_IKCONFIG_PROC=y
> > CONFIG_IMA=y
> > CONFIG_IMA_READ_POLICY=y
> > CONFIG_IMA_WRITE_POLICY=y
> > +CONFIG_INET_ESP=y
> > CONFIG_IP_NF_FILTER=y
> > CONFIG_IP_NF_RAW=y
> > CONFIG_IP_NF_TARGET_SYNPROXY=y
> > @@ -71,3 +72,4 @@ CONFIG_TEST_BPF=y
> > CONFIG_USERFAULTFD=y
> > CONFIG_VXLAN=y
> > CONFIG_XDP_SOCKETS=y
> > +CONFIG_XFRM_INTERFACE=y
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_xfrm_info.c b/tools/testing/selftests/bpf/prog_tests/test_xfrm_info.c
> > new file mode 100644
> > index 000000000000..3aef72540934
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_xfrm_info.c
>
> Nit. Just xfrm_info.c
Ok.
>
> > @@ -0,0 +1,343 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > +
> > +/*
> > + * Topology:
> > + * ---------
> > + * NS0 namespace | NS1 namespace | NS2 namespace
> > + * | |
> > + * +---------------+ | +---------------+ |
> > + * | ipsec0 |---------| ipsec0 | |
> > + * | 192.168.1.100 | | | 192.168.1.200 | |
> > + * | if_id: bpf | | +---------------+ |
> > + * +---------------+ | |
> > + * | | | +---------------+
> > + * | | | | ipsec0 |
> > + * \------------------------------------------| 192.168.1.200 |
> > + * | | +---------------+
> > + * | |
> > + * | | (overlay network)
> > + * ------------------------------------------------------
> > + * | | (underlay network)
> > + * +--------------+ | +--------------+ |
> > + * | veth01 |----------| veth10 | |
> > + * | 172.16.1.100 | | | 172.16.1.200 | |
> > + * ---------------+ | +--------------+ |
> > + * | |
> > + * +--------------+ | | +--------------+
> > + * | veth02 |-----------------------------------| veth20 |
> > + * | 172.16.2.100 | | | | 172.16.2.200 |
> > + * +--------------+ | | +--------------+
> > + *
> > + *
[...]
> > +
> > +#define RUN_TEST(name) \
> > + ({ \
> > + if (test__start_subtest(#name)) { \
> > + test_ ## name(); \
> > + } \
> > + })
> > +
> > +static void *test_xfrm_info_run_tests(void *arg)
> > +{
> > + cleanup();
> > +
> > + config_underlay();
> > + config_overlay();
>
> config_*() is returning ok/err but no error checking here. Does it make sense
> to continue if the config_*() failed?
I'll assert their success.
>
> > +
> > + RUN_TEST(xfrm_info);
>
> nit. Remove this macro indirection. There is only one test.
Ok. I was considering other possible tests in the future, but this can
be added then.
>
> > +
> > + cleanup();
> > +
> > + return NULL;
> > +}
> > +
> > +static int probe_iproute2(void)
> > +{
> > + if (SYS_NOFAIL("ip link add type xfrm help 2>&1 | "
> > + "grep external > /dev/null")) {
> > + fprintf(stdout, "%s:SKIP: iproute2 with xfrm external support needed for this test\n", __func__);
>
> Unfortunately, the BPF CI iproute2 does not have this support also :(
> I am worry it will just stay SKIP for some time and rot. Can you try to
> directly use netlink here?
Yeah, I wasn't sure if adding a libmnl (or alternative) dependency
was ok here, and also didn't want to copy all that nl logic here.
So I figured it would get there eventually.
I noticed libmnl is used by the nf tests, so maybe its inclusion isn't too
bad. Unless there's a better approach.
As you noted, I should add this for the "external" support. However, I don't
think adding the LWT route directly using nl is a good idea here so I'll
make the NS1 use a regular xfrmi.
>
> https://github.com/kernel-patches/bpf/actions/runs/3578467213/jobs/6019370754#step:6:6395
>
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
> > +void serial_test_xfrm_info(void)
>
> Remove "serial_". New test must be able to run in parallel ("./test_progs -j").
Ok.
>
> > +{
> > + pthread_t test_thread;
> > + int err;
> > +
> > + if (probe_iproute2()) {
> > + test__skip();
> > + return;
> > + }
> > +
> > + /* Run the tests in their own thread to isolate the namespace changes
> > + * so they do not affect the environment of other tests.
> > + * (specifically needed because of unshare(CLONE_NEWNS) in open_netns())
> > + */
>
> I think this comment is mostly inherited from other tests (eg. tc_redirect.c)
> but the pthread dance is actually unnecessary. The test_progs.c will restore
> the original netns before running the next test. I am abort to remove this from
> the tc_redirect.c also. Please also avoid this pthread create here.
Ok. Indeed was inherited.
>
> > + err = pthread_create(&test_thread, NULL, &test_xfrm_info_run_tests, NULL);
> > + if (ASSERT_OK(err, "pthread_create"))
> > + ASSERT_OK(pthread_join(test_thread, NULL), "pthread_join");
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_xfrm_info_kern.c b/tools/testing/selftests/bpf/progs/test_xfrm_info_kern.c
> > new file mode 100644
> > index 000000000000..98991a83c1e9
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_xfrm_info_kern.c
>
>
> Nit. Same here. Just xfrm_info.c.
Ok.
>
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <linux/pkt_cls.h>
> > +#include <bpf/bpf_helpers.h>
> > +
> > +#define log_err(__ret) bpf_printk("ERROR line:%d ret:%d\n", __LINE__, __ret)
>
> Please try not to use unnecessary bpf_printk(). BPF CI is not capturing it also.
Ok.
>
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 2);
> > + __type(key, __u32);
> > + __type(value, __u32);
> > +} dst_if_id_map SEC(".maps");
>
> It is easier to use global variables instead of a map.
Would these be available for read/write from the test application (as the
map is currently populated/read from userspace)?
>
> > +
> > +struct bpf_xfrm_info {
> > + __u32 if_id;
> > + int link;
> > +};
>
> This needs __attribute__((preserve_access_index) for CO-RE.
> It is easier to just include vmlinux.h to get the struct xfrm_md_info { ... }.
Ok.
>
> > +
> > +int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx,
> > + const struct bpf_xfrm_info *from) __ksym;
> > +int bpf_skb_get_xfrm_info(struct __sk_buff *skb_ctx,
> > + struct bpf_xfrm_info *to) __ksym;
> > +
> > +SEC("tc")
> > +int set_xfrm_info(struct __sk_buff *skb)
> > +{
> > + struct bpf_xfrm_info info = {};
> > + __u32 *if_id = NULL;
> > + __u32 index = 0;
> > + int ret = -1;
> > +
> > + if_id = bpf_map_lookup_elem(&dst_if_id_map, &index);
> > + if (!if_id) {
> > + log_err(ret);
> > + return TC_ACT_SHOT;
> > + }
> > +
> > + info.if_id = *if_id;
> > + ret = bpf_skb_set_xfrm_info(skb, &info);
> > + if (ret < 0) {
> > + log_err(ret);
> > + return TC_ACT_SHOT;
> > + }
> > +
> > + return TC_ACT_UNSPEC;
> > +}
> > +
> > +SEC("tc")
> > +int get_xfrm_info(struct __sk_buff *skb)
> > +{
> > + struct bpf_xfrm_info info = {};
> > + __u32 *if_id = NULL;
> > + __u32 index = 1;
> > + int ret = -1;
> > +
> > + if_id = bpf_map_lookup_elem(&dst_if_id_map, &index);
> > + if (!if_id) {
> > + log_err(ret);
> > + return TC_ACT_SHOT;
> > + }
> > +
> > + ret = bpf_skb_get_xfrm_info(skb, &info);
> > + if (ret < 0) {
> > + log_err(ret);
> > + return TC_ACT_SHOT;
> > + }
> > +
> > + *if_id = info.if_id;
> > +
> > + return TC_ACT_UNSPEC;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
>
Thanks for the review!
Eyal.
