Re: [PATCH bpf v2] selftests/bpf: Rewrite test_tc_redirect.sh as prog_tests/tc_redirect.c

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, May 3, 2021 at 4:12 AM Jussi Maki <joamaki@gmail.com> wrote:
>
> Ports test_tc_redirect.sh to the test_progs framework and removes the
> old test. This makes it more in line with rest of the tests and makes
> it possible to run this test with vmtest.sh and under the bpf CI.
>
> Signed-off-by: Jussi Maki <joamaki@gmail.com>
> ---

One bug and few mostly mechanical nits. It would be great to get a
code review from someone that knows something about iproute2 and the
whole networking stuff :)

>  tools/testing/selftests/bpf/network_helpers.c |   2 +-
>  tools/testing/selftests/bpf/network_helpers.h |   1 +
>  .../selftests/bpf/prog_tests/tc_redirect.c    | 594 ++++++++++++++++++
>  .../selftests/bpf/progs/test_tc_neigh.c       |  33 +-
>  .../selftests/bpf/progs/test_tc_neigh_fib.c   |   9 +-
>  .../selftests/bpf/progs/test_tc_peer.c        |  33 +-
>  .../testing/selftests/bpf/test_tc_redirect.sh | 216 -------
>  7 files changed, 622 insertions(+), 266 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/tc_redirect.c
>  delete mode 100755 tools/testing/selftests/bpf/test_tc_redirect.sh
>

[...]

> +
> +/**
> + * modify_proc() - Modify entry in /proc
> + *
> + * Modifies an entry in /proc and saves the original value for later
> + * restoration with restore_proc().
> + */
> +static int modify_proc(const char *path, const char *newval)
> +{
> +       struct proc_mod *mod;
> +       FILE *f;
> +
> +       f = fopen(path, "r+");
> +       if (!f)
> +               return -1;
> +
> +       if (num_proc_mods + 1 > MAX_PROC_MODS)
> +               goto fail;

you'll decrement num_proc_mods without incrementing it

> +
> +       num_proc_mods++;
> +       mod = &proc_mods[num_proc_mods-1];
> +
> +       strncpy(mod->path, path, PATH_MAX);
> +
> +       if (!fread(mod->oldval, 1, MAX_PROC_VALUE_LEN, f)) {
> +               log_err("reading from %s failed", path);
> +               goto fail;
> +       }
> +       rewind(f);
> +       if (fwrite(newval, strlen(newval), 1, f) != 1) {
> +               log_err("writing to %s failed", path);
> +               goto fail;
> +       }
> +
> +       fclose(f);
> +       return 0;
> +fail:
> +
> +       fclose(f);
> +       num_proc_mods--;
> +       return -1;
> +}
> +
> +/**
> + * restore_proc() - Restore all /proc modifications
> + */
> +static void restore_proc(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < num_proc_mods; i++) {
> +               struct proc_mod *mod = &proc_mods[i];
> +               FILE *f = fopen(mod->path, "w");

we typically split variable declaration and function calls when the
function can fail, so pretty much anything non-trivial will be done as
a separate statement (followed immediately by error check)

> +
> +               if (!f) {
> +                       log_err("fopen of %s failed", mod->path);
> +                       continue;
> +               }
> +
> +               if (fwrite(mod->oldval, mod->oldlen, 1, f) != 1)
> +                       log_err("fwrite to %s failed", mod->path);
> +
> +               fclose(f);
> +       }
> +       num_proc_mods = 0;
> +}
> +

[...]

> +
> +static int netns_setup_links_and_routes(struct netns_setup_result *result)
> +{
> +       char veth_src_fwd_addr[IFADDR_STR_LEN+1] = {0,};
> +       char veth_dst_fwd_addr[IFADDR_STR_LEN+1] = {0,};

just "= {};". same effect, but not misleading notation

> +
> +       SYS("ip link add veth_src type veth peer name veth_src_fwd");
> +       SYS("ip link add veth_dst type veth peer name veth_dst_fwd");
> +       if (get_ifaddr("veth_src_fwd", veth_src_fwd_addr))
> +               goto fail;
> +       if (get_ifaddr("veth_dst_fwd", veth_dst_fwd_addr))
> +               goto fail;
> +
> +       result->ifindex_veth_src_fwd = get_ifindex("veth_src_fwd");
> +       if (result->ifindex_veth_src_fwd < 0)
> +               goto fail;
> +       result->ifindex_veth_dst_fwd = get_ifindex("veth_dst_fwd");
> +       if (result->ifindex_veth_dst_fwd < 0)
> +               goto fail;
> +

[...]

> +static void test_tcp(int family, const char *addr, __u16 port)
> +{
> +       int listen_fd = -1, accept_fd = -1, client_fd = -1;
> +       char buf[] = "testing testing";
> +
> +       if (!ASSERT_OK(setns_by_name(NS_DST), "setns dst"))
> +               return;
> +
> +       listen_fd = start_server(family, SOCK_STREAM, addr, port, 0);
> +       if (!ASSERT_GE(listen_fd, 0, "listen"))
> +               goto done;
> +
> +       if (!ASSERT_OK(setns_by_name(NS_SRC), "setns src"))
> +               goto done;
> +
> +       client_fd = connect_to_fd(listen_fd, TIMEOUT_MILLIS);
> +       if (!ASSERT_GE(client_fd, 0, "connect_to_fd"))
> +               goto done;
> +
> +       accept_fd = accept(listen_fd, NULL, NULL);
> +       if (!ASSERT_GE(accept_fd, 0, "accept"))
> +               goto done;
> +
> +       if (!ASSERT_OK(settimeo(accept_fd, TIMEOUT_MILLIS), "settimeo"))
> +               goto done;
> +
> +       if (!ASSERT_EQ(
> +               write(client_fd, buf, sizeof(buf)),
> +               sizeof(buf),
> +               "send to server"))
> +               goto done;

I think cases like this are much cleaner if written as:

n = write(client_fd, buf, sizeof(buf));
if (!ASSERT_EQ(n, sizeof(buf), "send to server"))
    goto done;

Don't try to do too much in a single if. Same below.

> +
> +       if (!ASSERT_EQ(
> +               read(accept_fd, buf, sizeof(buf)),
> +               sizeof(buf),
> +               "recv from server"))
> +               goto done;
> +
> +done:
> +       setns_root();
> +       if (listen_fd >= 0)
> +               close(listen_fd);
> +       if (accept_fd >= 0)
> +               close(accept_fd);
> +       if (client_fd >= 0)
> +               close(client_fd);
> +}
> +
> +static int test_ping(int family, const char *addr)
> +{
> +       const char *ping = family == AF_INET6 ? "ping6" : "ping";
> +
> +       SYS("ip netns exec " NS_SRC " %s " PING_ARGS " %s", ping, addr);
> +       return 0;
> +fail:
> +       return -1;
> +}
> +
> +static void test_connectivity(void)
> +{
> +       test_tcp(AF_INET, IP4_DST, IP4_PORT);
> +       test_ping(AF_INET, IP4_DST);
> +       test_tcp(AF_INET6, IP6_DST, IP6_PORT);
> +       test_ping(AF_INET6, IP6_DST);
> +}
> +
> +static void test_tc_redirect_neigh_fib(struct netns_setup_result *setup_result)
> +{
> +       struct test_tc_neigh_fib *skel;
> +
> +       skel = test_tc_neigh_fib__open();
> +       if (!ASSERT_OK_PTR(skel, "test_tc_neigh_fib__open"))
> +               return;
> +
> +       if (!ASSERT_OK(test_tc_neigh_fib__load(skel), "test_tc_neigh_fib__load")) {
> +               test_tc_neigh_fib__destroy(skel);
> +               return;
> +       }
> +
> +       if (!ASSERT_OK(
> +               bpf_program__pin(skel->progs.tc_src, SRC_PROG_PIN_FILE),
> +               "pin " SRC_PROG_PIN_FILE))
> +               goto done;
> +

see above, use

err = bpf_program__pin(...);
if (!ASSERT_OK(err, ...))

That actually makes it easier to debug as well, btw.

> +       if (!ASSERT_OK(
> +               bpf_program__pin(skel->progs.tc_chk, CHK_PROG_PIN_FILE),
> +               "pin " CHK_PROG_PIN_FILE))
> +               goto done;
> +
> +       if (!ASSERT_OK(
> +               bpf_program__pin(skel->progs.tc_dst, DST_PROG_PIN_FILE),
> +               "pin " DST_PROG_PIN_FILE))
> +               goto done;
> +
> +       if (netns_load_bpf())
> +               goto done;
> +
> +       /* bpf_fib_lookup() checks if forwarding is enabled */
> +       if (!ASSERT_OK(setns_by_name(NS_FWD), "setns fwd"))
> +               goto done;
> +       if (!ASSERT_OK(modify_proc("/proc/sys/net/ipv4/ip_forward", "1"),
> +                                  "set ipv4.ip_forward"))
> +               goto done;
> +       if (!ASSERT_OK(modify_proc("/proc/sys/net/ipv6/conf/all/forwarding", "1"),
> +                                  "set ipv6.forwarding"))
> +               goto done;
> +       setns_root();
> +
> +       test_connectivity();
> +done:
> +       bpf_program__unpin(skel->progs.tc_src, SRC_PROG_PIN_FILE);
> +       bpf_program__unpin(skel->progs.tc_chk, CHK_PROG_PIN_FILE);
> +       bpf_program__unpin(skel->progs.tc_dst, DST_PROG_PIN_FILE);
> +       test_tc_neigh_fib__destroy(skel);
> +       netns_unload_bpf();
> +       setns_root();
> +       restore_proc();
> +}
> +

[...]