Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support for multi kprobes

[Andrii Nakryiko]

On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Fri, Mar 04, 2022 at 03:11:19PM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > >
> > > Adding support to bpf_program__attach_kprobe_opts to attach kprobes
> > > to multiple functions.
> > >
> > > If the kprobe program has BPF_TRACE_KPROBE_MULTI as expected_attach_type
> > > it will use the new kprobe_multi link to attach the program. In this case
> > > it will use 'func_name' as pattern for functions to attach.
> > >
> > > Adding also new section types 'kprobe.multi' and kretprobe.multi'
> > > that allows to specify wildcards (*?) for functions, like:
> > >
> > >   SEC("kprobe.multi/bpf_fentry_test*")
> > >   SEC("kretprobe.multi/bpf_fentry_test?")
> > >
> > > This will set kprobe's expected_attach_type to BPF_TRACE_KPROBE_MULTI,
> > > and attach it to functions provided by the function pattern.
> > >
> > > Using glob_match from selftests/bpf/test_progs.c and adding support to
> > > match '?' based on original perf code.
> > >
> > > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > Cc: Yucong Sun <fallentree@xxxxxx>
> > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 125 insertions(+), 5 deletions(-)
> > >
> >
> > [...]
> >
> > > +static struct bpf_link *
> > > +attach_kprobe_multi_opts(const struct bpf_program *prog,
> > > +                  const char *func_pattern,
> > > +                  const struct bpf_kprobe_opts *kopts)
> > > +{
> > > +       DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
> >
> > nit: just LIBBPF_OPTS
>
> ok
>
> >
> >
> > > +       struct kprobe_multi_resolve res = {
> > > +               .name = func_pattern,
> > > +       };
> > > +       struct bpf_link *link = NULL;
> > > +       char errmsg[STRERR_BUFSIZE];
> > > +       int err, link_fd, prog_fd;
> > > +       bool retprobe;
> > > +
> > > +       err = libbpf_kallsyms_parse(resolve_kprobe_multi_cb, &res);
> >
> > hm... I think as a generic API we should support three modes of
> > specifying attachment target:
> >
> >
> > 1. glob-based (very convenient, I agree)
> > 2. array of function names (very convenient when I know specific set
> > of functions)
> > 3. array of addresses (advanced use case, so probably will be rarely used).
> >
> >
> >
> > So I wonder if it's better to have a separate
> > bpf_program__attach_kprobe_multi() API for this, instead of doing both
> > inside bpf_program__attach_kprobe()...
> >
> > In such case bpf_program__attach_kprobe() could either fail if
> > expected attach type is BPF_TRACE_KPROBE_MULTI or it can redirect to
> > attach_kprobe_multi with func_name as a pattern or just single
> > function (let's think which one makes more sense)
> >
> > Let's at least think about this
>
> I think it would make the code more clear, how about this:
>
>         struct bpf_kprobe_multi_opts {
>                 /* size of this struct, for forward/backward compatiblity */
>                 size_t sz;
>
>                 const char **funcs;

naming nit: func_names (to oppose it to "func_pattern")? Or just
"names" to be in line with "addrs" (but then "pattern" instead of
"func_pattern"? with kprobe it's always about functions, so this
"func_" everywhere is a bit redundant)

>                 const unsigned long *addrs;
>                 const u64 *cookies;
>                 int cnt;

nit: let's use size_t


>                 bool retprobe;
>                 size_t :0;
>         };
>
>         bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
>                                               const char *pattern,
>                                               const struct bpf_kprobe_multi_opts *opts);
>
>
> if pattern is NULL we'd use opts data:
>
>         bpf_program__attach_kprobe_multi_opts(prog, "ksys_*", NULL);
>         bpf_program__attach_kprobe_multi_opts(prog, NULL, &opts);
>
> to have '2. array of function names' as direct function argument,
> we'd need to add 'cnt' as well, so I think it's better to have it
> in opts, and have just pattern for quick/convenient call without opts
>

yeah, naming pattern as direct argument for common use case makes
sense. Let's go with this scheme


[...]