On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
>
> Following ./Documentation/trace/kprobetrace.rst add support for loading
> kprobes programs on older kernels.
>
My main concern with this is that this code is born bit-rotten,
because selftests are never testing the legacy code path. How did you
think about testing this and ensuring that this keeps working going
forward?
> Signed-off-by: John Fastabend <john.fastabend@xxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 81 +++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index fcea6988f962..12b3105d112c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5005,20 +5005,89 @@ static int determine_uprobe_retprobe_bit(void)
> return parse_uint_from_file(file, "config:%d\n");
> }
>
> +static int use_kprobe_debugfs(const char *name,
> + uint64_t offset, int pid, bool retprobe)
> +{
> + const char *file = "/sys/kernel/debug/tracing/kprobe_events";
> + int fd = open(file, O_WRONLY | O_APPEND, 0);
> + char buf[PATH_MAX];
> + int err;
> +
> + if (fd < 0) {
> + pr_warning("failed open kprobe_events: %s\n",
> + strerror(errno));
> + return -errno;
errno after pr_warning() call might be clobbered, you need to save it
locally first
> + }
> +
> + snprintf(buf, sizeof(buf), "%c:kprobes/%s %s",
> + retprobe ? 'r' : 'p', name, name);
remember result, check it to detect overflow, and use it in write below?
> +
> + err = write(fd, buf, strlen(buf));
> + close(fd);
> + if (err < 0)
> + return -errno;
> + return 0;
> +}
> +
> 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];
> + uint64_t config1 = 0;
> int type, pfd, err;
>
> type = uprobe ? determine_uprobe_perf_type()
> : determine_kprobe_perf_type();
> if (type < 0) {
> - pr_warning("failed to determine %s perf type: %s\n",
> - uprobe ? "uprobe" : "kprobe",
> - libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> - return type;
> + if (uprobe) {
> + pr_warning("failed to determine uprobe perf type %s: %s\n",
> + name,
> + libbpf_strerror_r(type,
> + errmsg, sizeof(errmsg)));
> + } else {
I think this legacy kprobe setup deserves its own function (maybe even
combine it with use_kprobe_debugfs to do entire attribute setup from A
to Z?)
These 2 levels of nestedness is also unfortunate, how about having two
functions that are filling out perf_event_attr? Something like:
err = determine_perf_probe_attr(...)
if (err)
err = determine_legacy_probe_attr(...)
if (!err)
<bail out>
do perf call here
> + /* If we do not have an event_source/../kprobes then we
> + * can try to use kprobe-base event tracing, for details
> + * see ./Documentation/trace/kprobetrace.rst
> + */
> + const char *file = "/sys/kernel/debug/tracing/events/kprobes/";
> + char c[PATH_MAX];
what does c stand for?
> + int fd, n;
> +
> + snprintf(c, sizeof(c), "%s/%s/id", file, name);
check result? also, is there a reason to not use
"/sys/kernel/debug/tracing/events/kprobes/" directly in format string?
> +
> + err = use_kprobe_debugfs(name, offset, pid, retprobe);
> + if (err)
> + return err;
> +
> + type = PERF_TYPE_TRACEPOINT;
> + fd = open(c, O_RDONLY, 0);
> + if (fd < 0) {
> + pr_warning("failed to open tracepoint %s: %s\n",
> + c, strerror(errno));
> + return -errno;
> + }
> + n = read(fd, c, sizeof(c));
> + close(fd);
> + if (n < 0) {
> + pr_warning("failed to read %s: %s\n",
> + c, strerror(errno));
It's a bit fishy that you are reading into c and then print out c on
error. Its contents might be corrupted at this point.
And same thing about errno as above.
> + return -errno;
> + }
> + c[n] = '\0';
> + config1 = strtol(c, NULL, 0);
no need for config1 variable, just attr.config = strtol(...)?
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config = config1;
> + attr.sample_period = 1;
> + attr.wakeup_events = 1;
> + }
> + } else {
> + config1 = ptr_to_u64(name);
same, just straight attr.config1 = ... ?
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config1 = config1; /* kprobe_func or uprobe_path */
> + attr.config2 = offset; /* kprobe_addr or probe_offset */
> }
> if (retprobe) {
> int bit = uprobe ? determine_uprobe_retprobe_bit()
> @@ -5033,10 +5102,6 @@ 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 */
>
> /* pid filter is meaningful only for uprobes */
> pfd = syscall(__NR_perf_event_open, &attr,
>
What about the detaching? Would closing perf event FD be enough?
Wouldn't we need to clear a probe with -:<event>?
