| Message ID | 20200909151115.1559418-2-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Fix context type resolving for extension programs | expand |
On Wed, Sep 9, 2020 at 8:38 AM Jiri Olsa <jolsa@kernel.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@kernel.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 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Sep 9, 2020 at 8:38 AM Jiri Olsa <jolsa@kernel.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@kernel.org> Just FYI, I included this same patch in my freplace series. I didn't change anything in the version I just resent, but I'll work with Jiri and get an updated version of this into the next version based on your comments here... :) -Toke
On Thu, Sep 10, 2020 at 03:34:26PM -0700, Andrii Nakryiko wrote: SNIP > > + > > +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()? ok > > > + > > + 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? right, I'll try it, it should be enough SNIP > > + > > +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 ok, thanks jirka
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; + + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts, + .attach_prog_fd = prog_fd, + ); + + 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, + ); + + 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; + +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";
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@kernel.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