| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z_4Itg5H7-410o4d@krava>
Date: Tue, 15 Apr 2025 09:20:22 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Feng Yang <yangfeng59949@....com>
Cc: olsajiri@...il.com, andrii@...nel.org, ast@...nel.org,
bpf@...r.kernel.org, daniel@...earbox.net, eddyz87@...il.com,
haoluo@...gle.com, hengqi.chen@...il.com, john.fastabend@...il.com,
kpsingh@...nel.org, linux-kernel@...r.kernel.org,
martin.lau@...ux.dev, sdf@...ichev.me, song@...nel.org,
yonghong.song@...ux.dev
Subject: Re: [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error
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@...il.com> wrote:
> > On Mon, Apr 14, 2025 at 05:34:00PM +0800, Feng Yang wrote:
> > > From: Feng Yang <yangfeng@...inos.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@...il.com>
> > > Signed-off-by: Feng Yang <yangfeng@...inos.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)
Powered by blists - more mailing lists