Re: [PATCH ipsec-next,v2 3/3] selftests/bpf: add xfrm_info tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux