Re: [PATCH v3 bpf-next 1/3] libbpf: Fix event name too long error

[Feng Yang]

On Mon, 14 Apr 2025 13:43:38 +0200 Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> On Mon, Apr 14, 2025 at 05:34:00PM +0800, Feng Yang 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.
> > 
> > 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@xxxxxxxxx>
> > Signed-off-by: Feng Yang <yangfeng@xxxxxxxxxx>
> > ---
> >  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
> >