* [PATCH v2 bpf-next 0/2] bpf: Fix fill_link_info and add selftest @ 2023-07-27 11:43 Yafang Shao 2023-07-27 11:43 ` [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() Yafang Shao 2023-07-27 11:43 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info Yafang Shao 0 siblings, 2 replies; 7+ messages in thread From: Yafang Shao @ 2023-07-27 11:43 UTC (permalink / raw) To: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, jolsa Cc: bpf, Yafang Shao Patch #1: Fix an error in fill_link_info reported by Dan Patch #2: Add selftest for #1 Yafang Shao (2): bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() selftests/bpf: Add selftest for fill_link_info v1->v2: - Fix BPF CI failure due to the enabled ENDBDR kernel/bpf/syscall.c | 10 +- .../selftests/bpf/prog_tests/fill_link_info.c | 238 +++++++++++++++++++++ .../selftests/bpf/progs/test_fill_link_info.c | 25 +++ tools/testing/selftests/bpf/test_progs.h | 19 ++ 4 files changed, 287 insertions(+), 5 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/fill_link_info.c create mode 100644 tools/testing/selftests/bpf/progs/test_fill_link_info.c -- 1.8.3.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() 2023-07-27 11:43 [PATCH v2 bpf-next 0/2] bpf: Fix fill_link_info and add selftest Yafang Shao @ 2023-07-27 11:43 ` Yafang Shao 2023-07-27 13:07 ` Jiri Olsa 2023-07-27 11:43 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info Yafang Shao 1 sibling, 1 reply; 7+ messages in thread From: Yafang Shao @ 2023-07-27 11:43 UTC (permalink / raw) To: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, jolsa Cc: bpf, Yafang Shao, Dan Carpenter The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for perf_event" from Jul 9, 2023, leads to the following Smatch static checker warning: kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe() error: uninitialized symbol 'type'. That can happens when uname is NULL. So fix it by verifying the uname when we really need to fill it. Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/ Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- kernel/bpf/syscall.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 7f4e8c3..ad9360d 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event, size_t len; int err; - if (!ulen ^ !uname) - return -EINVAL; - if (!uname) - return 0; - err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf, probe_offset, probe_addr); if (err) return err; + if (!ulen ^ !uname) + return -EINVAL; + if (!uname) + return 0; + if (buf) { len = strlen(buf); err = bpf_copy_to_user(uname, buf, ulen, len); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() 2023-07-27 11:43 ` [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() Yafang Shao @ 2023-07-27 13:07 ` Jiri Olsa 2023-07-27 14:25 ` Yafang Shao 0 siblings, 1 reply; 7+ messages in thread From: Jiri Olsa @ 2023-07-27 13:07 UTC (permalink / raw) To: Yafang Shao Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, bpf, Dan Carpenter On Thu, Jul 27, 2023 at 11:43:08AM +0000, Yafang Shao wrote: > The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for > perf_event" from Jul 9, 2023, leads to the following Smatch static > checker warning: > > kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe() > error: uninitialized symbol 'type'. > > That can happens when uname is NULL. So fix it by verifying the uname > when we really need to fill it. > > Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/ > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > kernel/bpf/syscall.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 7f4e8c3..ad9360d 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event, > size_t len; > int err; > > - if (!ulen ^ !uname) > - return -EINVAL; would it make more sense to keep above check in place and move the !uname change below? I'd think we want to return error in case of wrong arguments as soon as possible > - if (!uname) > - return 0; > - > err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf, > probe_offset, probe_addr); > if (err) > return err; > > + if (!ulen ^ !uname) > + return -EINVAL; > + if (!uname) > + return 0; and here we just return 0 if we do not store the name to provided buffer thanks, jirka > + > if (buf) { > len = strlen(buf); > err = bpf_copy_to_user(uname, buf, ulen, len); > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() 2023-07-27 13:07 ` Jiri Olsa @ 2023-07-27 14:25 ` Yafang Shao 0 siblings, 0 replies; 7+ messages in thread From: Yafang Shao @ 2023-07-27 14:25 UTC (permalink / raw) To: Jiri Olsa Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, bpf, Dan Carpenter On Thu, Jul 27, 2023 at 9:07 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Thu, Jul 27, 2023 at 11:43:08AM +0000, Yafang Shao wrote: > > The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for > > perf_event" from Jul 9, 2023, leads to the following Smatch static > > checker warning: > > > > kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe() > > error: uninitialized symbol 'type'. > > > > That can happens when uname is NULL. So fix it by verifying the uname > > when we really need to fill it. > > > > Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/ > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > kernel/bpf/syscall.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 7f4e8c3..ad9360d 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event, > > size_t len; > > int err; > > > > - if (!ulen ^ !uname) > > - return -EINVAL; > > would it make more sense to keep above check in place and move the > !uname change below? I'd think we want to return error in case of > wrong arguments as soon as possible Good point. Will change it. > > > - if (!uname) > > - return 0; > > - > > err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf, > > probe_offset, probe_addr); > > if (err) > > return err; > > > > + if (!ulen ^ !uname) > > + return -EINVAL; > > + if (!uname) > > + return 0; > > and here we just return 0 if we do not store the name to provided buffer Will do it. -- Regards Yafang ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info 2023-07-27 11:43 [PATCH v2 bpf-next 0/2] bpf: Fix fill_link_info and add selftest Yafang Shao 2023-07-27 11:43 ` [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() Yafang Shao @ 2023-07-27 11:43 ` Yafang Shao 2023-07-27 13:49 ` Jiri Olsa 1 sibling, 1 reply; 7+ messages in thread From: Yafang Shao @ 2023-07-27 11:43 UTC (permalink / raw) To: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, jolsa Cc: bpf, Yafang Shao Add selftest for the fill_link_info of uprobe, kprobe and tracepoint. The result: #78/1 fill_link_info/kprobe_link_info:OK #78/2 fill_link_info/kretprobe_link_info:OK #78/3 fill_link_info/fill_invalid_user_buff:OK #78/4 fill_link_info/tracepoint_link_info:OK #78/5 fill_link_info/uprobe_link_info:OK #78/6 fill_link_info/uretprobe_link_info:OK #78 fill_link_info:OK Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- .../selftests/bpf/prog_tests/fill_link_info.c | 238 +++++++++++++++++++++ .../selftests/bpf/progs/test_fill_link_info.c | 25 +++ tools/testing/selftests/bpf/test_progs.h | 19 ++ 3 files changed, 282 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/fill_link_info.c create mode 100644 tools/testing/selftests/bpf/progs/test_fill_link_info.c diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c new file mode 100644 index 0000000..acb16b5 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c @@ -0,0 +1,238 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2023 Yafang Shao <laoar.shao@gmail.com> */ + +#include <string.h> +#include <linux/bpf.h> +#include <linux/limits.h> +#include <test_progs.h> +#include "trace_helpers.h" +#include "test_fill_link_info.skel.h" + +#define TP_CAT "sched" +#define TP_NAME "sched_switch" +#define KPROBE_FUNC "tcp_rcv_established" +#define UPROBE_FILE "/proc/self/exe" + +/* uprobe attach point */ +static noinline void uprobe_func(void) +{ + asm volatile (""); +} + +static int verify_link_info(int fd, enum bpf_perf_event_type type, long addr, ssize_t offset) +{ + struct bpf_link_info info; + __u32 len = sizeof(info); + char buf[PATH_MAX]; + int err = 0; + + memset(&info, 0, sizeof(info)); + buf[0] = '\0'; + +again: + err = bpf_link_get_info_by_fd(fd, &info, &len); + if (!ASSERT_OK(err, "get_link_info")) + return -1; + + switch (info.type) { + case BPF_LINK_TYPE_PERF_EVENT: + if (!ASSERT_EQ(info.perf_event.type, type, "perf_type_match")) + return -1; + + switch (info.perf_event.type) { + case BPF_PERF_EVENT_KPROBE: + case BPF_PERF_EVENT_KRETPROBE: + ASSERT_EQ(info.perf_event.kprobe.offset, offset, "kprobe_offset"); + + /* In case kptr setting is not permitted or MAX_SYMS is reached */ + if (addr) { + long addrs[2] = { + addr + offset, + addr + 0x4, /* For ENDBDR */ + }; + + ASSERT_IN_ARRAY(info.perf_event.kprobe.addr, addrs, "kprobe_addr"); + } + + if (!info.perf_event.kprobe.func_name) { + ASSERT_EQ(info.perf_event.kprobe.name_len, 0, "name_len"); + info.perf_event.kprobe.func_name = ptr_to_u64(&buf); + info.perf_event.kprobe.name_len = sizeof(buf); + goto again; + } + + err = strncmp(u64_to_ptr(info.perf_event.kprobe.func_name), KPROBE_FUNC, + strlen(KPROBE_FUNC)); + ASSERT_EQ(err, 0, "cmp_kprobe_func_name"); + break; + case BPF_PERF_EVENT_TRACEPOINT: + if (!info.perf_event.tracepoint.tp_name) { + ASSERT_EQ(info.perf_event.tracepoint.name_len, 0, "name_len"); + info.perf_event.tracepoint.tp_name = ptr_to_u64(&buf); + info.perf_event.tracepoint.name_len = sizeof(buf); + goto again; + } + + err = strncmp(u64_to_ptr(info.perf_event.tracepoint.tp_name), TP_NAME, + strlen(TP_NAME)); + ASSERT_EQ(err, 0, "cmp_tp_name"); + break; + case BPF_PERF_EVENT_UPROBE: + case BPF_PERF_EVENT_URETPROBE: + ASSERT_EQ(info.perf_event.uprobe.offset, offset, "uprobe_offset"); + + if (!info.perf_event.uprobe.file_name) { + ASSERT_EQ(info.perf_event.uprobe.name_len, 0, "name_len"); + info.perf_event.uprobe.file_name = ptr_to_u64(&buf); + info.perf_event.uprobe.name_len = sizeof(buf); + goto again; + } + + err = strncmp(u64_to_ptr(info.perf_event.uprobe.file_name), UPROBE_FILE, + strlen(UPROBE_FILE)); + ASSERT_EQ(err, 0, "cmp_file_name"); + break; + default: + break; + } + break; + default: + switch (type) { + case BPF_PERF_EVENT_KPROBE: + case BPF_PERF_EVENT_KRETPROBE: + case BPF_PERF_EVENT_TRACEPOINT: + case BPF_PERF_EVENT_UPROBE: + case BPF_PERF_EVENT_URETPROBE: + err = -1; + break; + default: + break; + } + break; + } + return err; +} + +static void kprobe_fill_invalid_user_buffer(int fd) +{ + struct bpf_link_info info; + __u32 len = sizeof(info); + int err = 0; + + memset(&info, 0, sizeof(info)); + + info.perf_event.kprobe.func_name = 0x1; /* invalid address */ + err = bpf_link_get_info_by_fd(fd, &info, &len); + ASSERT_EQ(err, -EINVAL, "invalid_buff_and_len"); + + info.perf_event.kprobe.name_len = 64; + err = bpf_link_get_info_by_fd(fd, &info, &len); + ASSERT_EQ(err, -EFAULT, "invalid_buff"); + + info.perf_event.kprobe.func_name = 0; + err = bpf_link_get_info_by_fd(fd, &info, &len); + ASSERT_EQ(err, -EINVAL, "invalid_len"); + + ASSERT_EQ(info.perf_event.kprobe.addr, 0, "func_addr"); + ASSERT_EQ(info.perf_event.kprobe.offset, 0, "func_offset"); + ASSERT_EQ(info.perf_event.type, 0, "type"); +} + +static void test_kprobe_fill_link_info(struct test_fill_link_info *skel, + enum bpf_perf_event_type type, + bool retprobe, bool invalid) +{ + DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts, + .attach_mode = PROBE_ATTACH_MODE_LINK, + .retprobe = retprobe, + ); + int link_fd, err; + long addr; + + skel->links.kprobe_run = bpf_program__attach_kprobe_opts(skel->progs.kprobe_run, + KPROBE_FUNC, &opts); + if (!ASSERT_OK_PTR(skel->links.kprobe_run, "attach_kprobe")) + return; + + link_fd = bpf_link__fd(skel->links.kprobe_run); + if (!ASSERT_GE(link_fd, 0, "link_fd")) + return; + + addr = ksym_get_addr(KPROBE_FUNC); + if (!invalid) { + err = verify_link_info(link_fd, type, addr, 0); + ASSERT_OK(err, "verify_link_info"); + } else { + kprobe_fill_invalid_user_buffer(link_fd); + } + bpf_link__detach(skel->links.kprobe_run); +} + +static void test_tp_fill_link_info(struct test_fill_link_info *skel) +{ + int link_fd, err; + + skel->links.tp_run = bpf_program__attach_tracepoint(skel->progs.tp_run, TP_CAT, TP_NAME); + if (!ASSERT_OK_PTR(skel->links.tp_run, "attach_tp")) + return; + + link_fd = bpf_link__fd(skel->links.tp_run); + if (!ASSERT_GE(link_fd, 0, "link_fd")) + return; + + err = verify_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0); + ASSERT_OK(err, "verify_link_info"); + bpf_link__detach(skel->links.tp_run); +} + +static void test_uprobe_fill_link_info(struct test_fill_link_info *skel, + enum bpf_perf_event_type type, ssize_t offset, + bool retprobe) +{ + int link_fd, err; + + skel->links.uprobe_run = bpf_program__attach_uprobe(skel->progs.uprobe_run, retprobe, + 0, /* self pid */ + UPROBE_FILE, offset); + if (!ASSERT_OK_PTR(skel->links.uprobe_run, "attach_uprobe")) + return; + + link_fd = bpf_link__fd(skel->links.uprobe_run); + if (!ASSERT_GE(link_fd, 0, "link_fd")) + return; + + err = verify_link_info(link_fd, type, 0, offset); + ASSERT_OK(err, "verify_link_info"); + bpf_link__detach(skel->links.uprobe_run); +} + +void serial_test_fill_link_info(void) +{ + struct test_fill_link_info *skel; + ssize_t offset; + + skel = test_fill_link_info__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + goto cleanup; + + /* load kallsyms to compare the addr */ + if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh")) + return; + if (test__start_subtest("kprobe_link_info")) + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, false); + if (test__start_subtest("kretprobe_link_info")) + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KRETPROBE, true, false); + if (test__start_subtest("fill_invalid_user_buff")) + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, true); + if (test__start_subtest("tracepoint_link_info")) + test_tp_fill_link_info(skel); + + offset = get_uprobe_offset(&uprobe_func); + if (test__start_subtest("uprobe_link_info")) + test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_UPROBE, offset, false); + if (test__start_subtest("uretprobe_link_info")) + test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_URETPROBE, offset, true); + +cleanup: + test_fill_link_info__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_fill_link_info.c b/tools/testing/selftests/bpf/progs/test_fill_link_info.c new file mode 100644 index 0000000..f776134 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_fill_link_info.c @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2023 Yafang Shao <laoar.shao@gmail.com> */ + +#include "vmlinux.h" +#include <bpf/bpf_tracing.h> + +SEC("kprobe") +int BPF_PROG(kprobe_run) +{ + return 0; +} + +SEC("uprobe") +int BPF_PROG(uprobe_run) +{ + return 0; +} + +SEC("tracepoint") +int BPF_PROG(tp_run) +{ + return 0; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 77bd492..b51c822 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -300,6 +300,25 @@ struct msg { ___ok; \ }) +#define ASSERT_IN_ARRAY(actual, expected, name) ({ \ + static int duration; \ + typeof(actual) ___act = (actual); \ + typeof((expected)[0]) * ___exp = (expected); \ + bool ___ok = false; \ + int i; \ + \ + for (i = 0; i < ARRAY_SIZE(expected); i++) { \ + if (___act == ___exp[i]) { \ + ___ok = true; \ + break; \ + } \ + } \ + CHECK(!___ok, (name), \ + "unexpected %s: actual %lld not in array\n", \ + (name), (long long)(___act)); \ + ___ok; \ +}) + #define ASSERT_STREQ(actual, expected, name) ({ \ static int duration = 0; \ const char *___act = actual; \ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info 2023-07-27 11:43 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info Yafang Shao @ 2023-07-27 13:49 ` Jiri Olsa 2023-07-27 14:27 ` Yafang Shao 0 siblings, 1 reply; 7+ messages in thread From: Jiri Olsa @ 2023-07-27 13:49 UTC (permalink / raw) To: Yafang Shao Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, bpf On Thu, Jul 27, 2023 at 11:43:09AM +0000, Yafang Shao wrote: SNIP > +static int verify_link_info(int fd, enum bpf_perf_event_type type, long addr, ssize_t offset) > +{ > + struct bpf_link_info info; > + __u32 len = sizeof(info); > + char buf[PATH_MAX]; > + int err = 0; > + > + memset(&info, 0, sizeof(info)); > + buf[0] = '\0'; > + > +again: > + err = bpf_link_get_info_by_fd(fd, &info, &len); > + if (!ASSERT_OK(err, "get_link_info")) > + return -1; > + > + switch (info.type) { > + case BPF_LINK_TYPE_PERF_EVENT: > + if (!ASSERT_EQ(info.perf_event.type, type, "perf_type_match")) > + return -1; > + > + switch (info.perf_event.type) { > + case BPF_PERF_EVENT_KPROBE: > + case BPF_PERF_EVENT_KRETPROBE: > + ASSERT_EQ(info.perf_event.kprobe.offset, offset, "kprobe_offset"); > + > + /* In case kptr setting is not permitted or MAX_SYMS is reached */ > + if (addr) { > + long addrs[2] = { > + addr + offset, > + addr + 0x4, /* For ENDBDR */ > + }; > + > + ASSERT_IN_ARRAY(info.perf_event.kprobe.addr, addrs, "kprobe_addr"); we have check for IBT in get_func_ip_test, it might be easier to use the same in here as well and do the exact check we wouldn't need the ASSERT_IN_ARRAY then and would be correct wrt other archs SNIP > +static void test_uprobe_fill_link_info(struct test_fill_link_info *skel, > + enum bpf_perf_event_type type, ssize_t offset, > + bool retprobe) > +{ > + int link_fd, err; > + > + skel->links.uprobe_run = bpf_program__attach_uprobe(skel->progs.uprobe_run, retprobe, > + 0, /* self pid */ > + UPROBE_FILE, offset); > + if (!ASSERT_OK_PTR(skel->links.uprobe_run, "attach_uprobe")) > + return; > + > + link_fd = bpf_link__fd(skel->links.uprobe_run); > + if (!ASSERT_GE(link_fd, 0, "link_fd")) > + return; > + > + err = verify_link_info(link_fd, type, 0, offset); > + ASSERT_OK(err, "verify_link_info"); > + bpf_link__detach(skel->links.uprobe_run); > +} > + > +void serial_test_fill_link_info(void) why does it need to be serial? > +{ > + struct test_fill_link_info *skel; > + ssize_t offset; > + > + skel = test_fill_link_info__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "skel_open")) > + goto cleanup; > + > + /* load kallsyms to compare the addr */ > + if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh")) > + return; > + if (test__start_subtest("kprobe_link_info")) > + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, false); > + if (test__start_subtest("kretprobe_link_info")) > + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KRETPROBE, true, false); > + if (test__start_subtest("fill_invalid_user_buff")) > + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, true); > + if (test__start_subtest("tracepoint_link_info")) > + test_tp_fill_link_info(skel); > + > + offset = get_uprobe_offset(&uprobe_func); > + if (test__start_subtest("uprobe_link_info")) > + test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_UPROBE, offset, false); > + if (test__start_subtest("uretprobe_link_info")) > + test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_URETPROBE, offset, true); do you plan to add kprobe_multi link test as well? thanks, jirka > + > +cleanup: > + test_fill_link_info__destroy(skel); > +} SNIP ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info 2023-07-27 13:49 ` Jiri Olsa @ 2023-07-27 14:27 ` Yafang Shao 0 siblings, 0 replies; 7+ messages in thread From: Yafang Shao @ 2023-07-27 14:27 UTC (permalink / raw) To: Jiri Olsa Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs, kpsingh, sdf, haoluo, bpf On Thu, Jul 27, 2023 at 9:49 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Thu, Jul 27, 2023 at 11:43:09AM +0000, Yafang Shao wrote: > > SNIP > > > +static int verify_link_info(int fd, enum bpf_perf_event_type type, long addr, ssize_t offset) > > +{ > > + struct bpf_link_info info; > > + __u32 len = sizeof(info); > > + char buf[PATH_MAX]; > > + int err = 0; > > + > > + memset(&info, 0, sizeof(info)); > > + buf[0] = '\0'; > > + > > +again: > > + err = bpf_link_get_info_by_fd(fd, &info, &len); > > + if (!ASSERT_OK(err, "get_link_info")) > > + return -1; > > + > > + switch (info.type) { > > + case BPF_LINK_TYPE_PERF_EVENT: > > + if (!ASSERT_EQ(info.perf_event.type, type, "perf_type_match")) > > + return -1; > > + > > + switch (info.perf_event.type) { > > + case BPF_PERF_EVENT_KPROBE: > > + case BPF_PERF_EVENT_KRETPROBE: > > + ASSERT_EQ(info.perf_event.kprobe.offset, offset, "kprobe_offset"); > > + > > + /* In case kptr setting is not permitted or MAX_SYMS is reached */ > > + if (addr) { > > + long addrs[2] = { > > + addr + offset, > > + addr + 0x4, /* For ENDBDR */ > > + }; > > + > > + ASSERT_IN_ARRAY(info.perf_event.kprobe.addr, addrs, "kprobe_addr"); > > we have check for IBT in get_func_ip_test, it might be easier > to use the same in here as well and do the exact check Thanks for your information! will change it. > > we wouldn't need the ASSERT_IN_ARRAY then and would be correct > wrt other archs > > > SNIP > > > +static void test_uprobe_fill_link_info(struct test_fill_link_info *skel, > > + enum bpf_perf_event_type type, ssize_t offset, > > + bool retprobe) > > +{ > > + int link_fd, err; > > + > > + skel->links.uprobe_run = bpf_program__attach_uprobe(skel->progs.uprobe_run, retprobe, > > + 0, /* self pid */ > > + UPROBE_FILE, offset); > > + if (!ASSERT_OK_PTR(skel->links.uprobe_run, "attach_uprobe")) > > + return; > > + > > + link_fd = bpf_link__fd(skel->links.uprobe_run); > > + if (!ASSERT_GE(link_fd, 0, "link_fd")) > > + return; > > + > > + err = verify_link_info(link_fd, type, 0, offset); > > + ASSERT_OK(err, "verify_link_info"); > > + bpf_link__detach(skel->links.uprobe_run); > > +} > > + > > +void serial_test_fill_link_info(void) > > why does it need to be serial? Ah, it can run in parallel. will change it. > > > +{ > > + struct test_fill_link_info *skel; > > + ssize_t offset; > > + > > + skel = test_fill_link_info__open_and_load(); > > + if (!ASSERT_OK_PTR(skel, "skel_open")) > > + goto cleanup; > > + > > + /* load kallsyms to compare the addr */ > > + if (!ASSERT_OK(load_kallsyms_refresh(), "load_kallsyms_refresh")) > > + return; > > + if (test__start_subtest("kprobe_link_info")) > > + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, false); > > + if (test__start_subtest("kretprobe_link_info")) > > + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KRETPROBE, true, false); > > + if (test__start_subtest("fill_invalid_user_buff")) > > + test_kprobe_fill_link_info(skel, BPF_PERF_EVENT_KPROBE, false, true); > > + if (test__start_subtest("tracepoint_link_info")) > > + test_tp_fill_link_info(skel); > > + > > + offset = get_uprobe_offset(&uprobe_func); > > + if (test__start_subtest("uprobe_link_info")) > > + test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_UPROBE, offset, false); > > + if (test__start_subtest("uretprobe_link_info")) > > + test_uprobe_fill_link_info(skel, BPF_PERF_EVENT_URETPROBE, offset, true); > > do you plan to add kprobe_multi link test as well? will add it in the next version. -- Regards Yafang ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-07-27 14:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-27 11:43 [PATCH v2 bpf-next 0/2] bpf: Fix fill_link_info and add selftest Yafang Shao 2023-07-27 11:43 ` [PATCH v2 bpf-next 1/2] bpf: Fix uninitialized symbol in bpf_perf_link_fill_kprobe() Yafang Shao 2023-07-27 13:07 ` Jiri Olsa 2023-07-27 14:25 ` Yafang Shao 2023-07-27 11:43 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftest for fill_link_info Yafang Shao 2023-07-27 13:49 ` Jiri Olsa 2023-07-27 14:27 ` Yafang Shao
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.