| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 4 Mar 2022 15:11:19 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <jolsa@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Masami Hiramatsu <mhiramat@...hat.com>,
Yucong Sun <fallentree@...com>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
lkml <linux-kernel@...r.kernel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH 08/10] libbpf: Add bpf_program__attach_kprobe_opts support
for multi kprobes
On Tue, Feb 22, 2022 at 9:07 AM Jiri Olsa <jolsa@...nel.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@...hat.com>
> Cc: Yucong Sun <fallentree@...com>
> Signed-off-by: Jiri Olsa <jolsa@...nel.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
> + 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
>
Powered by blists - more mailing lists