* [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups
@ 2022-04-06 11:43 Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 1/3] libbpf: improve library identification for uprobe binary path resolution Alan Maguire
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Alan Maguire @ 2022-04-06 11:43 UTC (permalink / raw)
To: andrii, ast, daniel
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf,
Alan Maguire
Follow-up series to [1] to address some suggestions from Andrii to
improve parsing and make it more robust (patches 1, 2) and to improve
validation of u[ret]probe firing by validating expected argument
and return values (patch 3).
[1] https://lore.kernel.org/bpf/164903521182.13106.12656654142629368774.git-patchwork-notify@kernel.org/
Changes since v1:
- split library name, auto-attach parsing into separate patches (Andrii, patches 1, 2)
- made str_has_sfx() static inline, avoided repeated strlen()s by storing lengths,
used strlen() instead of strnlen() (Andrii, patch 1)
- fixed sscanf() arg to use %li, switched logging to use "prog '%s'" format,
used direct strcmp() on probe_type instead of prefix check (Andrii, patch 2)
- switched auto-attach tests to log parameter/return values to be checked by
user-space side of tests. Needed to add pid filtering to avoid capturing
stray malloc()s (Andrii, patch 3)
Alan Maguire (3):
libbpf: improve library identification for uprobe binary path
resolution
libbpf: improve string parsing for uprobe auto-attach
selftests/bpf: uprobe tests should verify param/return values
tools/lib/bpf/libbpf.c | 85 +++++++++-------------
tools/lib/bpf/libbpf_internal.h | 11 +++
.../selftests/bpf/prog_tests/uprobe_autoattach.c | 25 +++++--
.../selftests/bpf/progs/test_uprobe_autoattach.c | 43 ++++++++---
4 files changed, 96 insertions(+), 68 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 bpf-next 1/3] libbpf: improve library identification for uprobe binary path resolution
2022-04-06 11:43 [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups Alan Maguire
@ 2022-04-06 11:43 ` Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 2/3] libbpf: improve string parsing for uprobe auto-attach Alan Maguire
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Alan Maguire @ 2022-04-06 11:43 UTC (permalink / raw)
To: andrii, ast, daniel
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf,
Alan Maguire
In the process of doing path resolution for uprobe attach, libraries are
identified by matching a ".so" substring in the binary_path.
This matches a lot of patterns that do not conform to library.so[.version]
format, so instead match a ".so" _suffix_, and if that fails match a
".so." substring for the versioned library case.
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/libbpf.c | 2 +-
tools/lib/bpf/libbpf_internal.h | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1111e9d..c92226a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10766,7 +10766,7 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
const char *search_paths[3] = {};
int i;
- if (strstr(file, ".so")) {
+ if (str_has_sfx(file, ".so") || strstr(file, ".so.")) {
search_paths[0] = getenv("LD_LIBRARY_PATH");
search_paths[1] = "/usr/lib64:/usr/lib";
search_paths[2] = arch_specific_lib_paths();
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index dd0d4cc..0802724 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -103,6 +103,17 @@
#define str_has_pfx(str, pfx) \
(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
+/* suffix check */
+static inline bool str_has_sfx(const char *str, const char *sfx)
+{
+ size_t str_len = strlen(str);
+ size_t sfx_len = strlen(sfx);
+
+ if (sfx_len <= str_len)
+ return strcmp(str + str_len - sfx_len, sfx);
+ return false;
+}
+
/* Symbol versioning is different between static and shared library.
* Properly versioned symbols are needed for shared library, but
* only the symbol of the new version is needed for static library.
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 bpf-next 2/3] libbpf: improve string parsing for uprobe auto-attach
2022-04-06 11:43 [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 1/3] libbpf: improve library identification for uprobe binary path resolution Alan Maguire
@ 2022-04-06 11:43 ` Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 3/3] selftests/bpf: uprobe tests should verify param/return values Alan Maguire
2022-04-07 18:50 ` [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Alan Maguire @ 2022-04-06 11:43 UTC (permalink / raw)
To: andrii, ast, daniel
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf,
Alan Maguire
For uprobe auto-attach, the parsing can be simplified for the SEC()
name to a single sscanf(); the return value of the sscanf can then
be used to distinguish between sections that simply specify
"u[ret]probe" (and thus cannot auto-attach), those that specify
"u[ret]probe/binary_path:function+offset" etc.
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/libbpf.c | 83 +++++++++++++++++++++-----------------------------
1 file changed, 34 insertions(+), 49 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c92226a..707dcc3 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10913,60 +10913,45 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
static int attach_uprobe(const struct bpf_program *prog, long cookie, struct bpf_link **link)
{
DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, opts);
- char *func, *probe_name, *func_end;
- char *func_name, binary_path[512];
- unsigned long long raw_offset;
- size_t offset = 0;
- int n;
+ char *probe_type = NULL, *binary_path = NULL, *func_name = NULL;
+ int n, ret = -EINVAL;
+ long offset = 0;
*link = NULL;
- opts.retprobe = str_has_pfx(prog->sec_name, "uretprobe");
- if (opts.retprobe)
- probe_name = prog->sec_name + sizeof("uretprobe") - 1;
- else
- probe_name = prog->sec_name + sizeof("uprobe") - 1;
- if (probe_name[0] == '/')
- probe_name++;
-
- /* handle SEC("u[ret]probe") - format is valid, but auto-attach is impossible. */
- if (strlen(probe_name) == 0)
- return 0;
-
- snprintf(binary_path, sizeof(binary_path), "%s", probe_name);
- /* ':' should be prior to function+offset */
- func_name = strrchr(binary_path, ':');
- if (!func_name) {
- pr_warn("section '%s' missing ':function[+offset]' specification\n",
- prog->sec_name);
- return -EINVAL;
- }
- func_name[0] = '\0';
- func_name++;
- n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
- if (n < 1) {
- pr_warn("uprobe name '%s' is invalid\n", func_name);
- return -EINVAL;
- }
- if (opts.retprobe && offset != 0) {
- free(func);
- pr_warn("uretprobes do not support offset specification\n");
- return -EINVAL;
- }
-
- /* Is func a raw address? */
- errno = 0;
- raw_offset = strtoull(func, &func_end, 0);
- if (!errno && !*func_end) {
- free(func);
- func = NULL;
- offset = (size_t)raw_offset;
+ n = sscanf(prog->sec_name, "%m[^/]/%m[^:]:%m[a-zA-Z0-9_.]+%li",
+ &probe_type, &binary_path, &func_name, &offset);
+ switch (n) {
+ case 1:
+ /* handle SEC("u[ret]probe") - format is valid, but auto-attach is impossible. */
+ ret = 0;
+ break;
+ case 2:
+ pr_warn("prog '%s': section '%s' missing ':function[+offset]' specification\n",
+ prog->name, prog->sec_name);
+ break;
+ case 3:
+ case 4:
+ opts.retprobe = strcmp(probe_type, "uretprobe") == 0;
+ if (opts.retprobe && offset != 0) {
+ pr_warn("prog '%s': uretprobes do not support offset specification\n",
+ prog->name);
+ break;
+ }
+ opts.func_name = func_name;
+ *link = bpf_program__attach_uprobe_opts(prog, -1, binary_path, offset, &opts);
+ ret = libbpf_get_error(*link);
+ break;
+ default:
+ pr_warn("prog '%s': invalid format of section definition '%s'\n", prog->name,
+ prog->sec_name);
+ break;
}
- opts.func_name = func;
+ free(probe_type);
+ free(binary_path);
+ free(func_name);
- *link = bpf_program__attach_uprobe_opts(prog, -1, binary_path, offset, &opts);
- free(func);
- return libbpf_get_error(*link);
+ return ret;
}
struct bpf_link *bpf_program__attach_uprobe(const struct bpf_program *prog,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 bpf-next 3/3] selftests/bpf: uprobe tests should verify param/return values
2022-04-06 11:43 [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 1/3] libbpf: improve library identification for uprobe binary path resolution Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 2/3] libbpf: improve string parsing for uprobe auto-attach Alan Maguire
@ 2022-04-06 11:43 ` Alan Maguire
2022-04-07 18:50 ` [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Alan Maguire @ 2022-04-06 11:43 UTC (permalink / raw)
To: andrii, ast, daniel
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf,
Alan Maguire
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
record argument and return value for auto-attached functions and ensure
these match expectations. Also need to filter by pid to ensure we do
not pick up stray malloc()s since auto-attach traces libc system-wide.
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
.../selftests/bpf/prog_tests/uprobe_autoattach.c | 25 +++++++++----
.../selftests/bpf/progs/test_uprobe_autoattach.c | 43 ++++++++++++++++------
2 files changed, 50 insertions(+), 18 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..d6003dc 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
@@ -5,14 +5,17 @@
#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, trigger_ret;
+ size_t malloc_sz = 1;
char *mem;
skel = test_uprobe_autoattach__open_and_load();
@@ -22,17 +25,25 @@ void test_uprobe_autoattach(void)
if (!ASSERT_OK(test_uprobe_autoattach__attach(skel), "skel_attach"))
goto cleanup;
+ skel->bss->test_pid = getpid();
+
/* trigger & validate uprobe & uretprobe */
- autoattach_trigger_func();
+ trigger_ret = autoattach_trigger_func(trigger_val);
+
+ skel->bss->test_pid = getpid();
/* trigger & validate shared library u[ret]probes attached by name */
- mem = malloc(1);
+ mem = malloc(malloc_sz);
free(mem);
- ASSERT_EQ(skel->bss->uprobe_byname_res, 1, "check_uprobe_byname_res");
- ASSERT_EQ(skel->bss->uretprobe_byname_res, 2, "check_uretprobe_byname_res");
- ASSERT_EQ(skel->bss->uprobe_byname2_res, 3, "check_uprobe_byname2_res");
- ASSERT_EQ(skel->bss->uretprobe_byname2_res, 4, "check_uretprobe_byname2_res");
+ ASSERT_EQ(skel->bss->uprobe_byname_parm1, trigger_val, "check_uprobe_byname_parm1");
+ ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran");
+ ASSERT_EQ(skel->bss->uretprobe_byname_rc, trigger_ret, "check_uretprobe_byname_rc");
+ ASSERT_EQ(skel->bss->uretprobe_byname_ran, 2, "check_uretprobe_byname_ran");
+ ASSERT_EQ(skel->bss->uprobe_byname2_parm1, malloc_sz, "check_uprobe_byname2_parm1");
+ ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran");
+ ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc");
+ ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran");
cleanup:
test_uprobe_autoattach__destroy(skel);
}
diff --git a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
index b442fb5..ab75522 100644
--- a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
+++ b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
@@ -1,15 +1,22 @@
// 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_res = 0;
-int uretprobe_byname_res = 0;
-int uprobe_byname2_res = 0;
-int uretprobe_byname2_res = 0;
+int uprobe_byname_parm1 = 0;
+int uprobe_byname_ran = 0;
+int uretprobe_byname_rc = 0;
+int uretprobe_byname_ran = 0;
+size_t uprobe_byname2_parm1 = 0;
+int uprobe_byname2_ran = 0;
+char *uretprobe_byname2_rc = NULL;
+int uretprobe_byname2_ran = 0;
+
+int test_pid;
/* This program cannot auto-attach, but that should not stop other
* programs from attaching.
@@ -23,14 +30,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;
+ uprobe_byname_parm1 = PT_REGS_PARM1_CORE(ctx);
+ uprobe_byname_ran = 1;
return 0;
}
SEC("uretprobe//proc/self/exe:autoattach_trigger_func")
int handle_uretprobe_byname(struct pt_regs *ctx)
{
- uretprobe_byname_res = 2;
+ uretprobe_byname_rc = PT_REGS_RC_CORE(ctx);
+ uretprobe_byname_ran = 2;
return 0;
}
@@ -38,14 +47,26 @@ 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;
+ int pid = bpf_get_current_pid_tgid() >> 32;
+
+ /* ignore irrelevant invocations */
+ if (test_pid != pid)
+ return 0;
+ uprobe_byname2_parm1 = PT_REGS_PARM1_CORE(ctx);
+ uprobe_byname2_ran = 3;
return 0;
}
-SEC("uretprobe/libc.so.6:free")
+SEC("uretprobe/libc.so.6:malloc")
int handle_uretprobe_byname2(struct pt_regs *ctx)
{
- uretprobe_byname2_res = 4;
+ int pid = bpf_get_current_pid_tgid() >> 32;
+
+ /* ignore irrelevant invocations */
+ if (test_pid != pid)
+ return 0;
+ uretprobe_byname2_rc = (char *)PT_REGS_RC_CORE(ctx);
+ uretprobe_byname2_ran = 4;
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups
2022-04-06 11:43 [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups Alan Maguire
` (2 preceding siblings ...)
2022-04-06 11:43 ` [PATCH v2 bpf-next 3/3] selftests/bpf: uprobe tests should verify param/return values Alan Maguire
@ 2022-04-07 18:50 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-07 18:50 UTC (permalink / raw)
To: Alan Maguire
Cc: andrii, ast, daniel, kafai, songliubraving, yhs, john.fastabend,
kpsingh, netdev, bpf
Hello:
This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Wed, 6 Apr 2022 12:43:48 +0100 you wrote:
> Follow-up series to [1] to address some suggestions from Andrii to
> improve parsing and make it more robust (patches 1, 2) and to improve
> validation of u[ret]probe firing by validating expected argument
> and return values (patch 3).
>
> [1] https://lore.kernel.org/bpf/164903521182.13106.12656654142629368774.git-patchwork-notify@kernel.org/
>
> [...]
Here is the summary with links:
- [v2,bpf-next,1/3] libbpf: improve library identification for uprobe binary path resolution
https://git.kernel.org/bpf/bpf-next/c/a1c9d61b19cb
- [v2,bpf-next,2/3] libbpf: improve string parsing for uprobe auto-attach
https://git.kernel.org/bpf/bpf-next/c/90db26e6be01
- [v2,bpf-next,3/3] selftests/bpf: uprobe tests should verify param/return values
https://git.kernel.org/bpf/bpf-next/c/1717e248014c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-07 18:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-06 11:43 [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 1/3] libbpf: improve library identification for uprobe binary path resolution Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 2/3] libbpf: improve string parsing for uprobe auto-attach Alan Maguire
2022-04-06 11:43 ` [PATCH v2 bpf-next 3/3] selftests/bpf: uprobe tests should verify param/return values Alan Maguire
2022-04-07 18:50 ` [PATCH v2 bpf-next 0/3] libbpf: uprobe name-based attach followups patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).