Re: [PATCHv2 bpf-next 11/24] libbpf: Add bpf_program__attach_uprobe_multi_opts function

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jun 23, 2023 at 01:40:08PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 20, 2023 at 1:37 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding bpf_program__attach_uprobe_multi_opts function that
> > allows to attach multiple uprobes with uprobe_multi link.
> >
> > The user can specify uprobes with direct arguments:
> >
> >   binary_path/func_pattern
> >
> > or with struct bpf_uprobe_multi_opts opts argument fields:
> >
> >   const char *path;
> >   const char **syms;
> >   const unsigned long *offsets;
> >   const unsigned long *ref_ctr_offsets;
> >
> > User can specify 3 mutually exclusive set of incputs:
> 
> typo: inputs
> 
> >
> >  1) use only binary_path/func_pattern aruments
> 
> typo: arguments

ok

> 
> >
> >  2) use only opts argument with allowed combinations of:
> >     path/offsets/ref_ctr_offsets/cookies/cnt
> >
> >  3) use binary_path with allowed combinations of:
> >     syms/offsets/ref_ctr_offsets/cookies/cnt
> >
> 
> why do we need this duplication of binary_path and opts.path? same for
> pid and opts.pid?

right, will remove opts.path|pid

> 
> > Any other usage results in error.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c   | 131 +++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/libbpf.h   |  31 +++++++++
> >  tools/lib/bpf/libbpf.map |   1 +
> >  3 files changed, 163 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 3e5c88caf5d5..d972cea4c658 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -11402,6 +11402,137 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
> >         return -ENOENT;
> >  }
> >
> > +struct bpf_link *
> > +bpf_program__attach_uprobe_multi_opts(const struct bpf_program *prog,
> 
> let's call it just bpf_program__attach_uprobe_multi()
> 
> we should have done that with bpf_program__attach_kprobe_multi_opts()
> as well. Generally, if the only variant of API is the one with opts,
> then there is no point in adding "_opts" suffix to the API name, IMO

ok

> 
> 
> > +                                     pid_t pid,
> > +                                     const char *binary_path,
> > +                                     const char *func_pattern,
> > +                                     const struct bpf_uprobe_multi_opts *opts)
> > +{
> > +       const unsigned long *ref_ctr_offsets = NULL, *offsets = NULL;
> > +       LIBBPF_OPTS(bpf_link_create_opts, lopts);
> > +       unsigned long *resolved_offsets = NULL;
> > +       const char **resolved_symbols = NULL;
> > +       int err = 0, link_fd, prog_fd;
> > +       struct bpf_link *link = NULL;
> > +       char errmsg[STRERR_BUFSIZE];
> > +       const char *path, **syms;
> > +       char full_path[PATH_MAX];
> > +       const __u64 *cookies;
> > +       bool has_pattern;
> > +       bool retprobe;
> > +       size_t cnt;
> > +
> > +       if (!OPTS_VALID(opts, bpf_uprobe_multi_opts))
> > +               return libbpf_err_ptr(-EINVAL);
> > +
> > +       path = OPTS_GET(opts, path, NULL);
> > +       syms = OPTS_GET(opts, syms, NULL);
> > +       offsets = OPTS_GET(opts, offsets, NULL);
> > +       ref_ctr_offsets = OPTS_GET(opts, ref_ctr_offsets, NULL);
> > +       cookies = OPTS_GET(opts, cookies, NULL);
> > +       cnt = OPTS_GET(opts, cnt, 0);
> > +
> > +       /*
> > +        * User can specify 3 mutually exclusive set of incputs:
> > +        *
> > +        * 1) use only binary_path/func_pattern aruments
> 
> same typos
> 
> > +        *
> > +        * 2) use only opts argument with allowed combinations of:
> > +        *    path/offsets/ref_ctr_offsets/cookies/cnt
> > +        *
> > +        * 3) use binary_path with allowed combinations of:
> > +        *    syms/offsets/ref_ctr_offsets/cookies/cnt
> > +        *
> > +        * Any other usage results in error.
> > +        */
> > +
> 
> 
> Looking at this part (sorry, I already trimmed reply before realizing
> I want to comment on this):
> 
> 
> +               err = elf_find_pattern_func_offset(binary_path, func_pattern,
> +                                                  &resolved_symbols,
> &resolved_offsets,
> +                                                  &cnt);
> +               if (err < 0)
> +                       return libbpf_err_ptr(err);
> +               offsets = resolved_offsets;
> +       } else if (syms) {
> +               err = elf_find_multi_func_offset(binary_path, cnt,
> syms, &resolved_offsets);
> 
> Few thoughts.
> 
> First, we are not using resolved_symbols returned from
> elf_find_pattern_func_offset(), do we? If not, let's drop it.

I think I used that in bpftrace with the RFC version,
where I wanted to display found symbols.. but there's
no use for it in libbpf for now.. will remove it

> 
> Second, I'm not a big fan of chosen naming. Maybe something like
> elf_resolve_{pattern,syms}_offsets?

sure

SNIP

> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_uprobe_multi_opts(const struct bpf_program *prog,
> > +                                     pid_t pid,
> > +                                     const char *binary_path,
> > +                                     const char *func_pattern,
> > +                                     const struct bpf_uprobe_multi_opts *opts);
> > +
> >  struct bpf_ksyscall_opts {
> >         /* size of this struct, for forward/backward compatibility */
> >         size_t sz;
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 7521a2fb7626..81558ef1bc38 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -390,6 +390,7 @@ LIBBPF_1.2.0 {
> >                 bpf_link_get_info_by_fd;
> >                 bpf_map_get_info_by_fd;
> >                 bpf_prog_get_info_by_fd;
> > +               bpf_program__attach_uprobe_multi_opts;
> 
> should be in 1.3.0?

yes, will fix

thanks,
jirka