From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Rafael David Tinoco <rafaeldtinoco@ubuntu.com>
Cc: bpf <bpf@vger.kernel.org>
Subject: Re: [RFC][PATCH] libbpf: support kprobe/kretprobe events in legacy environments
Date: Thu, 18 Mar 2021 21:51:25 -0700 [thread overview]
Message-ID: <CAEf4Bzap6qS9_HQZTHJsM-X2VZso+N5xMwa3HNG9ycMW4WXtQg@mail.gmail.com> (raw)
In-Reply-To: <20210318062520.3838605-1-rafaeldtinoco@ubuntu.com>
On Wed, Mar 17, 2021 at 11:25 PM Rafael David Tinoco
<rafaeldtinoco@ubuntu.com> wrote:
>
> * Request for comments version (still needs polishing).
> * Based on Andrii Nakryiko's suggestion.
> * Using bpf_program__set_priv in attach_kprobe() for kprobe cleanup.
no-no-no, set_priv() is going to be deprecated and removed (see [0]),
and is not the right mechanism here. Detachment should happen in
bpf_link__destroy().
[0] https://docs.google.com/document/d/1UyjTZuPFWiPFyKk1tV5an11_iaRuec6U-ZESZ54nNTY
>
> Signed-off-by: Rafael David Tinoco <rafaeldtinoco@ubuntu.com>
> ---
> src/libbpf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 90 insertions(+), 10 deletions(-)
>
> diff --git a/src/libbpf.c b/src/libbpf.c
> index 2f351d3..4dc09d3 100644
> --- a/src/libbpf.c
> +++ b/src/libbpf.c
> @@ -9677,8 +9677,14 @@ static int parse_uint_from_file(const char *file, const char *fmt)
>
> static int determine_kprobe_perf_type(void)
> {
> + int ret = 0;
> + struct stat s;
> const char *file = "/sys/bus/event_source/devices/kprobe/type";
>
> + ret = stat(file, &s);
> + if (ret < 0)
> + return -errno;
> +
> return parse_uint_from_file(file, "%d\n");
> }
>
> @@ -9703,25 +9709,87 @@ static int determine_uprobe_retprobe_bit(void)
> return parse_uint_from_file(file, "config:%d\n");
> }
>
> +static int determine_kprobe_perf_type_legacy(const char *func_name)
> +{
> + char file[256];
nit: I suspect 256 is much longer than necessary :)
> + const char *fname = "/sys/kernel/debug/tracing/events/kprobes/%s/id";
> +
> + snprintf(file, sizeof(file), fname, func_name);
> +
> + return parse_uint_from_file(file, "%d\n");
> +}
> +
> +static int poke_kprobe_events(bool add, const char *name, bool kretprobe)
it's probably a good idea to put a link to [0] somewhere here
[0] https://www.kernel.org/doc/html/latest/trace/kprobetrace.html
> +{
> + int fd, ret = 0;
> + char given[256], buf[256];
nit: given -> event_name, to follow official documentation terminology ?
> + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> +
> + if (kretprobe && add)
what if it's kretprobe removal? shouldn't you generate the same name
> + snprintf(given, sizeof(given), "kprobes/%s_ret", name);
> + else
> + snprintf(given, sizeof(given), "kprobes/%s", name);
BCC includes PID in the name of the probe and "bcc", maybe we should
do something similar?
> + if (add)
> + snprintf(buf, sizeof(buf),"%c:%s %s\n", kretprobe ? 'r' : 'p', given, name);
> + else
> + snprintf(buf, sizeof(buf), "-:%s\n", given);
> +
> + fd = open(file, O_WRONLY|O_APPEND, 0);
> + if (!fd)
> + return -errno;
> + ret = write(fd, buf, strlen(buf));
> + if (ret < 0) {
> + ret = -errno;
> + }
> + close(fd);
> +
> + return ret;
> +}
> +
> +static inline int add_kprobe_event_legacy(const char* func_name, bool kretprobe)
> +{
> + return poke_kprobe_events(true /*add*/, func_name, kretprobe);
> +}
> +
> +static inline int remove_kprobe_event_legacy(const char* func_name, bool kretprobe)
> +{
> + return poke_kprobe_events(false /*remove*/, func_name, kretprobe);
> +}
> +
> static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> uint64_t offset, int pid)
> {
> struct perf_event_attr attr = {};
> char errmsg[STRERR_BUFSIZE];
> int type, pfd, err;
> + bool legacy = false;
>
> type = uprobe ? determine_uprobe_perf_type()
> : determine_kprobe_perf_type();
> if (type < 0) {
I think we should do "feature probing" to decide whether we should go
with legacy or modern kprobes. And just stick to that, reporting any
errors. I'm not a big fan of this generic "let's try X, if it fails
for *whatever* reason, let's try Y", because you 1) can ignore some
serious problem 2) you'll be getting unnecessary warnings in your log
> - pr_warn("failed to determine %s perf type: %s\n",
> - uprobe ? "uprobe" : "kprobe",
> - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> - return type;
> + if (uprobe) {
> + pr_warn("failed to determine %s perf type: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> + return type;
> + }
> + err = add_kprobe_event_legacy(name, retprobe);
> + if (err < 0) {
> + pr_warn("failed to add legacy kprobe events: %s\n",
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return err;
> + }
> + type = uprobe ? type : determine_kprobe_perf_type_legacy(name);
> + if (type < 0) {
> + remove_kprobe_event_legacy(name, retprobe);
> + pr_warn("failed to determine kprobe perf type: %s\n",
> + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> + }
> + legacy = true;
> }
> - if (retprobe) {
> + if (retprobe && !legacy) {
> int bit = uprobe ? determine_uprobe_retprobe_bit()
> : determine_kprobe_retprobe_bit();
> -
> if (bit < 0) {
> pr_warn("failed to determine %s retprobe bit: %s\n",
> uprobe ? "uprobe" : "kprobe",
> @@ -9731,10 +9799,14 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> attr.config |= 1 << bit;
> }
> attr.size = sizeof(attr);
> - attr.type = type;
> - attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> - attr.config2 = offset; /* kprobe_addr or probe_offset */
> -
> + if (!legacy) {
> + attr.type = type;
> + attr.config1 = ptr_to_u64(name); /* kprobe_func or uprobe_path */
> + attr.config2 = offset; /* kprobe_addr or probe_offset */
> + } else {
> + attr.config = type;
> + attr.type = PERF_TYPE_TRACEPOINT;
> + }
> /* pid filter is meaningful only for uprobes */
> pfd = syscall(__NR_perf_event_open, &attr,
> pid < 0 ? -1 : pid /* pid */,
> @@ -9750,6 +9822,11 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> return pfd;
> }
>
> +void bpf_program__detach_kprobe_legacy(struct bpf_program *prog, void *retprobe)
> +{
> + remove_kprobe_event_legacy(prog->name, (bool) retprobe);
> +}
as I mentioned, this should be done by bpf_link__destroy()
> +
> struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> bool retprobe,
> const char *func_name)
> @@ -9766,6 +9843,9 @@ struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> return ERR_PTR(pfd);
> }
> +
> + bpf_program__set_priv(prog, (void *) retprobe, bpf_program__detach_kprobe_legacy);
> +
> link = bpf_program__attach_perf_event(prog, pfd);
> if (IS_ERR(link)) {
> close(pfd);
> --
> 2.27.0
>
next prev parent reply other threads:[~2021-03-19 4:52 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-18 6:25 [RFC][PATCH] libbpf: support kprobe/kretprobe events in legacy environments Rafael David Tinoco
2021-03-18 19:31 ` [PATCH] libbpf: allow bpf object kern_version to be overridden Rafael David Tinoco
2021-03-18 19:46 ` Andrii Nakryiko
2021-03-18 20:56 ` Daniel Borkmann
2021-03-19 4:38 ` Rafael David Tinoco
2021-03-19 4:51 ` Andrii Nakryiko [this message]
2021-03-22 18:04 ` [PATCH v2 bpf-next][RFC] libbpf: introduce legacy kprobe events support Rafael David Tinoco
2021-03-22 18:25 ` Rafael David Tinoco
2021-03-26 20:50 ` Andrii Nakryiko
2021-04-07 4:49 ` Rafael David Tinoco
2021-04-07 22:33 ` Andrii Nakryiko
2021-04-08 23:59 ` John Fastabend
2021-04-14 14:30 ` Rafael David Tinoco
2021-04-14 20:06 ` Rafael David Tinoco
2021-04-14 23:23 ` Andrii Nakryiko
2021-04-15 5:53 ` Rafael David Tinoco
2021-04-15 22:48 ` Andrii Nakryiko
2021-06-25 4:44 ` [PATCH bpf-next v3] " Rafael David Tinoco
2021-06-25 5:01 ` Rafael David Tinoco
2021-07-07 13:38 ` Rafael David Tinoco
2021-07-07 21:52 ` Andrii Nakryiko
2021-07-19 1:59 ` Rafael David Tinoco
2021-07-20 0:10 ` Andrii Nakryiko
2021-07-20 4:16 ` Rafael David Tinoco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4Bzap6qS9_HQZTHJsM-X2VZso+N5xMwa3HNG9ycMW4WXtQg@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=rafaeldtinoco@ubuntu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).