Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 6 Mar 2022 18:29:24 +0100
From:   Jiri Olsa <olsajiri@gmail.com>
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Masami Hiramatsu <mhiramat@redhat.com>,
        Yucong Sun <fallentree@fb.com>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts
 support for multi kprobes
Message-ID: <YiTvdMGi6GA7i2Ex@krava>
References: <20220222170600.611515-1-jolsa@kernel.org>
 <20220222170600.611515-9-jolsa@kernel.org>
 <CAEf4Bza0qRAzA7WmtPD4US4Kur3qf3X+LC5uowr_H3Y-_pLfCA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEf4Bza0qRAzA7WmtPD4US4Kur3qf3X+LC5uowr_H3Y-_pLfCA@mail.gmail.com>
Precedence: bulk

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@kernel.org> 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@redhat.com>
> > Cc: Yucong Sun <fallentree@fb.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  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;
		const unsigned long *addrs;
		const u64 *cookies;
		int cnt;
		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

> 
> 
> > +       if (err)
> > +               goto error;
> > +       if (!res.cnt) {
> > +               err = -ENOENT;
> > +               goto error;
> > +       }
> > +
> > +       retprobe = OPTS_GET(kopts, retprobe, false);
> > +
> > +       opts.kprobe_multi.addrs = ptr_to_u64(res.addrs);
> > +       opts.kprobe_multi.cnt = res.cnt;
> > +       opts.flags = retprobe ? BPF_F_KPROBE_MULTI_RETURN : 0;
> 
> this should be opts.kprobe_multi.flags

ugh, now I'm curious how kretprobes passed in tests ;-)

> 
> > +
> > +       link = calloc(1, sizeof(*link));
> > +       if (!link) {
> > +               err = -ENOMEM;
> > +               goto error;
> > +       }
> > +       link->detach = &bpf_link__detach_fd;
> > +
> > +       prog_fd = bpf_program__fd(prog);
> > +       link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &opts);
> > +       if (link_fd < 0) {
> > +               err = -errno;
> > +               pr_warn("prog '%s': failed to attach to %s: %s\n",
> 
> "to attach multi-kprobe for '%s': %s" ?

ok

> 
> > +                       prog->name, res.name,
> > +                       libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > +               goto error;
> > +       }
> > +       link->fd = link_fd;
> > +       free(res.addrs);
> > +       return link;
> > +
> > +error:
> > +       free(link);
> > +       free(res.addrs);
> > +       return libbpf_err_ptr(err);
> > +}
> > +
> >  struct bpf_link *
> >  bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> >                                 const char *func_name,
> > @@ -10054,6 +10163,9 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
> >         if (!OPTS_VALID(opts, bpf_kprobe_opts))
> >                 return libbpf_err_ptr(-EINVAL);
> >
> > +       if (prog->expected_attach_type == BPF_TRACE_KPROBE_MULTI)
> > +               return attach_kprobe_multi_opts(prog, func_name, opts);
> > +
> >         retprobe = OPTS_GET(opts, retprobe, false);
> >         offset = OPTS_GET(opts, offset, 0);
> >         pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
> 
> see how you don't support cookies (plural) and this offset doesn't
> make sense for multi-kprobe. Separate API is necessary to expose all
> the possibilities and functionality.
> 
> > @@ -10122,19 +10234,27 @@ struct bpf_link *bpf_program__attach_kprobe(const struct bpf_program *prog,
> >  static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie)
> >  {
> >         DECLARE_LIBBPF_OPTS(bpf_kprobe_opts, opts);
> > +       const char *func_name = NULL;
> >         unsigned long offset = 0;
> >         struct bpf_link *link;
> > -       const char *func_name;
> >         char *func;
> >         int n, err;
> >
> > -       opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe/");
> > -       if (opts.retprobe)
> > +       opts.retprobe = str_has_pfx(prog->sec_name, "kretprobe");
> > +
> > +       if (str_has_pfx(prog->sec_name, "kretprobe/"))
> >                 func_name = prog->sec_name + sizeof("kretprobe/") - 1;
> > -       else
> > +       else if (str_has_pfx(prog->sec_name, "kprobe/"))
> >                 func_name = prog->sec_name + sizeof("kprobe/") - 1;
> > +       else if (str_has_pfx(prog->sec_name, "kretprobe.multi/"))
> > +               func_name = prog->sec_name + sizeof("kretprobe.multi/") - 1;
> > +       else if (str_has_pfx(prog->sec_name, "kprobe.multi/"))
> > +               func_name = prog->sec_name + sizeof("kprobe.multi/") - 1;
> 
> starts to feel that we should find '/' and then do strcmp(), instead
> of this duplication of strings?

ok, another reason to separate the api

thanks,
jirka