[PATCH bpf-next 0/2] libbpf: uprobe name-based attach followups

[PATCH bpf-next 0/2] libbpf: uprobe name-based attach followups

[Alan Maguire]

Follow-up series to [1] to address some suggestions from Andrii to
improve parsing and make it more robust (patch 1) and to improve
validation of u[ret]probe firing by validating expected argument
and return values.

[1] https://lore.kernel.org/bpf/164903521182.13106.12656654142629368774.git-patchwork-notify@kernel.org/

Alan Maguire (2):
  libbpf: improve string handling for uprobe name-based attach
  selftests/bpf: uprobe tests should verify param/return values

 tools/lib/bpf/libbpf.c                             | 77 +++++++++-------------
 tools/lib/bpf/libbpf_internal.h                    |  5 ++
 .../selftests/bpf/prog_tests/uprobe_autoattach.c   | 16 +++--
 .../selftests/bpf/progs/test_uprobe_autoattach.c   | 25 ++++---
 4 files changed, 64 insertions(+), 59 deletions(-)

-- 
1.8.3.1

[PATCH bpf-next 1/2] libbpf: improve string handling for uprobe name-based attach

[Alan Maguire]

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] suffixes, so instead match a ".so"
_suffix_, and if that fails match a ".so." substring for the versioned
library case.

For uprobe auto-attach, the parsing can be simplified for the SEC()
name to a single ssscanf(); 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          | 77 ++++++++++++++++-------------------------
 tools/lib/bpf/libbpf_internal.h |  5 +++
 2 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 91ce94b..3f23e88 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10750,7 +10750,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();
@@ -10897,60 +10897,43 @@ 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;
+	char *probe_type = NULL, *binary_path = NULL, *func_name = NULL;
+	int n, ret = -EINVAL;
 	size_t offset = 0;
-	int n;
 
 	*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) {
+	n = sscanf(prog->sec_name, "%m[^/]/%m[^:]:%m[a-zA-Z0-9_.]+%zu",
+		   &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("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;
+		break;
+	case 3:
+	case 4:
+		opts.retprobe = str_has_pfx(prog->sec_name, "uretprobe");
+		if (opts.retprobe && offset != 0) {
+			pr_warn("uretprobes do not support offset specification\n");
+			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("uprobe name '%s' is invalid\n", 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,
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index b6247dc..155702a 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -103,6 +103,11 @@
 #define str_has_pfx(str, pfx) \
 	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
 
+/* similar for suffix */
+#define str_has_sfx(str, sfx) \
+	(strlen(sfx) <= strlen(str) ? \
+	 strncmp(str + strlen(str) - strlen(sfx), sfx, strlen(sfx)) == 0 : 0)
+
 /* 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

[PATCH bpf-next 2/2] selftests/bpf: uprobe tests should verify param/return values

[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
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 ("");
+	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;
 }
 
-- 
1.8.3.1

Re: [PATCH bpf-next 1/2] libbpf: improve string handling for uprobe name-based attach

[Andrii Nakryiko]

On Tue, Apr 5, 2022 at 2:46 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> 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] suffixes, so instead match a ".so"
> _suffix_, and if that fails match a ".so." substring for the versioned
> library case.
>

You are making two separate changes in one patch, let's split them.

> For uprobe auto-attach, the parsing can be simplified for the SEC()
> name to a single ssscanf(); the return value of the sscanf can then

too many sss :)

> 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          | 77 ++++++++++++++++-------------------------
>  tools/lib/bpf/libbpf_internal.h |  5 +++
>  2 files changed, 35 insertions(+), 47 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 91ce94b..3f23e88 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10750,7 +10750,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();
> @@ -10897,60 +10897,43 @@ 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;
> +       char *probe_type = NULL, *binary_path = NULL, *func_name = NULL;
> +       int n, ret = -EINVAL;
>         size_t offset = 0;
> -       int n;
>
>         *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) {
> +       n = sscanf(prog->sec_name, "%m[^/]/%m[^:]:%m[a-zA-Z0-9_.]+%zu",

note that previously you were using %li for offset which allows
decimal and hexadecimal formats, I think that's convenient, let's
allow that still

> +                  &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("section '%s' missing ':function[+offset]' specification\n",
>                         prog->sec_name);

please use 'prog '%s': ' prefix in these attach_xxx() functions for consistency

> -               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;
> +               break;
> +       case 3:
> +       case 4:
> +               opts.retprobe = str_has_pfx(prog->sec_name, "uretprobe");

you just parsed probe_type, strcmp() against that instead, no need for
prefix check

> +               if (opts.retprobe && offset != 0) {
> +                       pr_warn("uretprobes do not support offset specification\n");
> +                       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("uprobe name '%s' is invalid\n", prog->sec_name);

Add "prog '%s': " prefix. Also, the section name is not an uprobe
name. Maybe "prog '%s': invalid format of section definition '%s'\n"?

> +               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,
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index b6247dc..155702a 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -103,6 +103,11 @@
>  #define str_has_pfx(str, pfx) \
>         (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
>
> +/* similar for suffix */
> +#define str_has_sfx(str, sfx) \
> +       (strlen(sfx) <= strlen(str) ? \
> +        strncmp(str + strlen(str) - strlen(sfx), sfx, strlen(sfx)) == 0 : 0)
> +

so str_has_pfx() is a macro to avoid strlen() for string literals.
Here you don't do any optimization like that and instead calculating
and recalculating strlen() multiple times. Just make this a static
inline helper function?

and you don't need strncmp() anymore, strcmp() is as safe after all
the strlen() checks and calculations



>  /* 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
>

Re: [PATCH bpf-next 2/2] selftests/bpf: uprobe tests should verify param/return values

[Andrii Nakryiko]

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
>