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:   Tue, 8 Mar 2022 15:23:34 +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: <Yidm5vcehK7k1B2O@krava>
References: <20220222170600.611515-1-jolsa@kernel.org>
 <20220222170600.611515-9-jolsa@kernel.org>
 <CAEf4Bza0qRAzA7WmtPD4US4Kur3qf3X+LC5uowr_H3Y-_pLfCA@mail.gmail.com>
 <YiTvdMGi6GA7i2Ex@krava>
 <CAEf4BzZZy6XSb2naSam+W=_wY6JviX6Vz30N7mSg=xYZW_TxQA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEf4BzZZy6XSb2naSam+W=_wY6JviX6Vz30N7mSg=xYZW_TxQA@mail.gmail.com>
Precedence: bulk

On Mon, Mar 07, 2022 at 05:28:54PM -0800, Andrii Nakryiko wrote:
> On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <olsajiri@gmail.com> 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@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;
> 
> 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)

ok

> 
> >                 const unsigned long *addrs;
> >                 const u64 *cookies;
> >                 int cnt;
> 
> nit: let's use size_t

ok

> 
> 
> >                 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

great, I'll make the changes

thanks,
jirka