| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 May 2020 18:39:57 -0700
From: Yonghong Song <yhs@...com>
To: "Daniel T. Lee" <danieltimlee@...il.com>,
Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>
CC: <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
Andrii Nakryiko <andrii.nakryiko@...il.com>,
John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH bpf-next 1/3] samples: bpf: refactor kprobe tracing user
progs with libbpf
On 5/12/20 7:43 AM, Daniel T. Lee wrote:
> Currently, the kprobe BPF program attachment method for bpf_load is
> quite old. The implementation of bpf_load "directly" controls and
> manages(create, delete) the kprobe events of DEBUGFS. On the other hand,
> using using the libbpf automatically manages the kprobe event.
> (under bpf_link interface)
>
> By calling bpf_program__attach(_kprobe) in libbpf, the corresponding
> kprobe is created and the BPF program will be attached to this kprobe.
> To remove this, by simply invoking bpf_link__destroy will clean up the
> event.
>
> This commit refactors kprobe tracing programs (tracex{1~7}_user.c) with
> libbpf using bpf_link interface and bpf_program__attach.
>
> tracex2_kern.c, which tracks system calls (sys_*), has been modified to
> append prefix depending on architecture.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> ---
> samples/bpf/Makefile | 12 +++----
> samples/bpf/tracex1_user.c | 41 ++++++++++++++++++++----
> samples/bpf/tracex2_kern.c | 8 ++++-
> samples/bpf/tracex2_user.c | 55 ++++++++++++++++++++++++++------
> samples/bpf/tracex3_user.c | 65 ++++++++++++++++++++++++++++----------
> samples/bpf/tracex4_user.c | 55 +++++++++++++++++++++++++-------
> samples/bpf/tracex6_user.c | 53 +++++++++++++++++++++++++++----
> samples/bpf/tracex7_user.c | 43 ++++++++++++++++++++-----
> 8 files changed, 268 insertions(+), 64 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 424f6fe7ce38..4c91e5914329 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -64,13 +64,13 @@ fds_example-objs := fds_example.o
> sockex1-objs := sockex1_user.o
> sockex2-objs := sockex2_user.o
> sockex3-objs := bpf_load.o sockex3_user.o
> -tracex1-objs := bpf_load.o tracex1_user.o $(TRACE_HELPERS)
> -tracex2-objs := bpf_load.o tracex2_user.o
> -tracex3-objs := bpf_load.o tracex3_user.o
> -tracex4-objs := bpf_load.o tracex4_user.o
> +tracex1-objs := tracex1_user.o $(TRACE_HELPERS)
> +tracex2-objs := tracex2_user.o
> +tracex3-objs := tracex3_user.o
> +tracex4-objs := tracex4_user.o
> tracex5-objs := bpf_load.o tracex5_user.o $(TRACE_HELPERS)
> -tracex6-objs := bpf_load.o tracex6_user.o
> -tracex7-objs := bpf_load.o tracex7_user.o
> +tracex6-objs := tracex6_user.o
> +tracex7-objs := tracex7_user.o
> test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o
> trace_output-objs := bpf_load.o trace_output_user.o $(TRACE_HELPERS)
> lathist-objs := bpf_load.o lathist_user.o
> diff --git a/samples/bpf/tracex1_user.c b/samples/bpf/tracex1_user.c
> index 55fddbd08702..1b15ab98f7d3 100644
> --- a/samples/bpf/tracex1_user.c
> +++ b/samples/bpf/tracex1_user.c
> @@ -1,21 +1,45 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <stdio.h>
> -#include <linux/bpf.h>
> #include <unistd.h>
> -#include <bpf/bpf.h>
> -#include "bpf_load.h"
> +#include <bpf/libbpf.h>
> #include "trace_helpers.h"
>
> +#define __must_check
This is not very user friendly.
Maybe not including linux/err.h and
use libbpf API libbpf_get_error() instead?
> +#include <linux/err.h>
> +
> int main(int ac, char **argv)
> {
> - FILE *f;
> + struct bpf_link *link = NULL;
> + struct bpf_program *prog;
> + struct bpf_object *obj;
> char filename[256];
> + FILE *f;
>
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> + obj = bpf_object__open_file(filename, NULL);
> + if (IS_ERR(obj)) {
> + fprintf(stderr, "ERROR: opening BPF object file failed\n");
> + obj = NULL;
> + goto cleanup;
You do not need to goto cleanup, directly return 0 is okay here.
The same for other files in this patch.
> + }
> +
> + prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
> + if (!prog) {
> + fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
> + goto cleanup;
> + }
> +
> + /* load BPF program */
> + if (bpf_object__load(obj)) {
> + fprintf(stderr, "ERROR: loading BPF object file failed\n");
> + goto cleanup;
> + }
>
> - if (load_bpf_file(filename)) {
> - printf("%s", bpf_log_buf);
> - return 1;
> + link = bpf_program__attach(prog);
> + if (IS_ERR(link)) {
> + fprintf(stderr, "ERROR: bpf_program__attach failed\n");
> + link = NULL;
> + goto cleanup;
> }
>
> f = popen("taskset 1 ping -c5 localhost", "r");
> @@ -23,5 +47,8 @@ int main(int ac, char **argv)
>
> read_trace_pipe();
>
> +cleanup:
> + bpf_link__destroy(link);
> + bpf_object__close(obj);
Typically in kernel, we do multiple labels for such cases
like
destroy_link:
bpf_link__destroy(link);
close_object:
bpf_object__close(obj);
The error path in the main() function jumps to proper label.
This is more clean and less confusion.
The same for other cases in this file.
> return 0;
> }
> diff --git a/samples/bpf/tracex2_kern.c b/samples/bpf/tracex2_kern.c
> index d865bb309bcb..ff5d00916733 100644
> --- a/samples/bpf/tracex2_kern.c
> +++ b/samples/bpf/tracex2_kern.c
> @@ -11,6 +11,12 @@
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
>
> +#ifdef __x86_64__
> +#define SYSCALL "__x64_"
> +#else
> +#define SYSCALL
> +#endif
See test_progs.h, one more case to handle:
#ifdef __x86_64__
#define SYS_NANOSLEEP_KPROBE_NAME "__x64_sys_nanosleep"
#elif defined(__s390x__)
#define SYS_NANOSLEEP_KPROBE_NAME "__s390x_sys_nanosleep"
#else
#define SYS_NANOSLEEP_KPROBE_NAME "sys_nanosleep"
#endif
> +
> struct bpf_map_def SEC("maps") my_map = {
> .type = BPF_MAP_TYPE_HASH,
> .key_size = sizeof(long),
> @@ -77,7 +83,7 @@ struct bpf_map_def SEC("maps") my_hist_map = {
> .max_entries = 1024,
> };
>
> -SEC("kprobe/sys_write")
> +SEC("kprobe/" SYSCALL "sys_write")
> int bpf_prog3(struct pt_regs *ctx)
> {
> long write_size = PT_REGS_PARM3(ctx);
[...]
Powered by blists - more mailing lists