[PATCH bpf-next v2 1/2] libbpf: Add unique_match option for multi kprobe

[PATCH bpf-next v2 1/2] libbpf: Add unique_match option for multi kprobe

[Yonghong Song]

Jordan reported an issue in Meta production environment where func
try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang
compiler at lto mode. The original 'kprobe/try_to_wake_up' does not
work any more since try_to_wake_up() does not match the actual func
name in /proc/kallsyms.

There are a couple of ways to resolve this issue. For example, in
attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up()
can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users
to use bpf_program__attach_kprobe() where they need to lookup
/proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two
approaches requires extra work by either libbpf or user.

Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*')
for symbol matching. In the above example, 'try_to_wake_up*' can match
to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows
bpf prog works for different kernels as some kernels may have
try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>().

The original intention is to kprobe try_to_wake_up() only, so an optional
field unique_match is added to struct bpf_kprobe_multi_opts. If the
field is set to true, the number of matched functions must be one.
Otherwise, the attachment will fail. In the above case, multi kprobe
with 'try_to_wake_up*' and unique_match preserves user functionality.

Reported-by: Jordan Rome <linux@jordanrome.com>
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/lib/bpf/libbpf.c | 13 ++++++++++++-
 tools/lib/bpf/libbpf.h |  4 +++-
 2 files changed, 15 insertions(+), 2 deletions(-)

Changelog:
  v1 -> v2:
    - Avoid possible memory leak of res.addrs.
    - Return an error for !pattern && unique_match case.

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 46492cc0927d..a7cc6545ec63 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11534,7 +11534,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 	struct bpf_link *link = NULL;
 	const unsigned long *addrs;
 	int err, link_fd, prog_fd;
-	bool retprobe, session;
+	bool retprobe, session, unique_match;
 	const __u64 *cookies;
 	const char **syms;
 	size_t cnt;
@@ -11553,6 +11553,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 	addrs   = OPTS_GET(opts, addrs, false);
 	cnt     = OPTS_GET(opts, cnt, false);
 	cookies = OPTS_GET(opts, cookies, false);
+	unique_match = OPTS_GET(opts, unique_match, false);
 
 	if (!pattern && !addrs && !syms)
 		return libbpf_err_ptr(-EINVAL);
@@ -11560,6 +11561,8 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 		return libbpf_err_ptr(-EINVAL);
 	if (!pattern && !cnt)
 		return libbpf_err_ptr(-EINVAL);
+	if (!pattern && unique_match)
+		return libbpf_err_ptr(-EINVAL);
 	if (addrs && syms)
 		return libbpf_err_ptr(-EINVAL);
 
@@ -11570,6 +11573,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
 			err = libbpf_available_kallsyms_parse(&res);
 		if (err)
 			goto error;
+
+		if (unique_match && res.cnt != 1) {
+			pr_warn("prog '%s': failed to find a unique match, num matches: %lu\n",
+				prog->name, res.cnt);
+			err = -EINVAL;
+			goto error;
+		}
+
 		addrs = res.addrs;
 		cnt = res.cnt;
 	}
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index d45807103565..3020ee45303a 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts {
 	bool retprobe;
 	/* create session kprobes */
 	bool session;
+	/* enforce unique match */
+	bool unique_match;
 	size_t :0;
 };
 
-#define bpf_kprobe_multi_opts__last_field session
+#define bpf_kprobe_multi_opts__last_field unique_match
 
 LIBBPF_API struct bpf_link *
 bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
-- 
2.43.5

[PATCH bpf-next v2 2/2] selftests/bpf: Add a test for kprobe multi with unique_match

[Yonghong Song]

Add a kprobe multi subtest to test kprobe multi unique_match option.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/kprobe_multi_test.c        | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index 66ab1cae923e..e19ef509ebf8 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -397,6 +397,31 @@ static void test_session_cookie_skel_api(void)
 	kprobe_multi_session_cookie__destroy(skel);
 }
 
+static void test_unique_match(void)
+{
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	struct kprobe_multi *skel = NULL;
+	struct bpf_link *link = NULL;
+
+	skel = kprobe_multi__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi__open_and_load"))
+		return;
+
+	opts.unique_match = true;
+	skel->bss->pid = getpid();
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual,
+						     "bpf_fentry_test*", &opts);
+	if (!ASSERT_ERR_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
+		bpf_link__destroy(link);
+
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_manual,
+						     "bpf_fentry_test8*", &opts);
+	if (ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
+		bpf_link__destroy(link);
+
+	kprobe_multi__destroy(skel);
+}
+
 static size_t symbol_hash(long key, void *ctx __maybe_unused)
 {
 	return str_hash((const char *) key);
@@ -765,5 +790,7 @@ void test_kprobe_multi_test(void)
 		test_session_skel_api();
 	if (test__start_subtest("session_cookie"))
 		test_session_cookie_skel_api();
+	if (test__start_subtest("unique_match"))
+		test_unique_match();
 	RUN_TESTS(kprobe_multi_verifier);
 }
-- 
2.43.5

Re: [PATCH bpf-next v2 1/2] libbpf: Add unique_match option for multi kprobe

[Andrii Nakryiko]

On Thu, Jan 9, 2025 at 9:40 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Jordan reported an issue in Meta production environment where func
> try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang
> compiler at lto mode. The original 'kprobe/try_to_wake_up' does not
> work any more since try_to_wake_up() does not match the actual func
> name in /proc/kallsyms.
>
> There are a couple of ways to resolve this issue. For example, in
> attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up()
> can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users
> to use bpf_program__attach_kprobe() where they need to lookup
> /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two
> approaches requires extra work by either libbpf or user.
>
> Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*')
> for symbol matching. In the above example, 'try_to_wake_up*' can match
> to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows
> bpf prog works for different kernels as some kernels may have
> try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>().
>
> The original intention is to kprobe try_to_wake_up() only, so an optional
> field unique_match is added to struct bpf_kprobe_multi_opts. If the
> field is set to true, the number of matched functions must be one.
> Otherwise, the attachment will fail. In the above case, multi kprobe
> with 'try_to_wake_up*' and unique_match preserves user functionality.
>
> Reported-by: Jordan Rome <linux@jordanrome.com>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c | 13 ++++++++++++-
>  tools/lib/bpf/libbpf.h |  4 +++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> Changelog:
>   v1 -> v2:
>     - Avoid possible memory leak of res.addrs.
>     - Return an error for !pattern && unique_match case.
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 46492cc0927d..a7cc6545ec63 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11534,7 +11534,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>         struct bpf_link *link = NULL;
>         const unsigned long *addrs;
>         int err, link_fd, prog_fd;
> -       bool retprobe, session;
> +       bool retprobe, session, unique_match;
>         const __u64 *cookies;
>         const char **syms;
>         size_t cnt;
> @@ -11553,6 +11553,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>         addrs   = OPTS_GET(opts, addrs, false);
>         cnt     = OPTS_GET(opts, cnt, false);
>         cookies = OPTS_GET(opts, cookies, false);
> +       unique_match = OPTS_GET(opts, unique_match, false);
>
>         if (!pattern && !addrs && !syms)
>                 return libbpf_err_ptr(-EINVAL);
> @@ -11560,6 +11561,8 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>                 return libbpf_err_ptr(-EINVAL);
>         if (!pattern && !cnt)
>                 return libbpf_err_ptr(-EINVAL);
> +       if (!pattern && unique_match)
> +               return libbpf_err_ptr(-EINVAL);
>         if (addrs && syms)
>                 return libbpf_err_ptr(-EINVAL);
>
> @@ -11570,6 +11573,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>                         err = libbpf_available_kallsyms_parse(&res);
>                 if (err)
>                         goto error;
> +
> +               if (unique_match && res.cnt != 1) {
> +                       pr_warn("prog '%s': failed to find a unique match, num matches: %lu\n",
> +                               prog->name, res.cnt);

I've added pattern itself into the error message, and also %lu -> %zu
(it's size_t, not long unsigned)


> +                       err = -EINVAL;
> +                       goto error;
> +               }
> +
>                 addrs = res.addrs;
>                 cnt = res.cnt;
>         }
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index d45807103565..3020ee45303a 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts {
>         bool retprobe;
>         /* create session kprobes */
>         bool session;
> +       /* enforce unique match */
> +       bool unique_match;
>         size_t :0;
>  };
>
> -#define bpf_kprobe_multi_opts__last_field session
> +#define bpf_kprobe_multi_opts__last_field unique_match
>
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> --
> 2.43.5
>

Re: [PATCH bpf-next v2 1/2] libbpf: Add unique_match option for multi kprobe

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu,  9 Jan 2025 09:40:23 -0800 you wrote:
> Jordan reported an issue in Meta production environment where func
> try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang
> compiler at lto mode. The original 'kprobe/try_to_wake_up' does not
> work any more since try_to_wake_up() does not match the actual func
> name in /proc/kallsyms.
> 
> There are a couple of ways to resolve this issue. For example, in
> attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up()
> can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users
> to use bpf_program__attach_kprobe() where they need to lookup
> /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two
> approaches requires extra work by either libbpf or user.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/2] libbpf: Add unique_match option for multi kprobe
    https://git.kernel.org/bpf/bpf-next/c/e2b0bda62d54
  - [bpf-next,v2,2/2] selftests/bpf: Add a test for kprobe multi with unique_match
    https://git.kernel.org/bpf/bpf-next/c/a43796b52012

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html