Re: [RFC][PATCH] libbpf: support kprobe/kretprobe events in legacy environments

[Andrii Nakryiko <andrii.nakryiko@gmail.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
>