| Message ID | 1649195156-9465-3-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: uprobe name-based attach followups | expand |
On Tue, Apr 5, 2022 at 2:46 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > uprobe/uretprobe tests don't do any validation of arguments/return values, > and without this we can't be sure we are attached to the right function, > or that we are indeed attached to a uprobe or uretprobe. To fix this > validate argument and return value for auto-attached functions. > > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > .../selftests/bpf/prog_tests/uprobe_autoattach.c | 16 ++++++++++---- > .../selftests/bpf/progs/test_uprobe_autoattach.c | 25 +++++++++++++++------- > 2 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > index 03b15d6..ff85f1f 100644 > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c > @@ -5,14 +5,16 @@ > #include "test_uprobe_autoattach.skel.h" > > /* uprobe attach point */ > -static void autoattach_trigger_func(void) > +static noinline int autoattach_trigger_func(int arg) > { > - asm volatile (""); don't remove asm volatile, with static functions compiler can still do function transformations and stuff. From GCC documentation for noinline attribute ([0]): noinline This function attribute prevents a function from being considered for inlining. If the function does not have side effects, there are optimizations other than inlining that cause function calls to be optimized away, although the function call is live. To keep such calls from being optimized away, put asm (""); [0] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes So I suppose noinline + asm volatile is the most safe combination :) for static functions > + return arg + 1; > } > > void test_uprobe_autoattach(void) > { > struct test_uprobe_autoattach *skel; > + int trigger_val = 100; > + size_t malloc_sz = 1; > char *mem; > > skel = test_uprobe_autoattach__open_and_load(); [...] > > -SEC("uretprobe/libc.so.6:free") > +SEC("uretprobe/libc.so.6:getpid") > int handle_uretprobe_byname2(struct pt_regs *ctx) > { > - uretprobe_byname2_res = 4; > + if (PT_REGS_RC_CORE(ctx) == uretprobe_byname2_rc) > + uretprobe_byname2_res = 4; Instead of checking equality on BPF side, it's more convenient to record the actual captured value into global variable and let user-space part check and log it properly. So if something goes wrong, we don't just have matched/not matched flag, but we see an actual value captured. With that, let's better switch uretprobe from free to malloc (we additionally check that uprobe and uretprobe can coexist properly on the same function) and capture returned pointer from malloc(). We can then compare that pointer to the mem variable. How cool is that? :) > return 0; > } > > -- > 1.8.3.1 >
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c index 03b15d6..ff85f1f 100644 --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c @@ -5,14 +5,16 @@ #include "test_uprobe_autoattach.skel.h" /* uprobe attach point */ -static void autoattach_trigger_func(void) +static noinline int autoattach_trigger_func(int arg) { - asm volatile (""); + return arg + 1; } void test_uprobe_autoattach(void) { struct test_uprobe_autoattach *skel; + int trigger_val = 100; + size_t malloc_sz = 1; char *mem; skel = test_uprobe_autoattach__open_and_load(); @@ -22,12 +24,18 @@ void test_uprobe_autoattach(void) if (!ASSERT_OK(test_uprobe_autoattach__attach(skel), "skel_attach")) goto cleanup; + skel->bss->uprobe_byname_parm1 = trigger_val; + skel->bss->uretprobe_byname_rc = trigger_val + 1; + skel->bss->uprobe_byname2_parm1 = malloc_sz; + skel->bss->uretprobe_byname2_rc = getpid(); + /* trigger & validate uprobe & uretprobe */ - autoattach_trigger_func(); + (void) autoattach_trigger_func(trigger_val); /* trigger & validate shared library u[ret]probes attached by name */ - mem = malloc(1); + mem = malloc(malloc_sz); free(mem); + (void) getpid(); ASSERT_EQ(skel->bss->uprobe_byname_res, 1, "check_uprobe_byname_res"); ASSERT_EQ(skel->bss->uretprobe_byname_res, 2, "check_uretprobe_byname_res"); diff --git a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c index b442fb5..db104bd 100644 --- a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c +++ b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c @@ -1,15 +1,20 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2022, Oracle and/or its affiliates. */ -#include <linux/ptrace.h> -#include <linux/bpf.h> +#include "vmlinux.h" + +#include <bpf/bpf_core_read.h> #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> +int uprobe_byname_parm1 = 0; int uprobe_byname_res = 0; +int uretprobe_byname_rc = 0; int uretprobe_byname_res = 0; +size_t uprobe_byname2_parm1 = 0; int uprobe_byname2_res = 0; -int uretprobe_byname2_res = 0; +int uretprobe_byname2_rc = 0; +pid_t uretprobe_byname2_res = 0; /* This program cannot auto-attach, but that should not stop other * programs from attaching. @@ -23,14 +28,16 @@ int handle_uprobe_noautoattach(struct pt_regs *ctx) SEC("uprobe//proc/self/exe:autoattach_trigger_func") int handle_uprobe_byname(struct pt_regs *ctx) { - uprobe_byname_res = 1; + if (PT_REGS_PARM1_CORE(ctx) == uprobe_byname_parm1) + uprobe_byname_res = 1; return 0; } SEC("uretprobe//proc/self/exe:autoattach_trigger_func") int handle_uretprobe_byname(struct pt_regs *ctx) { - uretprobe_byname_res = 2; + if (PT_REGS_RC_CORE(ctx) == uretprobe_byname_rc) + uretprobe_byname_res = 2; return 0; } @@ -38,14 +45,16 @@ int handle_uretprobe_byname(struct pt_regs *ctx) SEC("uprobe/libc.so.6:malloc") int handle_uprobe_byname2(struct pt_regs *ctx) { - uprobe_byname2_res = 3; + if (PT_REGS_PARM1_CORE(ctx) == uprobe_byname2_parm1) + uprobe_byname2_res = 3; return 0; } -SEC("uretprobe/libc.so.6:free") +SEC("uretprobe/libc.so.6:getpid") int handle_uretprobe_byname2(struct pt_regs *ctx) { - uretprobe_byname2_res = 4; + if (PT_REGS_RC_CORE(ctx) == uretprobe_byname2_rc) + uretprobe_byname2_res = 4; return 0; }
uprobe/uretprobe tests don't do any validation of arguments/return values, and without this we can't be sure we are attached to the right function, or that we are indeed attached to a uprobe or uretprobe. To fix this validate argument and return value for auto-attached functions. Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> --- .../selftests/bpf/prog_tests/uprobe_autoattach.c | 16 ++++++++++---- .../selftests/bpf/progs/test_uprobe_autoattach.c | 25 +++++++++++++++------- 2 files changed, 29 insertions(+), 12 deletions(-)