Re: [PATCH RFCv2 bpf-next 3/4] libbpf: Add support for kprobe multi wrapper attach

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Feb 28, 2024 at 05:23:57PM -0800, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 1:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support for specify wrapper mode in bpf_kprobe_multi_opts
> > struct object and new bpf program loader section:
> >
> >  SEC("kprobe.wrapper/bpf_fentry_test*")
> >
> > to load program as kprobe multi wrapper.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c | 38 +++++++++++++++++++++++++++++++++++---
> >  tools/lib/bpf/libbpf.h |  4 +++-
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 01f407591a92..5416d784c857 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8944,6 +8944,7 @@ static int attach_tp(const struct bpf_program *prog, long cookie, struct bpf_lin
> >  static int attach_raw_tp(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_trace(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> > +static int attach_kprobe_wrapper(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> >  static int attach_iter(const struct bpf_program *prog, long cookie, struct bpf_link **link);
> > @@ -8960,6 +8961,7 @@ static const struct bpf_sec_def section_defs[] = {
> >         SEC_DEF("uretprobe.s+",         KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
> >         SEC_DEF("kprobe.multi+",        KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> >         SEC_DEF("kretprobe.multi+",     KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
> > +       SEC_DEF("kprobe.wrapper+",      KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_wrapper),
> >         SEC_DEF("uprobe.multi+",        KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
> >         SEC_DEF("uretprobe.multi+",     KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_NONE, attach_uprobe_multi),
> >         SEC_DEF("uprobe.multi.s+",      KPROBE, BPF_TRACE_UPROBE_MULTI, SEC_SLEEPABLE, attach_uprobe_multi),
> > @@ -11034,7 +11036,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >         int err, link_fd, prog_fd;
> >         const __u64 *cookies;
> >         const char **syms;
> > -       bool retprobe;
> > +       __u32 flags = 0;
> >         size_t cnt;
> >
> >         if (!OPTS_VALID(opts, bpf_kprobe_multi_opts))
> > @@ -11065,13 +11067,16 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >                 cnt = res.cnt;
> >         }
> >
> > -       retprobe = OPTS_GET(opts, retprobe, false);
> > +       if (OPTS_GET(opts, retprobe, false))
> > +               flags |= BPF_F_KPROBE_MULTI_RETURN;
> > +       if (OPTS_GET(opts, wrapper, false))
> > +               flags |= BPF_F_KPROBE_MULTI_WRAPPER;
> 
> probably error out if both retprobe and wrapper are set?

ok

> 
> >
> >         lopts.kprobe_multi.syms = syms;
> >         lopts.kprobe_multi.addrs = addrs;
> >         lopts.kprobe_multi.cookies = cookies;
> >         lopts.kprobe_multi.cnt = cnt;
> > -       lopts.kprobe_multi.flags = retprobe ? BPF_F_KPROBE_MULTI_RETURN : 0;
> > +       lopts.kprobe_multi.flags = flags;
> >
> >         link = calloc(1, sizeof(*link));
> >         if (!link) {
> > @@ -11187,6 +11192,33 @@ static int attach_kprobe_multi(const struct bpf_program *prog, long cookie, stru
> >         return libbpf_get_error(*link);
> >  }
> >
> > +static int attach_kprobe_wrapper(const struct bpf_program *prog, long cookie, struct bpf_link **link)
> > +{
> > +       LIBBPF_OPTS(bpf_kprobe_multi_opts, opts,
> > +               .wrapper = true,
> > +       );
> 
> nit: keep on a single line?

ok

> 
> > +       const char *spec;
> > +       char *pattern;
> > +       int n;
> > +
> > +       *link = NULL;
> > +
> > +       /* no auto-attach for SEC("kprobe.wrapper") */
> > +       if (strcmp(prog->sec_name, "kprobe.wrapper") == 0)
> > +               return 0;
> > +
> > +       spec = prog->sec_name + sizeof("kprobe.wrapper/") - 1;
> > +       n = sscanf(spec, "%m[a-zA-Z0-9_.*?]", &pattern);
> > +       if (n < 1) {
> > +               pr_warn("kprobe wrapper pattern is invalid: %s\n", pattern);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *link = bpf_program__attach_kprobe_multi_opts(prog, pattern, &opts);
> > +       free(pattern);
> 
> is it guaranteed that free() won't clobber errno? or should we record
> it right after attach call (and stop using libbpf_get_error())?

hum, I copy&pasted from the attach_kprobe_multi, so did not think of that ;-)

anyway man page says:
  The free() function returns no value, and preserves errno.

so we're good

thanks,
jirka

> 
> 
> > +       return libbpf_get_error(*link);
> > +}
> > +
> >  static int attach_uprobe_multi(const struct bpf_program *prog, long cookie, struct bpf_link **link)
> >  {
> >         char *probe_type = NULL, *binary_path = NULL, *func_name = NULL;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 5723cbbfcc41..72f4e3ad295f 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -539,10 +539,12 @@ struct bpf_kprobe_multi_opts {
> >         size_t cnt;
> >         /* create return kprobes */
> >         bool retprobe;
> > +       /* create wrapper kprobes */
> > +       bool wrapper;
> >         size_t :0;
> >  };
> >
> > -#define bpf_kprobe_multi_opts__last_field retprobe
> > +#define bpf_kprobe_multi_opts__last_field wrapper
> >
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> > --
> > 2.43.2
> >