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 <[email protected]>
Cc: Yucong Sun <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/lib/bpf/libbpf.c | 130 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 125 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fb530b004a0d..9bee2d70b99d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8622,6 +8622,8 @@ static const struct bpf_sec_def section_defs[] = {
SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE),
SEC_DEF("kretprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
SEC_DEF("uretprobe/", KPROBE, 0, SEC_NONE),
+ SEC_DEF("kprobe.multi/", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe),
+ SEC_DEF("kretprobe.multi/", KPROBE, BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe),
SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE | SEC_SLOPPY_PFX | SEC_DEPRECATED),
SEC_DEF("action", SCHED_ACT, 0, SEC_NONE | SEC_SLOPPY_PFX),
@@ -10038,6 +10040,113 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe,
return pfd;
}
+/* Adapted from perf/util/string.c */
+static bool glob_match(const char *str, const char *pat)
+{
+ while (*str && *pat && *pat != '*') {
+ if (*pat == '?') { /* Matches any single character */
+ str++;
+ pat++;
+ continue;
+ }
+ if (*str != *pat)
+ return false;
+ str++;
+ pat++;
+ }
+ /* Check wild card */
+ if (*pat == '*') {
+ while (*pat == '*')
+ pat++;
+ if (!*pat) /* Tail wild card matches all */
+ return true;
+ while (*str)
+ if (glob_match(str++, pat))
+ return true;
+ }
+ return !*str && !*pat;
+}
+
+struct kprobe_multi_resolve {
+ const char *name;
+ __u64 *addrs;
+ size_t cap;
+ size_t cnt;
+};
+
+static int
+resolve_kprobe_multi_cb(unsigned long long sym_addr, char sym_type,
+ const char *sym_name, void *ctx)
+{
+ struct kprobe_multi_resolve *res = ctx;
+ int err;
+
+ if (!glob_match(sym_name, res->name))
+ return 0;
+
+ err = libbpf_ensure_mem((void **)&res->addrs, &res->cap, sizeof(__u64),
+ res->cnt + 1);
+ if (err)
+ return err;
+
+ res->addrs[res->cnt++] = sym_addr;
+ return 0;
+}
+
+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);
+ 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);
+ 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;
+
+ 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",
+ 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);
@@ -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;
+
+ if (!func_name)
+ return libbpf_err_ptr(-EINVAL);
- n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
+ n = sscanf(func_name, "%m[a-zA-Z0-9_.*?]+%li", &func, &offset);
if (n < 1) {
err = -EINVAL;
pr_warn("kprobe name is invalid: %s\n", func_name);
--
2.35.1
On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> Cc: Yucong Sun <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> 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
> + 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
> + 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
> +
> + 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" ?
> + 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?
> +
> + if (!func_name)
> + return libbpf_err_ptr(-EINVAL);
>
> - n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
> + n = sscanf(func_name, "%m[a-zA-Z0-9_.*?]+%li", &func, &offset);
'*' and '?' are still invalid for non-multi-kprobe...
> if (n < 1) {
> err = -EINVAL;
> pr_warn("kprobe name is invalid: %s\n", func_name);
> --
> 2.35.1
>
On Fri, Mar 04, 2022 at 03:11:19PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > Cc: Yucong Sun <[email protected]>
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > 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
On Mon, Mar 07, 2022 at 05:28:54PM -0800, Andrii Nakryiko wrote:
> On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Cc: Yucong Sun <[email protected]>
> > > > Signed-off-by: Jiri Olsa <[email protected]>
> > > > ---
> > > > 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
On Sun, Mar 6, 2022 at 9:29 AM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Cc: Yucong Sun <[email protected]>
> > > Signed-off-by: Jiri Olsa <[email protected]>
> > > ---
> > > 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
[...]