On Thu, Apr 10, 2025 at 4:27 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote:
>
> Hi Feng,
>
> On Thu, Apr 10, 2025 at 1:30 PM Feng Yang <yangfeng59949@xxxxxxx> wrote:
> >
> > From: Feng Yang <yangfeng@xxxxxxxxxx>
> >
> > If the event name is too long, it will cause an EINVAL error.
> >
> > The kernel error path is
> > probes_write
> > trace_parse_run_command
> > create_or_delete_trace_uprobe
> > trace_uprobe_create
> > trace_probe_create
> > __trace_uprobe_create
> > traceprobe_parse_event_name
> > else if (len >= MAX_EVENT_NAME_LEN)
> > Requires less than 64 bytes.
> >
>
> Please don't submit patch in a hurry.
> This patch does NOT fix the issue.
It would be good to also have a bit more human-readable explanation of
the issue.
pw-bot: cr
>
> > Signed-off-by: Feng Yang <yangfeng@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/libbpf.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index b2591f5cab65..8e48ba99f06c 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -12227,6 +12227,16 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> > return libbpf_err_ptr(err);
> > }
> >
> > +static const char *get_last_part(const char *path)
> > +{
> > + const char *last_slash = strrchr(path, '/');
> > +
> > + if (last_slash != NULL)
> > + return last_slash + 1;
> > + else
> > + return path;
> > +}
> > +
>
> Use basename(3) instead.
>
> > LIBBPF_API struct bpf_link *
> > bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> > const char *binary_path, size_t func_offset,
> > @@ -12241,7 +12251,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> > size_t ref_ctr_off;
> > int pfd, err;
> > bool retprobe, legacy;
> > - const char *func_name;
> > + const char *func_name, *binary_name;
> >
> > if (!OPTS_VALID(opts, bpf_uprobe_opts))
> > return libbpf_err_ptr(-EINVAL);
> > @@ -12254,6 +12264,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> > if (!binary_path)
> > return libbpf_err_ptr(-EINVAL);
> >
> > + binary_name = get_last_part(binary_path);
>
> What if len(binary_name) >= MAX_EVENT_NAME_LEN ?
>
> > /* Check if "binary_path" refers to an archive. */
> > archive_sep = strstr(binary_path, "!/");
> > if (archive_sep) {
> > @@ -12318,7 +12329,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> > return libbpf_err_ptr(-EINVAL);
> >
> > gen_uprobe_legacy_event_name(probe_name, sizeof(probe_name),
> > - binary_path, func_offset);
> > + binary_name, func_offset);
> >
> > legacy_probe = strdup(probe_name);
> > if (!legacy_probe)
> > --
> > 2.43.0
> >
> >
>
> FYI, when I mentioned this issue in ([0]), I tested with the following diff:
> [0]: https://github.com/iovisor/bcc/pull/5271
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b2591f5cab65..4087fc3ae62f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11142,10 +11142,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,
> + snprintf(buf, buf_sz, "libbpf_%u_%.32s_0x%zx_%d", getpid(),
> kfunc_name, offset,
> __sync_fetch_and_add(&index, 1));
>
> - /* 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 +11270,7 @@ int probe_kern_syscall_wrapper(int token_fd)
>
> return pfd >= 0 ? 1 : 0;
> } else { /* legacy mode */
> - char probe_name[128];
> + char probe_name[64];
>
> 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 +11328,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[64];
>
> gen_kprobe_legacy_event_name(probe_name, sizeof(probe_name),
> func_name, offset);
> @@ -11880,7 +11880,8 @@ static void gen_uprobe_legacy_event_name(char
> *buf, size_t buf_sz,
> {
> int i;
>
> - snprintf(buf, buf_sz, "libbpf_%u_%s_0x%zx", getpid(), binary_path,
> (size_t)offset);
> + snprintf(buf, buf_sz, "libbpf_%u_%.32s_0x%zx",
> + getpid(), basename((void *)binary_path), (size_t)offset);
>
> /* sanitize binary_path in the probe name */
> for (i = 0; buf[i]; i++) {
> @@ -12312,7 +12313,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[64];
>
> if (ref_ctr_off)
> return libbpf_err_ptr(-EINVAL);
> --
> 2.43.0
>
> Cheers,
> ---
> Hengqi
