* [PATCH v3 bpf-next 0/3] libbpf: Fix event name too long error and add tests
@ 2025-04-14 9:33 Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error Feng Yang
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Feng Yang @ 2025-04-14 9:33 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, hengqi.chen
Cc: bpf, linux-kernel
From: Feng Yang <yangfeng@kylinos.cn>
Hi everyone,
This series tries to fix event name too long error and add tests.
When the binary path is excessively long, the generated probe_name in libbpf
exceeds the kernel's MAX_EVENT_NAME_LEN limit (64 bytes).
This causes legacy uprobe event attachment to fail with error code -22.
---
Changes in v3:
- add __sync_fetch_and_add(&index) and let snprintf() do the trimming. Thanks, Andrii Nakryiko!
- add selftests.
- Link to v2: https://lore.kernel.org/all/20250411080545.319865-1-yangfeng59949@163.com/
Changes in v2:
- Use basename() and %.32s to fix. Thanks, Hengqi Chen!
- Link to v1: https://lore.kernel.org/all/20250410052712.206785-1-yangfeng59949@163.com/
Feng Yang (3):
libbpf: Fix event name too long error
selftests/bpf: Add test for attaching uprobe with long event names
selftests/bpf: Add test for attaching kprobe with long event names
tools/lib/bpf/libbpf.c | 19 +++--
.../selftests/bpf/prog_tests/attach_probe.c | 84 +++++++++++++++++++
.../selftests/bpf/test_kmods/bpf_testmod.c | 5 ++
.../bpf/test_kmods/bpf_testmod_kfunc.h | 2 +
4 files changed, 103 insertions(+), 7 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error
2025-04-14 9:33 [PATCH v3 bpf-next 0/3] libbpf: Fix event name too long error and add tests Feng Yang
@ 2025-04-14 9:34 ` Feng Yang
2025-04-14 11:43 ` Jiri Olsa
2025-04-14 9:34 ` [PATCH v3 bpf-next 2/3] selftests/bpf: Add test for attaching uprobe with long event names Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe " Feng Yang
2 siblings, 1 reply; 9+ messages in thread
From: Feng Yang @ 2025-04-14 9:34 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, hengqi.chen
Cc: bpf, linux-kernel
From: Feng Yang <yangfeng@kylinos.cn>
When the binary path is excessively long, the generated probe_name in libbpf
exceeds the kernel's MAX_EVENT_NAME_LEN limit (64 bytes).
This causes legacy uprobe event attachment to fail with error code -22.
Before Fix:
./test_progs -t attach_probe/kprobe-long_name
......
libbpf: failed to add legacy kprobe event for 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0': -EINVAL
libbpf: prog 'handle_kprobe': failed to create kprobe 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0' perf event: -EINVAL
test_attach_kprobe_long_event_name:FAIL:attach_kprobe_long_event_name unexpected error: -22
test_attach_probe:PASS:uprobe_ref_ctr_cleanup 0 nsec
#13/11 attach_probe/kprobe-long_name:FAIL
#13 attach_probe:FAIL
./test_progs -t attach_probe/uprobe-long_name
......
libbpf: failed to add legacy uprobe event for /root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9: -EINVAL
libbpf: prog 'handle_uprobe': failed to create uprobe '/root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9' perf event: -EINVAL
test_attach_uprobe_long_event_name:FAIL:attach_uprobe_long_event_name unexpected error: -22
#13/10 attach_probe/uprobe-long_name:FAIL
#13 attach_probe:FAIL
After Fix:
./test_progs -t attach_probe/uprobe-long_name
#13/10 attach_probe/uprobe-long_name:OK
#13 attach_probe:OK
Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
./test_progs -t attach_probe/kprobe-long_name
#13/11 attach_probe/kprobe-long_name:OK
#13 attach_probe:OK
Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
Fixes: 46ed5fc33db9 ("libbpf: Refactor and simplify legacy kprobe code")
Fixes: cc10623c6810 ("libbpf: Add legacy uprobe attaching support")
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
tools/lib/bpf/libbpf.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b2591f5cab65..9e047641e001 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -60,6 +60,8 @@
#define BPF_FS_MAGIC 0xcafe4a11
#endif
+#define MAX_EVENT_NAME_LEN 64
+
#define BPF_FS_DEFAULT_PATH "/sys/fs/bpf"
#define BPF_INSN_SZ (sizeof(struct bpf_insn))
@@ -11142,10 +11144,10 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
static int index = 0;
int i;
- snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset,
- __sync_fetch_and_add(&index, 1));
+ snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
+ __sync_fetch_and_add(&index, 1), kfunc_name, offset);
- /* sanitize binary_path in the probe name */
+ /* sanitize kfunc_name in the probe name */
for (i = 0; buf[i]; i++) {
if (!isalnum(buf[i]))
buf[i] = '_';
@@ -11270,7 +11272,7 @@ int probe_kern_syscall_wrapper(int token_fd)
return pfd >= 0 ? 1 : 0;
} else { /* legacy mode */
- char probe_name[128];
+ char probe_name[MAX_EVENT_NAME_LEN];
gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
if (add_kprobe_event_legacy(probe_name, false, syscall_name, 0) < 0)
@@ -11328,7 +11330,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
func_name, offset,
-1 /* pid */, 0 /* ref_ctr_off */);
} else {
- char probe_name[256];
+ char probe_name[MAX_EVENT_NAME_LEN];
gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name),
func_name, offset);
@@ -11878,9 +11880,12 @@ static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, stru
static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
const char *binary_path, uint64_t offset)
{
+ static int index = 0;
int i;
- snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(), binary_path, (size_t)offset);
+ snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
+ __sync_fetch_and_add(&index, 1),
+ basename((void *)binary_path), (size_t)offset);
/* sanitize binary_path in the probe name */
for (i = 0; buf[i]; i++) {
@@ -12312,7 +12317,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
func_offset, pid, ref_ctr_off);
} else {
- char probe_name[PATH_MAX + 64];
+ char probe_name[MAX_EVENT_NAME_LEN];
if (ref_ctr_off)
return libbpf_err_ptr(-EINVAL);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 bpf-next 2/3] selftests/bpf: Add test for attaching uprobe with long event names
2025-04-14 9:33 [PATCH v3 bpf-next 0/3] libbpf: Fix event name too long error and add tests Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error Feng Yang
@ 2025-04-14 9:34 ` Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe " Feng Yang
2 siblings, 0 replies; 9+ messages in thread
From: Feng Yang @ 2025-04-14 9:34 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, hengqi.chen
Cc: bpf, linux-kernel
From: Feng Yang <yangfeng@kylinos.cn>
This test verifies that attaching uprobe/uretprobe with long event names
does not trigger EINVAL errors.
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
.../selftests/bpf/prog_tests/attach_probe.c | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index 329c7862b52d..9b7f36f39c32 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -122,6 +122,52 @@ static void test_attach_probe_manual(enum probe_attach_mode attach_mode)
test_attach_probe_manual__destroy(skel);
}
+/* attach uprobe/uretprobe long event name testings */
+static void test_attach_uprobe_long_event_name(void)
+{
+ DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
+ struct bpf_link *uprobe_link, *uretprobe_link;
+ struct test_attach_probe_manual *skel;
+ ssize_t uprobe_offset;
+ char path[PATH_MAX] = {0};
+
+ skel = test_attach_probe_manual__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel_kprobe_manual_open_and_load"))
+ return;
+
+ uprobe_offset = get_uprobe_offset(&trigger_func);
+ if (!ASSERT_GE(uprobe_offset, 0, "uprobe_offset"))
+ goto cleanup;
+
+ if (!ASSERT_GT(readlink("/proc/self/exe", path, PATH_MAX - 1), 0, "readlink"))
+ goto cleanup;
+
+ /* manual-attach uprobe/uretprobe */
+ uprobe_opts.attach_mode = PROBE_ATTACH_MODE_LEGACY;
+ uprobe_opts.ref_ctr_offset = 0;
+ uprobe_opts.retprobe = false;
+ uprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe,
+ 0 /* self pid */,
+ path,
+ uprobe_offset,
+ &uprobe_opts);
+ if (!ASSERT_OK_PTR(uprobe_link, "attach_uprobe_long_event_name"))
+ goto cleanup;
+ skel->links.handle_uprobe = uprobe_link;
+
+ uprobe_opts.retprobe = true;
+ uretprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe,
+ -1 /* any pid */,
+ path,
+ uprobe_offset, &uprobe_opts);
+ if (!ASSERT_OK_PTR(uretprobe_link, "attach_uretprobe_long_event_name"))
+ goto cleanup;
+ skel->links.handle_uretprobe = uretprobe_link;
+
+cleanup:
+ test_attach_probe_manual__destroy(skel);
+}
+
static void test_attach_probe_auto(struct test_attach_probe *skel)
{
struct bpf_link *uprobe_err_link;
@@ -323,6 +369,9 @@ void test_attach_probe(void)
if (test__start_subtest("uprobe-ref_ctr"))
test_uprobe_ref_ctr(skel);
+ if (test__start_subtest("uprobe-long_name"))
+ test_attach_uprobe_long_event_name();
+
cleanup:
test_attach_probe__destroy(skel);
ASSERT_EQ(uprobe_ref_ctr, 0, "uprobe_ref_ctr_cleanup");
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe with long event names
2025-04-14 9:33 [PATCH v3 bpf-next 0/3] libbpf: Fix event name too long error and add tests Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 2/3] selftests/bpf: Add test for attaching uprobe with long event names Feng Yang
@ 2025-04-14 9:34 ` Feng Yang
2025-04-14 11:47 ` Jiri Olsa
2 siblings, 1 reply; 9+ messages in thread
From: Feng Yang @ 2025-04-14 9:34 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, hengqi.chen
Cc: bpf, linux-kernel
From: Feng Yang <yangfeng@kylinos.cn>
This test verifies that attaching kprobe/kretprobe with long event names
does not trigger EINVAL errors.
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
.../selftests/bpf/prog_tests/attach_probe.c | 35 +++++++++++++++++++
.../selftests/bpf/test_kmods/bpf_testmod.c | 5 +++
.../bpf/test_kmods/bpf_testmod_kfunc.h | 2 ++
3 files changed, 42 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index 9b7f36f39c32..633b5eb4379b 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -168,6 +168,39 @@ static void test_attach_uprobe_long_event_name(void)
test_attach_probe_manual__destroy(skel);
}
+/* attach kprobe/kretprobe long event name testings */
+static void test_attach_kprobe_long_event_name(void)
+{
+ DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, kprobe_opts);
+ struct bpf_link *kprobe_link, *kretprobe_link;
+ struct test_attach_probe_manual *skel;
+
+ skel = test_attach_probe_manual__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel_kprobe_manual_open_and_load"))
+ return;
+
+ /* manual-attach kprobe/kretprobe */
+ kprobe_opts.attach_mode = PROBE_ATTACH_MODE_LEGACY;
+ kprobe_opts.retprobe = false;
+ kprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kprobe,
+ "bpf_kfunc_looooooooooooooooooooooooooooooong_name",
+ &kprobe_opts);
+ if (!ASSERT_OK_PTR(kprobe_link, "attach_kprobe_long_event_name"))
+ goto cleanup;
+ skel->links.handle_kprobe = kprobe_link;
+
+ kprobe_opts.retprobe = true;
+ kretprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kretprobe,
+ "bpf_kfunc_looooooooooooooooooooooooooooooong_name",
+ &kprobe_opts);
+ if (!ASSERT_OK_PTR(kretprobe_link, "attach_kretprobe_long_event_name"))
+ goto cleanup;
+ skel->links.handle_kretprobe = kretprobe_link;
+
+cleanup:
+ test_attach_probe_manual__destroy(skel);
+}
+
static void test_attach_probe_auto(struct test_attach_probe *skel)
{
struct bpf_link *uprobe_err_link;
@@ -371,6 +404,8 @@ void test_attach_probe(void)
if (test__start_subtest("uprobe-long_name"))
test_attach_uprobe_long_event_name();
+ if (test__start_subtest("kprobe-long_name"))
+ test_attach_kprobe_long_event_name();
cleanup:
test_attach_probe__destroy(skel);
diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
index f38eaf0d35ef..439f6c2b2456 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -1053,6 +1053,10 @@ __bpf_kfunc int bpf_kfunc_st_ops_inc10(struct st_ops_args *args)
return args->a;
}
+__bpf_kfunc void bpf_kfunc_looooooooooooooooooooooooooooooong_name(void)
+{
+}
+
BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
@@ -1093,6 +1097,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_prologue, KF_TRUSTED_ARGS | KF_SLEEPABL
BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_pro_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
BTF_ID_FLAGS(func, bpf_kfunc_st_ops_inc10, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_looooooooooooooooooooooooooooooong_name)
BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
static int bpf_testmod_ops_init(struct btf *btf)
diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
index b58817938deb..e5b833140418 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
@@ -159,4 +159,6 @@ void bpf_kfunc_trusted_task_test(struct task_struct *ptr) __ksym;
void bpf_kfunc_trusted_num_test(int *ptr) __ksym;
void bpf_kfunc_rcu_task_test(struct task_struct *ptr) __ksym;
+void bpf_kfunc_looooooooooooooooooooooooooooooong_name(void) __ksym;
+
#endif /* _BPF_TESTMOD_KFUNC_H */
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error
2025-04-14 9:34 ` [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error Feng Yang
@ 2025-04-14 11:43 ` Jiri Olsa
2025-04-15 2:01 ` Feng Yang
0 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2025-04-14 11:43 UTC (permalink / raw)
To: Feng Yang
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, hengqi.chen, bpf,
linux-kernel
On Mon, Apr 14, 2025 at 05:34:00PM +0800, Feng Yang wrote:
> From: Feng Yang <yangfeng@kylinos.cn>
>
> When the binary path is excessively long, the generated probe_name in libbpf
> exceeds the kernel's MAX_EVENT_NAME_LEN limit (64 bytes).
> This causes legacy uprobe event attachment to fail with error code -22.
>
> Before Fix:
> ./test_progs -t attach_probe/kprobe-long_name
> ......
> libbpf: failed to add legacy kprobe event for 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0': -EINVAL
> libbpf: prog 'handle_kprobe': failed to create kprobe 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0' perf event: -EINVAL
> test_attach_kprobe_long_event_name:FAIL:attach_kprobe_long_event_name unexpected error: -22
> test_attach_probe:PASS:uprobe_ref_ctr_cleanup 0 nsec
> #13/11 attach_probe/kprobe-long_name:FAIL
> #13 attach_probe:FAIL
>
> ./test_progs -t attach_probe/uprobe-long_name
> ......
> libbpf: failed to add legacy uprobe event for /root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9: -EINVAL
> libbpf: prog 'handle_uprobe': failed to create uprobe '/root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9' perf event: -EINVAL
> test_attach_uprobe_long_event_name:FAIL:attach_uprobe_long_event_name unexpected error: -22
> #13/10 attach_probe/uprobe-long_name:FAIL
> #13 attach_probe:FAIL
> After Fix:
> ./test_progs -t attach_probe/uprobe-long_name
> #13/10 attach_probe/uprobe-long_name:OK
> #13 attach_probe:OK
> Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
>
> ./test_progs -t attach_probe/kprobe-long_name
> #13/11 attach_probe/kprobe-long_name:OK
> #13 attach_probe:OK
> Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
>
> Fixes: 46ed5fc33db9 ("libbpf: Refactor and simplify legacy kprobe code")
> Fixes: cc10623c6810 ("libbpf: Add legacy uprobe attaching support")
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
> ---
> tools/lib/bpf/libbpf.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b2591f5cab65..9e047641e001 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -60,6 +60,8 @@
> #define BPF_FS_MAGIC 0xcafe4a11
> #endif
>
> +#define MAX_EVENT_NAME_LEN 64
> +
> #define BPF_FS_DEFAULT_PATH "/sys/fs/bpf"
>
> #define BPF_INSN_SZ (sizeof(struct bpf_insn))
> @@ -11142,10 +11144,10 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
> static int index = 0;
> int i;
>
> - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset,
> - __sync_fetch_and_add(&index, 1));
> + snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
> + __sync_fetch_and_add(&index, 1), kfunc_name, offset);
so the fix is to move unique id before kfunc_name to make sure it gets
to the event name right? would be great to have it in changelog
>
> - /* sanitize binary_path in the probe name */
> + /* sanitize kfunc_name in the probe name */
> for (i = 0; buf[i]; i++) {
> if (!isalnum(buf[i]))
> buf[i] = '_';
> @@ -11270,7 +11272,7 @@ int probe_kern_syscall_wrapper(int token_fd)
>
> return pfd >= 0 ? 1 : 0;
> } else { /* legacy mode */
> - char probe_name[128];
> + char probe_name[MAX_EVENT_NAME_LEN];
>
> gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
> if (add_kprobe_event_legacy(probe_name, false, syscall_name, 0) < 0)
> @@ -11328,7 +11330,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> func_name, offset,
> -1 /* pid */, 0 /* ref_ctr_off */);
> } else {
> - char probe_name[256];
> + char probe_name[MAX_EVENT_NAME_LEN];
>
> gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name),
> func_name, offset);
> @@ -11878,9 +11880,12 @@ static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, stru
> static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
> const char *binary_path, uint64_t offset)
> {
> + static int index = 0;
> int i;
>
> - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(), binary_path, (size_t)offset);
> + snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
> + __sync_fetch_and_add(&index, 1),
> + basename((void *)binary_path), (size_t)offset);
gen_kprobe_legacy_event_name and gen_uprobe_legacy_event_name seem to
be identical now, maybe we can have just one ?
thanks,
jirka
>
> /* sanitize binary_path in the probe name */
> for (i = 0; buf[i]; i++) {
> @@ -12312,7 +12317,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
> func_offset, pid, ref_ctr_off);
> } else {
> - char probe_name[PATH_MAX + 64];
> + char probe_name[MAX_EVENT_NAME_LEN];
>
> if (ref_ctr_off)
> return libbpf_err_ptr(-EINVAL);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe with long event names
2025-04-14 9:34 ` [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe " Feng Yang
@ 2025-04-14 11:47 ` Jiri Olsa
2025-04-15 2:52 ` Feng Yang
0 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2025-04-14 11:47 UTC (permalink / raw)
To: Feng Yang
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, hengqi.chen, bpf,
linux-kernel
On Mon, Apr 14, 2025 at 05:34:02PM +0800, Feng Yang wrote:
> From: Feng Yang <yangfeng@kylinos.cn>
>
> This test verifies that attaching kprobe/kretprobe with long event names
> does not trigger EINVAL errors.
>
> Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
> ---
> .../selftests/bpf/prog_tests/attach_probe.c | 35 +++++++++++++++++++
> .../selftests/bpf/test_kmods/bpf_testmod.c | 5 +++
> .../bpf/test_kmods/bpf_testmod_kfunc.h | 2 ++
> 3 files changed, 42 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index 9b7f36f39c32..633b5eb4379b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -168,6 +168,39 @@ static void test_attach_uprobe_long_event_name(void)
> test_attach_probe_manual__destroy(skel);
> }
>
> +/* attach kprobe/kretprobe long event name testings */
> +static void test_attach_kprobe_long_event_name(void)
> +{
> + DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, kprobe_opts);
> + struct bpf_link *kprobe_link, *kretprobe_link;
> + struct test_attach_probe_manual *skel;
> +
> + skel = test_attach_probe_manual__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel_kprobe_manual_open_and_load"))
> + return;
> +
> + /* manual-attach kprobe/kretprobe */
> + kprobe_opts.attach_mode = PROBE_ATTACH_MODE_LEGACY;
> + kprobe_opts.retprobe = false;
> + kprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kprobe,
> + "bpf_kfunc_looooooooooooooooooooooooooooooong_name",
> + &kprobe_opts);
> + if (!ASSERT_OK_PTR(kprobe_link, "attach_kprobe_long_event_name"))
> + goto cleanup;
> + skel->links.handle_kprobe = kprobe_link;
> +
> + kprobe_opts.retprobe = true;
> + kretprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kretprobe,
> + "bpf_kfunc_looooooooooooooooooooooooooooooong_name",
> + &kprobe_opts);
> + if (!ASSERT_OK_PTR(kretprobe_link, "attach_kretprobe_long_event_name"))
> + goto cleanup;
> + skel->links.handle_kretprobe = kretprobe_link;
> +
> +cleanup:
> + test_attach_probe_manual__destroy(skel);
> +}
> +
> static void test_attach_probe_auto(struct test_attach_probe *skel)
> {
> struct bpf_link *uprobe_err_link;
> @@ -371,6 +404,8 @@ void test_attach_probe(void)
>
> if (test__start_subtest("uprobe-long_name"))
> test_attach_uprobe_long_event_name();
> + if (test__start_subtest("kprobe-long_name"))
> + test_attach_kprobe_long_event_name();
>
> cleanup:
> test_attach_probe__destroy(skel);
> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> index f38eaf0d35ef..439f6c2b2456 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> @@ -1053,6 +1053,10 @@ __bpf_kfunc int bpf_kfunc_st_ops_inc10(struct st_ops_args *args)
> return args->a;
> }
>
> +__bpf_kfunc void bpf_kfunc_looooooooooooooooooooooooooooooong_name(void)
> +{
> +}
does it need to be a kfunc? IIUC it just needs to be a normal kernel/module function
jirka
> +
> BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
> BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
> BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
> @@ -1093,6 +1097,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_prologue, KF_TRUSTED_ARGS | KF_SLEEPABL
> BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
> BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_pro_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
> BTF_ID_FLAGS(func, bpf_kfunc_st_ops_inc10, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_looooooooooooooooooooooooooooooong_name)
> BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
>
> static int bpf_testmod_ops_init(struct btf *btf)
> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> index b58817938deb..e5b833140418 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> @@ -159,4 +159,6 @@ void bpf_kfunc_trusted_task_test(struct task_struct *ptr) __ksym;
> void bpf_kfunc_trusted_num_test(int *ptr) __ksym;
> void bpf_kfunc_rcu_task_test(struct task_struct *ptr) __ksym;
>
> +void bpf_kfunc_looooooooooooooooooooooooooooooong_name(void) __ksym;
> +
> #endif /* _BPF_TESTMOD_KFUNC_H */
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error
2025-04-14 11:43 ` Jiri Olsa
@ 2025-04-15 2:01 ` Feng Yang
2025-04-15 7:20 ` Jiri Olsa
0 siblings, 1 reply; 9+ messages in thread
From: Feng Yang @ 2025-04-15 2:01 UTC (permalink / raw)
To: olsajiri
Cc: andrii, ast, bpf, daniel, eddyz87, haoluo, hengqi.chen,
john.fastabend, kpsingh, linux-kernel, martin.lau, sdf, song,
yangfeng59949, yonghong.song
On Mon, 14 Apr 2025 13:43:38 +0200 Jiri Olsa <olsajiri@gmail.com> wrote:
> On Mon, Apr 14, 2025 at 05:34:00PM +0800, Feng Yang wrote:
> > From: Feng Yang <yangfeng@kylinos.cn>
> >
> > When the binary path is excessively long, the generated probe_name in libbpf
> > exceeds the kernel's MAX_EVENT_NAME_LEN limit (64 bytes).
> > This causes legacy uprobe event attachment to fail with error code -22.
> >
> > Before Fix:
> > ./test_progs -t attach_probe/kprobe-long_name
> > ......
> > libbpf: failed to add legacy kprobe event for 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0': -EINVAL
> > libbpf: prog 'handle_kprobe': failed to create kprobe 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0' perf event: -EINVAL
> > test_attach_kprobe_long_event_name:FAIL:attach_kprobe_long_event_name unexpected error: -22
> > test_attach_probe:PASS:uprobe_ref_ctr_cleanup 0 nsec
> > #13/11 attach_probe/kprobe-long_name:FAIL
> > #13 attach_probe:FAIL
> >
> > ./test_progs -t attach_probe/uprobe-long_name
> > ......
> > libbpf: failed to add legacy uprobe event for /root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9: -EINVAL
> > libbpf: prog 'handle_uprobe': failed to create uprobe '/root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9' perf event: -EINVAL
> > test_attach_uprobe_long_event_name:FAIL:attach_uprobe_long_event_name unexpected error: -22
> > #13/10 attach_probe/uprobe-long_name:FAIL
> > #13 attach_probe:FAIL
> > After Fix:
> > ./test_progs -t attach_probe/uprobe-long_name
> > #13/10 attach_probe/uprobe-long_name:OK
> > #13 attach_probe:OK
> > Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
> >
> > ./test_progs -t attach_probe/kprobe-long_name
> > #13/11 attach_probe/kprobe-long_name:OK
> > #13 attach_probe:OK
> > Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Fixes: 46ed5fc33db9 ("libbpf: Refactor and simplify legacy kprobe code")
> > Fixes: cc10623c6810 ("libbpf: Add legacy uprobe attaching support")
> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> > Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
> > ---
> > tools/lib/bpf/libbpf.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index b2591f5cab65..9e047641e001 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -60,6 +60,8 @@
> > #define BPF_FS_MAGIC 0xcafe4a11
> > #endif
> >
> > +#define MAX_EVENT_NAME_LEN 64
> > +
> > #define BPF_FS_DEFAULT_PATH "/sys/fs/bpf"
> >
> > #define BPF_INSN_SZ (sizeof(struct bpf_insn))
> > @@ -11142,10 +11144,10 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
> > static int index = 0;
> > int i;
> >
> > - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset,
> > - __sync_fetch_and_add(&index, 1));
> > + snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
> > + __sync_fetch_and_add(&index, 1), kfunc_name, offset);
>
> so the fix is to move unique id before kfunc_name to make sure it gets
> to the event name right? would be great to have it in changelog
>
Yes, defining MAX_EVENT_NAME_LEN ensures event names are truncated via snprintf
to prevent exceeding the maximum length limit.
Moving the unique id before kfunc_name avoids truncating the id.
Regarding the changelog: Should this information go into the commit message of the patch, or somewhere else?
>
> >
> > - /* sanitize binary_path in the probe name */
> > + /* sanitize kfunc_name in the probe name */
> > for (i = 0; buf[i]; i++) {
> > if (!isalnum(buf[i]))
> > buf[i] = '_';
> > @@ -11270,7 +11272,7 @@ int probe_kern_syscall_wrapper(int token_fd)
> >
> > return pfd >= 0 ? 1 : 0;
> > } else { /* legacy mode */
> > - char probe_name[128];
> > + char probe_name[MAX_EVENT_NAME_LEN];
> >
> > gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
> > if (add_kprobe_event_legacy(probe_name, false, syscall_name, 0) < 0)
> > @@ -11328,7 +11330,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> > func_name, offset,
> > -1 /* pid */, 0 /* ref_ctr_off */);
> > } else {
> > - char probe_name[256];
> > + char probe_name[MAX_EVENT_NAME_LEN];
> >
> > gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name),
> > func_name, offset);
> > @@ -11878,9 +11880,12 @@ static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, stru
> > static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
> > const char *binary_path, uint64_t offset)
> > {
> > + static int index = 0;
> > int i;
> >
> > - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(), binary_path, (size_t)offset);
> > + snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
> > + __sync_fetch_and_add(&index, 1),
> > + basename((void *)binary_path), (size_t)offset);
>
> gen_kprobe_legacy_event_name and gen_uprobe_legacy_event_name seem to
> be identical now, maybe we can have just one ?
>
> thanks,
> jirka
>
The gen_uprobe_legacy_event_name function includes an extra basename compared to gen_kprobe_legacy_event_name,
as the prefixes of binary_path are often too similar to distinguish easily.
When merging these two into a single function, is it acceptable to pass basename((void *)binary_path)
directly during the uprobe invocation, or should we remove the addition of basename? Thank you!
> >
> > /* sanitize binary_path in the probe name */
> > for (i = 0; buf[i]; i++) {
> > @@ -12312,7 +12317,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> > pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
> > func_offset, pid, ref_ctr_off);
> > } else {
> > - char probe_name[PATH_MAX + 64];
> > + char probe_name[MAX_EVENT_NAME_LEN];
> >
> > if (ref_ctr_off)
> > return libbpf_err_ptr(-EINVAL);
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe with long event names
2025-04-14 11:47 ` Jiri Olsa
@ 2025-04-15 2:52 ` Feng Yang
0 siblings, 0 replies; 9+ messages in thread
From: Feng Yang @ 2025-04-15 2:52 UTC (permalink / raw)
To: olsajiri
Cc: andrii, ast, bpf, daniel, eddyz87, haoluo, hengqi.chen,
john.fastabend, kpsingh, linux-kernel, martin.lau, sdf, song,
yangfeng59949, yonghong.song
On Mon, 14 Apr 2025 13:47:55 +0200, Jiri Olsa <olsajiri@gmail.com> wrote:
> On Mon, Apr 14, 2025 at 05:34:02PM +0800, Feng Yang wrote:
> > From: Feng Yang <yangfeng@kylinos.cn>
> >
> > This test verifies that attaching kprobe/kretprobe with long event names
> > does not trigger EINVAL errors.
> >
> > Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
> > ---
> > .../selftests/bpf/prog_tests/attach_probe.c | 35 +++++++++++++++++++
> > .../selftests/bpf/test_kmods/bpf_testmod.c | 5 +++
> > .../bpf/test_kmods/bpf_testmod_kfunc.h | 2 ++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > index 9b7f36f39c32..633b5eb4379b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > @@ -168,6 +168,39 @@ static void test_attach_uprobe_long_event_name(void)
> > test_attach_probe_manual__destroy(skel);
> > }
> >
> > +/* attach kprobe/kretprobe long event name testings */
> > +static void test_attach_kprobe_long_event_name(void)
> > +{
> > + DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, kprobe_opts);
> > + struct bpf_link *kprobe_link, *kretprobe_link;
> > + struct test_attach_probe_manual *skel;
> > +
> > + skel = test_attach_probe_manual__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "skel_kprobe_manual_open_and_load"))
> > + return;
> > +
> > + /* manual-attach kprobe/kretprobe */
> > + kprobe_opts.attach_mode = PROBE_ATTACH_MODE_LEGACY;
> > + kprobe_opts.retprobe = false;
> > + kprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kprobe,
> > + "bpf_kfunc_looooooooooooooooooooooooooooooong_name",
> > + &kprobe_opts);
> > + if (!ASSERT_OK_PTR(kprobe_link, "attach_kprobe_long_event_name"))
> > + goto cleanup;
> > + skel->links.handle_kprobe = kprobe_link;
> > +
> > + kprobe_opts.retprobe = true;
> > + kretprobe_link = bpf_program__attach_kprobe_opts(skel->progs.handle_kretprobe,
> > + "bpf_kfunc_looooooooooooooooooooooooooooooong_name",
> > + &kprobe_opts);
> > + if (!ASSERT_OK_PTR(kretprobe_link, "attach_kretprobe_long_event_name"))
> > + goto cleanup;
> > + skel->links.handle_kretprobe = kretprobe_link;
> > +
> > +cleanup:
> > + test_attach_probe_manual__destroy(skel);
> > +}
> > +
> > static void test_attach_probe_auto(struct test_attach_probe *skel)
> > {
> > struct bpf_link *uprobe_err_link;
> > @@ -371,6 +404,8 @@ void test_attach_probe(void)
> >
> > if (test__start_subtest("uprobe-long_name"))
> > test_attach_uprobe_long_event_name();
> > + if (test__start_subtest("kprobe-long_name"))
> > + test_attach_kprobe_long_event_name();
> >
> > cleanup:
> > test_attach_probe__destroy(skel);
> > diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> > index f38eaf0d35ef..439f6c2b2456 100644
> > --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> > @@ -1053,6 +1053,10 @@ __bpf_kfunc int bpf_kfunc_st_ops_inc10(struct st_ops_args *args)
> > return args->a;
> > }
> >
> > +__bpf_kfunc void bpf_kfunc_looooooooooooooooooooooooooooooong_name(void)
> > +{
> > +}
>
> does it need to be a kfunc? IIUC it just needs to be a normal kernel/module function
>
> jirka
>
Indeed, so is it okay if I make the following modifications:
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -134,6 +134,10 @@ bpf_testmod_test_arg_ptr_to_struct(struct bpf_testmod_struct_arg_1 *a) {
return bpf_testmod_test_struct_arg_result;
}
+noinline void bpf_testmod_looooooooooooooooooooooooooooooong_name(void)
+{
+}
+
__bpf_kfunc void
bpf_testmod_test_mod_kfunc(int i)
Thanks.
> > +
> > BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
> > BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
> > BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
> > @@ -1093,6 +1097,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_prologue, KF_TRUSTED_ARGS | KF_SLEEPABL
> > BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
> > BTF_ID_FLAGS(func, bpf_kfunc_st_ops_test_pro_epilogue, KF_TRUSTED_ARGS | KF_SLEEPABLE)
> > BTF_ID_FLAGS(func, bpf_kfunc_st_ops_inc10, KF_TRUSTED_ARGS)
> > +BTF_ID_FLAGS(func, bpf_kfunc_looooooooooooooooooooooooooooooong_name)
> > BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
> >
> > static int bpf_testmod_ops_init(struct btf *btf)
> > diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> > index b58817938deb..e5b833140418 100644
> > --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> > +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> > @@ -159,4 +159,6 @@ void bpf_kfunc_trusted_task_test(struct task_struct *ptr) __ksym;
> > void bpf_kfunc_trusted_num_test(int *ptr) __ksym;
> > void bpf_kfunc_rcu_task_test(struct task_struct *ptr) __ksym;
> >
> > +void bpf_kfunc_looooooooooooooooooooooooooooooong_name(void) __ksym;
> > +
> > #endif /* _BPF_TESTMOD_KFUNC_H */
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error
2025-04-15 2:01 ` Feng Yang
@ 2025-04-15 7:20 ` Jiri Olsa
0 siblings, 0 replies; 9+ messages in thread
From: Jiri Olsa @ 2025-04-15 7:20 UTC (permalink / raw)
To: Feng Yang
Cc: olsajiri, andrii, ast, bpf, daniel, eddyz87, haoluo, hengqi.chen,
john.fastabend, kpsingh, linux-kernel, martin.lau, sdf, song,
yonghong.song
On Tue, Apr 15, 2025 at 10:01:15AM +0800, Feng Yang wrote:
> On Mon, 14 Apr 2025 13:43:38 +0200 Jiri Olsa <olsajiri@gmail.com> wrote:
> > On Mon, Apr 14, 2025 at 05:34:00PM +0800, Feng Yang wrote:
> > > From: Feng Yang <yangfeng@kylinos.cn>
> > >
> > > When the binary path is excessively long, the generated probe_name in libbpf
> > > exceeds the kernel's MAX_EVENT_NAME_LEN limit (64 bytes).
> > > This causes legacy uprobe event attachment to fail with error code -22.
> > >
> > > Before Fix:
> > > ./test_progs -t attach_probe/kprobe-long_name
> > > ......
> > > libbpf: failed to add legacy kprobe event for 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0': -EINVAL
> > > libbpf: prog 'handle_kprobe': failed to create kprobe 'bpf_kfunc_looooooooooooooooooooooooooooooong_name+0x0' perf event: -EINVAL
> > > test_attach_kprobe_long_event_name:FAIL:attach_kprobe_long_event_name unexpected error: -22
> > > test_attach_probe:PASS:uprobe_ref_ctr_cleanup 0 nsec
> > > #13/11 attach_probe/kprobe-long_name:FAIL
> > > #13 attach_probe:FAIL
> > >
> > > ./test_progs -t attach_probe/uprobe-long_name
> > > ......
> > > libbpf: failed to add legacy uprobe event for /root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9: -EINVAL
> > > libbpf: prog 'handle_uprobe': failed to create uprobe '/root/linux-bpf/bpf-next/tools/testing/selftests/bpf/test_progs:0x13efd9' perf event: -EINVAL
> > > test_attach_uprobe_long_event_name:FAIL:attach_uprobe_long_event_name unexpected error: -22
> > > #13/10 attach_probe/uprobe-long_name:FAIL
> > > #13 attach_probe:FAIL
> > > After Fix:
> > > ./test_progs -t attach_probe/uprobe-long_name
> > > #13/10 attach_probe/uprobe-long_name:OK
> > > #13 attach_probe:OK
> > > Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
> > >
> > > ./test_progs -t attach_probe/kprobe-long_name
> > > #13/11 attach_probe/kprobe-long_name:OK
> > > #13 attach_probe:OK
> > > Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
> > >
> > > Fixes: 46ed5fc33db9 ("libbpf: Refactor and simplify legacy kprobe code")
> > > Fixes: cc10623c6810 ("libbpf: Add legacy uprobe attaching support")
> > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> > > Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
> > > ---
> > > tools/lib/bpf/libbpf.c | 19 ++++++++++++-------
> > > 1 file changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index b2591f5cab65..9e047641e001 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -60,6 +60,8 @@
> > > #define BPF_FS_MAGIC 0xcafe4a11
> > > #endif
> > >
> > > +#define MAX_EVENT_NAME_LEN 64
> > > +
> > > #define BPF_FS_DEFAULT_PATH "/sys/fs/bpf"
> > >
> > > #define BPF_INSN_SZ (sizeof(struct bpf_insn))
> > > @@ -11142,10 +11144,10 @@ static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
> > > static int index = 0;
> > > int i;
> > >
> > > - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx_%d", getpid(), kfunc_name, offset,
> > > - __sync_fetch_and_add(&index, 1));
> > > + snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
> > > + __sync_fetch_and_add(&index, 1), kfunc_name, offset);
> >
> > so the fix is to move unique id before kfunc_name to make sure it gets
> > to the event name right? would be great to have it in changelog
> >
>
> Yes, defining MAX_EVENT_NAME_LEN ensures event names are truncated via snprintf
> to prevent exceeding the maximum length limit.
> Moving the unique id before kfunc_name avoids truncating the id.
> Regarding the changelog: Should this information go into the commit message of the patch, or somewhere else?
having this in changelog would help
>
> >
> > >
> > > - /* sanitize binary_path in the probe name */
> > > + /* sanitize kfunc_name in the probe name */
> > > for (i = 0; buf[i]; i++) {
> > > if (!isalnum(buf[i]))
> > > buf[i] = '_';
> > > @@ -11270,7 +11272,7 @@ int probe_kern_syscall_wrapper(int token_fd)
> > >
> > > return pfd >= 0 ? 1 : 0;
> > > } else { /* legacy mode */
> > > - char probe_name[128];
> > > + char probe_name[MAX_EVENT_NAME_LEN];
> > >
> > > gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
> > > if (add_kprobe_event_legacy(probe_name, false, syscall_name, 0) < 0)
> > > @@ -11328,7 +11330,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> > > func_name, offset,
> > > -1 /* pid */, 0 /* ref_ctr_off */);
> > > } else {
> > > - char probe_name[256];
> > > + char probe_name[MAX_EVENT_NAME_LEN];
> > >
> > > gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name),
> > > func_name, offset);
> > > @@ -11878,9 +11880,12 @@ static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, stru
> > > static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
> > > const char *binary_path, uint64_t offset)
> > > {
> > > + static int index = 0;
> > > int i;
> > >
> > > - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(), binary_path, (size_t)offset);
> > > + snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
> > > + __sync_fetch_and_add(&index, 1),
> > > + basename((void *)binary_path), (size_t)offset);
> >
> > gen_kprobe_legacy_event_name and gen_uprobe_legacy_event_name seem to
> > be identical now, maybe we can have just one ?
> >
> > thanks,
> > jirka
> >
>
> The gen_uprobe_legacy_event_name function includes an extra basename compared to gen_kprobe_legacy_event_name,
> as the prefixes of binary_path are often too similar to distinguish easily.
> When merging these two into a single function, is it acceptable to pass basename((void *)binary_path)
> directly during the uprobe invocation, or should we remove the addition of basename? Thank you!
I think basename is fine, perhaps just pass it as argument
like below (on top of your change, untested)
jirka
---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 9e047641e001..93e804b25da1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11138,14 +11138,13 @@ static const char *tracefs_available_filter_functions_addrs(void)
: TRACEFS"/available_filter_functions_addrs";
}
-static void gen_kprobe_legacy_event_name(char *buf, size_t buf_sz,
- const char *kfunc_name, size_t offset)
+static void gen_legacy_event_name(char *buf, size_t buf_sz, const char *name, size_t offset)
{
static int index = 0;
int i;
snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
- __sync_fetch_and_add(&index, 1), kfunc_name, offset);
+ __sync_fetch_and_add(&index, 1), name, offset);
/* sanitize kfunc_name in the probe name */
for (i = 0; buf[i]; i++) {
@@ -11274,7 +11273,7 @@ int probe_kern_syscall_wrapper(int token_fd)
} else { /* legacy mode */
char probe_name[MAX_EVENT_NAME_LEN];
- gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
+ gen_legacy_event_name(probe_name, sizeof(probe_name), syscall_name, 0);
if (add_kprobe_event_legacy(probe_name, false, syscall_name, 0) < 0)
return 0;
@@ -11332,8 +11331,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
} else {
char probe_name[MAX_EVENT_NAME_LEN];
- gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name),
- func_name, offset);
+ gen_legacy_event_name(probe_name, sizeof(probe_name), func_name, offset);
legacy_probe = strdup(probe_name);
if (!legacy_probe)
@@ -11877,23 +11875,6 @@ static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, stru
return ret;
}
-static void gen_uprobe_legacy_event_name(char *buf, size_t buf_sz,
- const char *binary_path, uint64_t offset)
-{
- static int index = 0;
- int i;
-
- snprintf(buf, buf_sz, "libbpf_%u_%d_%s_0x%zx", getpid(),
- __sync_fetch_and_add(&index, 1),
- basename((void *)binary_path), (size_t)offset);
-
- /* sanitize binary_path in the probe name */
- for (i = 0; buf[i]; i++) {
- if (!isalnum(buf[i]))
- buf[i] = '_';
- }
-}
-
static inline int add_uprobe_event_legacy(const char *probe_name, bool retprobe,
const char *binary_path, size_t offset)
{
@@ -12322,8 +12303,8 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
if (ref_ctr_off)
return libbpf_err_ptr(-EINVAL);
- gen_uprobe_legacy_event_name(probe_name, sizeof(probe_name),
- binary_path, func_offset);
+ gen_legacy_event_name(probe_name, sizeof(probe_name),
+ basename((char *) binary_path), func_offset);
legacy_probe = strdup(probe_name);
if (!legacy_probe)
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-04-15 7:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-14 9:33 [PATCH v3 bpf-next 0/3] libbpf: Fix event name too long error and add tests Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error Feng Yang
2025-04-14 11:43 ` Jiri Olsa
2025-04-15 2:01 ` Feng Yang
2025-04-15 7:20 ` Jiri Olsa
2025-04-14 9:34 ` [PATCH v3 bpf-next 2/3] selftests/bpf: Add test for attaching uprobe with long event names Feng Yang
2025-04-14 9:34 ` [PATCH v3 bpf-next 3/3] selftests/bpf: Add test for attaching kprobe " Feng Yang
2025-04-14 11:47 ` Jiri Olsa
2025-04-15 2:52 ` Feng Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox