Re: [PATCH bpf-next v4] libbpf: introduce legacy kprobe events support
From: Andrii Nakryiko <hidden>
Date: 2021-08-06 22:29:57
On Sat, Jul 31, 2021 at 10:11 PM Rafael David Tinoco [off-list ref] wrote:
Allow kprobe tracepoint events creation through legacy interface, as the kprobe dynamic PMUs support, used by default, was only created in v4.17. After commit "bpf: implement minimal BPF perf link", it was allowed that some extra - to the link - information is accessed through container_of struct bpf_link. This allows the tracing perf event legacy name, and information whether it is a retprobe, to be saved outside bpf_link structure, which would not be optimal. This enables CO.RE support for older kernels.
nit: it's CO-RE (or Compile Once - Run Everywhere), let's keep consistent spelling.
Cc: Andrii Nakryiko <redacted> Signed-off-by: Rafael David Tinoco <redacted> ---
Looks good overall, modulo various nits and skipped error handling. Please wait for bpf_perf_link changes to get applied and submit the next revision based on it. Thanks!
quoted hunk ↗ jump to hunk
tools/lib/bpf/libbpf.c | 127 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 2 deletions(-)diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index e1b7b2b6618c..40037340a3e7 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c@@ -8985,9 +8985,55 @@ int bpf_link__unpin(struct bpf_link *link) return 0; } +static int poke_kprobe_events(bool add, const char *name, bool retprobe, uint64_t offset) {
'{' should be on separate line
Please use scripts/checkpatch.pl on your changes, there are a bunch of
stylistic problems in your code, which you would have caught yourself
if you ran this script.
+ int fd, ret = 0;
+ pid_t p = getpid();
+ char cmd[192] = {}, probename[128] = {}, probefunc[128] = {};
+ const char *file = "/sys/kernel/debug/tracing/kprobe_events";
+
+ if (retprobe)
+ snprintf(probename, sizeof(probename), "kretprobes/%s_libbpf_%u", name, p);
+ else
+ snprintf(probename, sizeof(probename), "kprobes/%s_libbpf_%u", name, p);
+
+ if (offset)
+ snprintf(probefunc, sizeof(probefunc), "%s+%lu", name, offset);
+
+ if (add) {
+ snprintf(cmd, sizeof(cmd), "%c:%s %s",
+ retprobe ? 'r' : 'p',
+ probename,
+ offset ? probefunc : name);
+ } else {
+ snprintf(cmd, sizeof(cmd), "-:%s", probename);
+ }
+
+ fd = open(file, O_WRONLY | O_APPEND, 0);
+ if (!fd)
+ return -errno;
+ ret = write(fd, cmd, strlen(cmd));
+ if (ret < 0)
+ ret = -errno;
+ close(fd);
+
+ return ret;
+}
+
+static inline int add_kprobe_event_legacy(const char *name, bool retprobe, uint64_t offset)
+{
+ return poke_kprobe_events(true, name, retprobe, offset);
+}
+
+static inline int remove_kprobe_event_legacy(const char *name, bool retprobe)
+{
+ return poke_kprobe_events(false, name, retprobe, 0);
+}
+
struct bpf_link_perf {
struct bpf_link link;
int perf_event_fd;
+ char *legacy_name;My initial reaction was to have a still different struct specifically for legacy kprobe (e.g., struct bpf_link_kprobe_legacy or something like that). But we'll need to do similar changes to uprobes as well, so now I think it's ok to have this as an optional extra field on perf_event-based link. If that causes problems we can always change that later. To make it clearer, can you name it "legacy_probe_name" and leave a short comment mentioning that this is used to remember original identifier we used to create legacy kprobe?
quoted hunk ↗ jump to hunk
+ bool is_retprobe; }; static int bpf_link_perf_detach(struct bpf_link *link)@@ -9002,6 +9048,10 @@ static int bpf_link_perf_detach(struct bpf_link *link) close(perf_link->perf_event_fd); close(link->fd); + /* legacy kprobe needs to be removed after perf event fd closure */ + if (perf_link->legacy_name) + remove_kprobe_event_legacy(perf_link->legacy_name, perf_link->is_retprobe);
check and propagate error?
quoted hunk ↗ jump to hunk
+ return libbpf_err(err); }@@ -9009,6 +9059,9 @@ static void bpf_link_perf_dealloc(struct bpf_link *link) { struct bpf_link_perf *perf_link = container_of(link, struct bpf_link_perf, link); + if (perf_link->legacy_name) + free(perf_link->legacy_name);
free() handles NULL properly, no need for if
quoted hunk ↗ jump to hunk
+ free(perf_link); }@@ -9122,6 +9175,26 @@ static int parse_uint_from_file(const char *file, const char *fmt) return ret; } +static bool determine_kprobe_legacy(void) +{ + const char *file = "/sys/bus/event_source/devices/kprobe/type"; + + return access(file, 0) == 0 ? false : true; +} + +static int determine_kprobe_perf_type_legacy(const char *func_name, bool is_retprobe) +{ + char file[192]; +
extra empty line
quoted hunk ↗ jump to hunk
+ const char *fname = "/sys/kernel/debug/tracing/events/%s/%s_libbpf_%d/id"; + + snprintf(file, sizeof(file), fname, + is_retprobe ? "kretprobes" : "kprobes", + func_name, getpid()); + + return parse_uint_from_file(file, "%d\n"); +} + static int determine_kprobe_perf_type(void) { const char *file = "/sys/bus/event_source/devices/kprobe/type";@@ -9197,6 +9270,41 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name, return pfd; } +static int perf_event_kprobe_open_legacy(bool retprobe, const char *name, uint64_t offset, int pid) +{ + struct perf_event_attr attr = {}; + char errmsg[STRERR_BUFSIZE]; + int type, pfd, err; + + err = add_kprobe_event_legacy(name, retprobe, offset); + if (err < 0) { + pr_warn("failed to add legacy kprobe event: %s\n", + libbpf_strerror_r(err, errmsg, sizeof(errmsg))); + return err; + } + type = determine_kprobe_perf_type_legacy(name, retprobe); + if (type < 0) { + pr_warn("failed to determine legacy kprobe event id: %s\n", + libbpf_strerror_r(type, errmsg, sizeof(errmsg))); + return type; + } + attr.size = sizeof(attr); + attr.config = type; + attr.type = PERF_TYPE_TRACEPOINT; + + pfd = syscall(__NR_perf_event_open, &attr, + pid < 0 ? -1 : pid, /* pid */ + pid == -1 ? 0 : -1, /* cpu */ + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC); + if (pfd < 0) { + err = -errno; + pr_warn("legacy kprobe perf_event_open() failed: %s\n", + libbpf_strerror_r(err, errmsg, sizeof(errmsg))); + return err; + } + return pfd; +} + struct bpf_link * bpf_program__attach_kprobe_opts(struct bpf_program *prog, const char *func_name,@@ -9208,6 +9316,7 @@ bpf_program__attach_kprobe_opts(struct bpf_program *prog, unsigned long offset; bool retprobe; int pfd, err; + bool legacy; if (!OPTS_VALID(opts, bpf_kprobe_opts)) return libbpf_err_ptr(-EINVAL);@@ -9216,8 +9325,16 @@ bpf_program__attach_kprobe_opts(struct bpf_program *prog, offset = OPTS_GET(opts, offset, 0); pe_opts.user_ctx = OPTS_GET(opts, user_ctx, 0); - pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name, - offset, -1 /* pid */); + legacy = determine_kprobe_legacy(); + if (!legacy) { + pfd = perf_event_open_probe(false /* uprobe */, + retprobe, func_name, + offset, -1 /* pid */); + } else { + pfd = perf_event_kprobe_open_legacy(retprobe, func_name, + 0 /* offset */, + -1 /* pid */); + } if (pfd < 0) { pr_warn("prog '%s': failed to create %s '%s' perf event: %s\n", prog->name, retprobe ? "kretprobe" : "kprobe", func_name,@@ -9233,6 +9350,12 @@ bpf_program__attach_kprobe_opts(struct bpf_program *prog, libbpf_strerror_r(err, errmsg, sizeof(errmsg))); return libbpf_err_ptr(err); } + if (legacy) { + struct bpf_link_perf *perf_link = container_of(link, struct bpf_link_perf, link);
empty line between variable declaration and statements
+ perf_link->legacy_name = strdup(func_name);
check NULL, could be ENOMEM. Probably easier to allocate this at the beginning, before we do heavy-lifting of perf_event_open(), clean up will be simpler.
+ perf_link->is_retprobe = retprobe;
+ }
+
return link;
}
--
2.30.2