On Wed, Mar 17, 2021 at 11:25 PM Rafael David Tinoco <rafaeldtinoco@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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 >
