netdev - Re: [PATCH bpf 2/2] selftests/bpf: Add selftest for calling global functions from freplace

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzak82RbCYethEN7u05UKkmY=DqCiX=oHAFnHocb4fEG6w@mail.gmail.com>
Date:   Fri, 3 Jun 2022 15:03:20 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf 2/2] selftests/bpf: Add selftest for calling global
 functions from freplace

On Fri, Jun 3, 2022 at 8:42 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Add a selftest that calls a global function with a context object parameter
> from an freplace function to check that the program context type is
> correctly converted to the freplace target when fetching the context type
> from the kernel BTF.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---
>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  | 13 ++++++++++
>  .../bpf/progs/freplace_global_func.c          | 24 +++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/freplace_global_func.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> index d9aad15e0d24..6e86a1d92e97 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> @@ -395,6 +395,17 @@ static void test_func_map_prog_compatibility(void)
>                                      "./test_attach_probe.o");
>  }
>
> +static void test_func_replace_global_func(void)
> +{
> +       const char *prog_name[] = {
> +               "freplace/test_pkt_access",
> +       };

empty line between variables and statements

> +       test_fexit_bpf2bpf_common("./freplace_global_func.o",
> +                                 "./test_pkt_access.o",
> +                                 ARRAY_SIZE(prog_name),
> +                                 prog_name, false, NULL);
> +}
> +
>  /* NOTE: affect other tests, must run in serial mode */
>  void serial_test_fexit_bpf2bpf(void)
>  {
> @@ -416,4 +427,6 @@ void serial_test_fexit_bpf2bpf(void)
>                 test_func_replace_multi();
>         if (test__start_subtest("fmod_ret_freplace"))
>                 test_fmod_ret_freplace();
> +       if (test__start_subtest("func_replace_global_func"))
> +               test_func_replace_global_func();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/freplace_global_func.c b/tools/testing/selftests/bpf/progs/freplace_global_func.c
> new file mode 100644
> index 000000000000..d9f8276229cc
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/freplace_global_func.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook */
> +#include <linux/stddef.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_tracing.h>
> +
> +__attribute__ ((noinline))

__noinline

> +int test_ctx_global_func(struct __sk_buff *skb)
> +{
> +       volatile int retval = 1;
> +       return retval;

just curious, why volatile instead of direct `return 1;`? Also, empty
line between variable and return.

> +}
> +
> +__u64 test_pkt_access_global_func = 0;

nit: it's a variable, please put it at the top before functions, or at
the very least add empty line between it and SEC()

> +SEC("freplace/test_pkt_access")
> +int new_test_pkt_access(struct __sk_buff *skb)
> +{
> +       test_pkt_access_global_func = test_ctx_global_func(skb);
> +       return -1;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.36.1
>