netdev - Re: [PATCH bpf-next 8/8] bpf, testing: add various tail call test cases

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAEf4BzYffw=COVaz8-xZeq8-=Wczmq-h+mAPdwoKTNYrpf739g@mail.gmail.com>
Date:   Wed, 20 Nov 2019 11:26:51 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        john fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 8/8] bpf, testing: add various tail call test cases

On Mon, Nov 18, 2019 at 5:38 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> Add several BPF kselftest cases for tail calls which test the various
> patch directions (NOP -> JMP, JMP -> JMP, JMP -> NOP), and that multiple
> locations are patched.
>
>   # ./test_progs -n 44
>   #44 tailcalls:OK
>   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> ---
>  .../selftests/bpf/prog_tests/tailcalls.c      | 210 ++++++++++++++++++
>  tools/testing/selftests/bpf/progs/tailcall1.c |  48 ++++
>  tools/testing/selftests/bpf/progs/tailcall2.c |  59 +++++
>  3 files changed, 317 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/tailcalls.c
>  create mode 100644 tools/testing/selftests/bpf/progs/tailcall1.c
>  create mode 100644 tools/testing/selftests/bpf/progs/tailcall2.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> new file mode 100644
> index 000000000000..6862bb5f9688
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +
> +static void test_tailcall_1(void)
> +{
> +       int err, map_fd, prog_fd, main_fd, i, j;
> +       struct bpf_map *prog_array;
> +       struct bpf_program *prog;
> +       struct bpf_object *obj;
> +       __u32 retval, duration;
> +       char prog_name[32];
> +       char buff[128] = {};
> +
> +       err = bpf_prog_load("tailcall1.o", BPF_PROG_TYPE_SCHED_CLS, &obj,
> +                           &prog_fd);
> +       if (CHECK_FAIL(err))
> +               return;
> +
> +       prog = bpf_object__find_program_by_title(obj, "classifier");
> +       if (CHECK_FAIL(!prog))
> +               goto out;
> +
> +       main_fd = bpf_program__fd(prog);
> +       if (CHECK_FAIL(main_fd < 0))
> +               goto out;


can this happen if bpf_prog_load succeeded? same for a bunch of
prog_fd checks below.

> +
> +       prog_array = bpf_object__find_map_by_name(obj, "jmp_table");
> +       if (CHECK_FAIL(!prog_array))
> +               goto out;
> +

[...]

> +
> +       for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
> +               err = bpf_prog_test_run(main_fd, 1, buff, sizeof(buff), 0,
> +                                       &duration, &retval, NULL);
> +               CHECK(err || retval != i, "tailcall",
> +                     "err %d errno %d retval %d\n", err, errno, retval);
> +
> +               err = bpf_map_delete_elem(map_fd, &i);
> +               if (CHECK_FAIL(err))
> +                       goto out;
> +       }
> +
> +       /* Testing JMP -> NOP */

nit: this comment should probably go before previous loop?

> +       err = bpf_prog_test_run(main_fd, 1, buff, sizeof(buff), 0,
> +                               &duration, &retval, NULL);
> +       CHECK(err || retval != 3, "tailcall", "err %d errno %d retval %d\n",
> +             err, errno, retval);
> +

[...]

> +       for (i = 0; i < bpf_map__def(prog_array)->max_entries; i++) {
> +               j = bpf_map__def(prog_array)->max_entries - 1 - i;
> +
> +               err = bpf_prog_test_run(main_fd, 1, buff, sizeof(buff), 0,
> +                                       &duration, &retval, NULL);
> +               CHECK(err || retval != j, "tailcall",
> +                     "err %d errno %d retval %d\n", err, errno, retval);
> +
> +               err = bpf_map_delete_elem(map_fd, &i);

in addition to explicit delete, can you test update to NULL? Also, I
think it might be useful to validate update from NULL to NULL (it's a
separate check in your poke_run code).

> +               if (CHECK_FAIL(err))
> +                       goto out;
> +       }
> +
> +       err = bpf_prog_test_run(main_fd, 1, buff, sizeof(buff), 0,
> +                               &duration, &retval, NULL);
> +       CHECK(err || retval != 3, "tailcall", "err %d errno %d retval %d\n",
> +             err, errno, retval);
> +out:
> +       bpf_object__close(obj);
> +}
> +

[...]

> +void test_tailcalls(void)
> +{
> +       test_tailcall_1();
> +       test_tailcall_2();
> +}

these could be sub-tests:


if (test__start_subtest("tailcall_1"))
    test_tailcall_1();
if (test__start_subtest("tailcall_2"))
    test_tailcall_2();

though, a bit more descriptive names would be certainly better :)

> diff --git a/tools/testing/selftests/bpf/progs/tailcall1.c b/tools/testing/selftests/bpf/progs/tailcall1.c
> new file mode 100644
> index 000000000000..63531e1a9fa4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall1.c
> @@ -0,0 +1,48 @@

[...]