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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzbY3zV-xYDBvCYztXOkn=MJwHxOVyAH7YRH8JH869qtDg@mail.gmail.com>
Date:   Thu, 10 Sep 2020 15:34:26 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg
 dereference in extension trace

On Wed, Sep 9, 2020 at 8:38 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> Adding test that setup following program:
>
>   SEC("classifier/test_pkt_md_access")
>   int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
>   SEC("freplace/test_pkt_md_access")
>   int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
>   SEC("fentry/test_pkt_md_access_new")
>   int BPF_PROG(fentry, struct sk_buff *skb)
>
> The test verifies that the tracing program can
> dereference skb argument properly.
>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  .../selftests/bpf/prog_tests/trace_ext.c      | 93 +++++++++++++++++++
>  .../selftests/bpf/progs/test_trace_ext.c      | 18 ++++
>  .../bpf/progs/test_trace_ext_tracing.c        | 25 +++++
>  3 files changed, 136 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
> new file mode 100644
> index 000000000000..1089dafb4653
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include <sys/stat.h>
> +#include <linux/sched.h>
> +#include <sys/syscall.h>
> +
> +#include "test_trace_ext.skel.h"
> +#include "test_trace_ext_tracing.skel.h"
> +
> +static __u32 duration;
> +
> +void test_trace_ext(void)
> +{
> +       struct test_trace_ext_tracing *skel_trace = NULL;
> +       struct test_trace_ext_tracing__bss *bss_trace;
> +       const char *file = "./test_pkt_md_access.o";
> +       struct test_trace_ext *skel_ext = NULL;
> +       struct test_trace_ext__bss *bss_ext;
> +       int err, prog_fd, ext_fd;
> +       struct bpf_object *obj;
> +       char buf[100];
> +       __u32 retval;
> +       __u64 len;
> +
> +       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> +       if (CHECK_FAIL(err))
> +               return;

We should avoid using bpf_prog_load() for new code. Can you please
just skeleton instead? Or at least bpf_object__open_file()?

> +
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> +                           .attach_prog_fd = prog_fd,
> +       );

DECLARE_LIBBPF_OPTS does declare a variable, so should be together
with all the other variables above, otherwise some overly strict C89
mode compiler will start complaining. You can assign
`opts.attach_prog_fd = prog_fd;` outside of declaration. But I also
don't think you need this one. Having .attach_prog_fd in open_opts is
not great, because it's a per-program setting specified at bpf_object
level. Would bpf_program__set_attach_target() work here?

> +
> +       skel_ext = test_trace_ext__open_opts(&opts);
> +       if (CHECK(!skel_ext, "setup", "freplace/test_pkt_md_access open failed\n"))
> +               goto cleanup;
> +
> +       err = test_trace_ext__load(skel_ext);
> +       if (CHECK(err, "setup", "freplace/test_pkt_md_access load failed\n")) {
> +               libbpf_strerror(err, buf, sizeof(buf));
> +               fprintf(stderr, "%s\n", buf);
> +               goto cleanup;
> +       }
> +
> +       err = test_trace_ext__attach(skel_ext);
> +       if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err))
> +               goto cleanup;
> +
> +       ext_fd = bpf_program__fd(skel_ext->progs.test_pkt_md_access_new);
> +
> +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts_trace,
> +                           .attach_prog_fd = ext_fd,
> +       );
> +

same

> +       skel_trace = test_trace_ext_tracing__open_opts(&opts_trace);
> +       if (CHECK(!skel_trace, "setup", "tracing/test_pkt_md_access_new open failed\n"))
> +               goto cleanup;
> +
> +       err = test_trace_ext_tracing__load(skel_trace);
> +       if (CHECK(err, "setup", "tracing/test_pkt_md_access_new load failed\n")) {
> +               libbpf_strerror(err, buf, sizeof(buf));
> +               fprintf(stderr, "%s\n", buf);
> +               goto cleanup;
> +       }
> +
> +       err = test_trace_ext_tracing__attach(skel_trace);
> +       if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
> +               goto cleanup;
> +
> +       err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
> +                               NULL, NULL, &retval, &duration);
> +       CHECK(err || retval, "",
> +             "err %d errno %d retval %d duration %d\n",
> +             err, errno, retval, duration);
> +
> +       bss_ext = skel_ext->bss;
> +       bss_trace = skel_trace->bss;
> +
> +       len = bss_ext->ext_called;
> +
> +       CHECK(bss_ext->ext_called == 0,
> +               "check", "failed to trigger freplace/test_pkt_md_access\n");
> +       CHECK(bss_trace->fentry_called != len,
> +               "check", "failed to trigger fentry/test_pkt_md_access_new\n");
> +       CHECK(bss_trace->fexit_called != len,
> +               "check", "failed to trigger fexit/test_pkt_md_access_new\n");
> +
> +cleanup:
> +       test_trace_ext__destroy(skel_ext);
> +       bpf_object__close(obj);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> new file mode 100644
> index 000000000000..a6318f6b52ee
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Facebook
> +#include <linux/bpf.h>
> +#include <stdbool.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +#include <bpf/bpf_tracing.h>
> +
> +volatile __u64 ext_called = 0;

nit: no need for volatile, global variables are not going anywhere;
same below in two places

> +
> +SEC("freplace/test_pkt_md_access")
> +int test_pkt_md_access_new(struct __sk_buff *skb)
> +{
> +       ext_called = skb->len;
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
> new file mode 100644
> index 000000000000..9e52a831446f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +volatile __u64 fentry_called = 0;
> +
> +SEC("fentry/test_pkt_md_access_new")
> +int BPF_PROG(fentry, struct sk_buff *skb)
> +{
> +       fentry_called = skb->len;
> +       return 0;
> +}
> +
> +volatile __u64 fexit_called = 0;
> +
> +SEC("fexit/test_pkt_md_access_new")
> +int BPF_PROG(fexit, struct sk_buff *skb)
> +{
> +       fexit_called = skb->len;
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.26.2
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ