On Tue, Apr 15, 2025 at 6:44 PM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote:
>
> Hi Andrii,
>
> On Wed, Apr 16, 2025 at 7:05 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Apr 15, 2025 at 2:40 AM Feng Yang <yangfeng59949@xxxxxxx> wrote:
> > >
> > > From: Feng Yang <yangfeng@xxxxxxxxxx>
> > >
> > > 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.
> > >
> > > The fix reorders the fields to place the unique ID before the name.
> > > This ensures that even if truncation occurs via snprintf, the unique ID
> > > remains intact, preserving event name uniqueness. Additionally, explicit
> > > checks with MAX_EVENT_NAME_LEN are added to enforce length constraints.
> > >
> > > Before Fix:
> > > ./test_progs -t attach_probe/kprobe-long_name
> > > ......
> > > libbpf: failed to add legacy kprobe event for 'bpf_testmod_looooooooooooooooooooooooooooooong_name+0x0': -EINVAL
> > > libbpf: prog 'handle_kprobe': failed to create kprobe 'bpf_testmod_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@xxxxxxxxx>
> > > Signed-off-by: Feng Yang <yangfeng@xxxxxxxxxx>
> > > ---
> > > tools/lib/bpf/libbpf.c | 41 +++++++++++++++--------------------------
> > > 1 file changed, 15 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index b2591f5cab65..b7fc57ac16a6 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))
> > > @@ -11136,16 +11138,16 @@ 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_probe_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_%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), name, offset);
> > >
> > > - /* sanitize binary_path in the probe name */
> > > + /* sanitize name in the probe name */
> > > for (i = 0; buf[i]; i++) {
> > > if (!isalnum(buf[i]))
> > > buf[i] = '_';
> > > @@ -11270,9 +11272,9 @@ 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);
> > > + gen_probe_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;
> > >
> > > @@ -11328,9 +11330,9 @@ 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),
> > > + gen_probe_legacy_event_name(probe_name, sizeof(probe_name),
> > > func_name, offset);
> > >
> > > legacy_probe = strdup(probe_name);
> > > @@ -11875,20 +11877,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)
> > > -{
> > > - int i;
> > > -
> > > - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(), 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)
> > > {
> > > @@ -12312,13 +12300,14 @@ 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);
> > >
> > > - gen_uprobe_legacy_event_name(probe_name, sizeof(probe_name),
> > > - binary_path, func_offset);
> > > + gen_probe_legacy_event_name(probe_name, sizeof(probe_name),
> > > + basename((void *)binary_path),
> >
> > This patch is a nice refactoring overall and I like it. But this (void
> > *) cast on binary_path I'm not so fond of. Yes, if you read
> > smallprint, you'll see that with _GNU_SOURCE basename won't *really*
> > modify input argument, but meh.
> >
>
> This has been used in bpf_object__new() like:
>
> /* Using basename() GNU version which doesn't modify arg. */
> libbpf_strlcpy(obj->name, basename((void *)path), sizeof(obj->name));
yeah, I know, which is why I remembered this semantical quirk. Still,
in this case we are just adding a hint, so some simple strrchr() seems
adequate
>
> > Let's instead do a simple `strrchr(binary_path, '/') ?: binary_path`?
> >
> > pw-bot: cr
> >
> >
> > > + func_offset);
> > >
> > > legacy_probe = strdup(probe_name);
> > > if (!legacy_probe)
> > > --
> > > 2.43.0
> > >
