[PATCH bpf-next 0/3] samples: bpf: refactor kprobe tracing progs with libbpf

[PATCH bpf-next 0/3] samples: bpf: refactor kprobe tracing progs with libbpf

[Daniel T. Lee]

Currently, the kprobe BPF program attachment method for bpf_load is
pretty outdated. 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)

This patchset refactors kprobe tracing programs with using libbpf API
for loading bpf program instead of previous bpf_load implementation.

Daniel T. Lee (3):
  samples: bpf: refactor kprobe tracing user progs with libbpf
  samples: bpf: refactor tail call user progs with libbpf
  samples: bpf: refactor kprobe, tail call kern progs map definition

 samples/bpf/Makefile           | 16 ++++----
 samples/bpf/sampleip_kern.c    | 12 +++---
 samples/bpf/sockex3_kern.c     | 36 ++++++++---------
 samples/bpf/sockex3_user.c     | 66 ++++++++++++++++++++++----------
 samples/bpf/trace_event_kern.c | 24 ++++++------
 samples/bpf/tracex1_user.c     | 41 ++++++++++++++++----
 samples/bpf/tracex2_kern.c     | 32 +++++++++-------
 samples/bpf/tracex2_user.c     | 55 +++++++++++++++++++++-----
 samples/bpf/tracex3_kern.c     | 24 ++++++------
 samples/bpf/tracex3_user.c     | 65 +++++++++++++++++++++++--------
 samples/bpf/tracex4_kern.c     | 12 +++---
 samples/bpf/tracex4_user.c     | 55 ++++++++++++++++++++------
 samples/bpf/tracex5_kern.c     | 14 +++----
 samples/bpf/tracex5_user.c     | 70 ++++++++++++++++++++++++++++++----
 samples/bpf/tracex6_kern.c     | 38 +++++++++---------
 samples/bpf/tracex6_user.c     | 53 ++++++++++++++++++++++---
 samples/bpf/tracex7_user.c     | 43 +++++++++++++++++----
 17 files changed, 471 insertions(+), 185 deletions(-)

-- 
2.25.1

[PATCH bpf-next 1/3] samples: bpf: refactor kprobe tracing user progs with libbpf

[Daniel T. Lee]

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@gmail.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
+#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;
+	}
+
+	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);
 	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
+
 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);
diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
index c9544a4ce61a..71bdf2a9543f 100644
--- a/samples/bpf/tracex2_user.c
+++ b/samples/bpf/tracex2_user.c
@@ -3,17 +3,22 @@
 #include <unistd.h>
 #include <stdlib.h>
 #include <signal.h>
-#include <linux/bpf.h>
 #include <string.h>
 #include <sys/resource.h>
 
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
 #include "bpf_util.h"
 
+#define __must_check
+#include <linux/err.h>
+
 #define MAX_INDEX	64
 #define MAX_STARS	38
 
+/* my_map, my_hist_map */
+static int map_fd[2];
+
 static void stars(char *str, long val, long max, int width)
 {
 	int i;
@@ -115,18 +120,40 @@ static void int_exit(int sig)
 int main(int ac, char **argv)
 {
 	struct rlimit r = {1024*1024, RLIM_INFINITY};
-	char filename[256];
 	long key, next_key, value;
+	struct bpf_link *links[2];
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	char filename[256];
+	int i, j = 0;
 	FILE *f;
-	int i;
-
-	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 
 	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
 		perror("setrlimit(RLIMIT_MEMLOCK)");
 		return 1;
 	}
 
+	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;
+	}
+
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	map_fd[0] = bpf_object__find_map_fd_by_name(obj, "my_map");
+	map_fd[1] = bpf_object__find_map_fd_by_name(obj, "my_hist_map");
+	if (map_fd[0] < 0 || map_fd[1] < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
 	signal(SIGINT, int_exit);
 	signal(SIGTERM, int_exit);
 
@@ -138,9 +165,14 @@ int main(int ac, char **argv)
 	f = popen("dd if=/dev/zero of=/dev/null count=5000000", "r");
 	(void) f;
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	bpf_object__for_each_program(prog, obj) {
+		links[j] = bpf_program__attach(prog);
+		if (IS_ERR(links[j])) {
+			fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+			links[j] = NULL;
+			goto cleanup;
+		}
+		j++;
 	}
 
 	for (i = 0; i < 5; i++) {
@@ -156,5 +188,10 @@ int main(int ac, char **argv)
 	}
 	print_hist(map_fd[1]);
 
+cleanup:
+	for (j--; j >= 0; j--)
+		bpf_link__destroy(links[j]);
+
+	bpf_object__close(obj);
 	return 0;
 }
diff --git a/samples/bpf/tracex3_user.c b/samples/bpf/tracex3_user.c
index cf8fedc773f2..3045e118199a 100644
--- a/samples/bpf/tracex3_user.c
+++ b/samples/bpf/tracex3_user.c
@@ -7,13 +7,15 @@
 #include <unistd.h>
 #include <stdbool.h>
 #include <string.h>
-#include <linux/bpf.h>
 #include <sys/resource.h>
 
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
 #include "bpf_util.h"
 
+#define __must_check
+#include <linux/err.h>
+
 #define SLOTS 100
 
 static void clear_stats(int fd)
@@ -109,20 +111,11 @@ static void print_hist(int fd)
 int main(int ac, char **argv)
 {
 	struct rlimit r = {1024*1024, RLIM_INFINITY};
+	struct bpf_link *links[2];
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
-	int i;
-
-	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-
-	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
-		perror("setrlimit(RLIMIT_MEMLOCK)");
-		return 1;
-	}
-
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
-	}
+	int map_fd, i, j = 0;
 
 	for (i = 1; i < ac; i++) {
 		if (strcmp(argv[i], "-a") == 0) {
@@ -137,6 +130,41 @@ int main(int ac, char **argv)
 		}
 	}
 
+	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
+		perror("setrlimit(RLIMIT_MEMLOCK)");
+		return 1;
+	}
+
+	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;
+	}
+
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	map_fd = bpf_object__find_map_fd_by_name(obj, "lat_map");
+	if (map_fd < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		links[j] = bpf_program__attach(prog);
+		if (IS_ERR(links[j])) {
+			fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+			links[j] = NULL;
+			goto cleanup;
+		}
+		j++;
+	}
+
 	printf("  heatmap of IO latency\n");
 	if (text_only)
 		printf("  %s", sym[num_colors - 1]);
@@ -153,9 +181,14 @@ int main(int ac, char **argv)
 	for (i = 0; ; i++) {
 		if (i % 20 == 0)
 			print_banner();
-		print_hist(map_fd[1]);
+		print_hist(map_fd);
 		sleep(2);
 	}
 
+cleanup:
+	for (j--; j >= 0; j--)
+		bpf_link__destroy(links[j]);
+
+	bpf_object__close(obj);
 	return 0;
 }
diff --git a/samples/bpf/tracex4_user.c b/samples/bpf/tracex4_user.c
index ec52203fce39..a131a48bc15a 100644
--- a/samples/bpf/tracex4_user.c
+++ b/samples/bpf/tracex4_user.c
@@ -8,11 +8,13 @@
 #include <stdbool.h>
 #include <string.h>
 #include <time.h>
-#include <linux/bpf.h>
 #include <sys/resource.h>
 
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
+
+#define __must_check
+#include <linux/err.h>
 
 struct pair {
 	long long val;
@@ -36,8 +38,8 @@ static void print_old_objects(int fd)
 	key = write(1, "\e[1;1H\e[2J", 12); /* clear screen */
 
 	key = -1;
-	while (bpf_map_get_next_key(map_fd[0], &key, &next_key) == 0) {
-		bpf_map_lookup_elem(map_fd[0], &next_key, &v);
+	while (bpf_map_get_next_key(fd, &key, &next_key) == 0) {
+		bpf_map_lookup_elem(fd, &next_key, &v);
 		key = next_key;
 		if (val - v.val < 1000000000ll)
 			/* object was allocated more then 1 sec ago */
@@ -50,25 +52,56 @@ static void print_old_objects(int fd)
 int main(int ac, char **argv)
 {
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_link *links[2];
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
-	int i;
-
-	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	int map_fd, i, j = 0;
 
 	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
 		perror("setrlimit(RLIMIT_MEMLOCK, RLIM_INFINITY)");
 		return 1;
 	}
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	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;
+	}
+
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	map_fd = bpf_object__find_map_fd_by_name(obj, "my_map");
+	if (map_fd < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		links[j] = bpf_program__attach(prog);
+		if (IS_ERR(links[j])) {
+			fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+			links[j] = NULL;
+			goto cleanup;
+		}
+		j++;
 	}
 
 	for (i = 0; ; i++) {
-		print_old_objects(map_fd[1]);
+		print_old_objects(map_fd);
 		sleep(1);
 	}
 
+cleanup:
+	for (j--; j >= 0; j--)
+		bpf_link__destroy(links[j]);
+
+	bpf_object__close(obj);
 	return 0;
 }
diff --git a/samples/bpf/tracex6_user.c b/samples/bpf/tracex6_user.c
index 4bb3c830adb2..e363dcb1c2dd 100644
--- a/samples/bpf/tracex6_user.c
+++ b/samples/bpf/tracex6_user.c
@@ -4,7 +4,6 @@
 #include <assert.h>
 #include <fcntl.h>
 #include <linux/perf_event.h>
-#include <linux/bpf.h>
 #include <sched.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -15,12 +14,18 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
-#include "bpf_load.h"
 #include <bpf/bpf.h>
+#include <bpf/libbpf.h>
 #include "perf-sys.h"
 
+#define __must_check
+#include <linux/err.h>
+
 #define SAMPLE_PERIOD  0x7fffffffffffffffULL
 
+/* counters, values, values2 */
+static int map_fd[3];
+
 static void check_on_cpu(int cpu, struct perf_event_attr *attr)
 {
 	struct bpf_perf_event_value value2;
@@ -174,16 +179,52 @@ static void test_bpf_perf_event(void)
 int main(int argc, char **argv)
 {
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_link *links[2];
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
+	int i = 0;
+
+	setrlimit(RLIMIT_MEMLOCK, &r);
 
 	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;
+	}
 
-	setrlimit(RLIMIT_MEMLOCK, &r);
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	map_fd[0] = bpf_object__find_map_fd_by_name(obj, "counters");
+	map_fd[1] = bpf_object__find_map_fd_by_name(obj, "values");
+	map_fd[2] = bpf_object__find_map_fd_by_name(obj, "values2");
+	if (map_fd[0] < 0 || map_fd[1] < 0 || map_fd[2] < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		links[i] = bpf_program__attach(prog);
+		if (IS_ERR(links[i])) {
+			fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+			links[i] = NULL;
+			goto cleanup;
+		}
+		i++;
 	}
 
 	test_bpf_perf_event();
+
+cleanup:
+	for (i--; i >= 0; i--)
+		bpf_link__destroy(links[i]);
+
+	bpf_object__close(obj);
 	return 0;
 }
diff --git a/samples/bpf/tracex7_user.c b/samples/bpf/tracex7_user.c
index ea6dae78f0df..699755eb6850 100644
--- a/samples/bpf/tracex7_user.c
+++ b/samples/bpf/tracex7_user.c
@@ -1,28 +1,55 @@
 #define _GNU_SOURCE
 
 #include <stdio.h>
-#include <linux/bpf.h>
 #include <unistd.h>
-#include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
+
+#define __must_check
+#include <linux/err.h>
 
 int main(int argc, char **argv)
 {
-	FILE *f;
+	struct bpf_link *link = NULL;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
 	char command[256];
-	int ret;
+	int ret = 0;
+	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;
+	}
+
+	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;
 	}
 
 	snprintf(command, 256, "mount %s tmpmnt/", argv[1]);
 	f = popen(command, "r");
 	ret = pclose(f);
 
+cleanup:
+	bpf_link__destroy(link);
+	bpf_object__close(obj);
 	return ret ? 0 : 1;
 }
-- 
2.25.1

[PATCH bpf-next 2/3] samples: bpf: refactor tail call user progs with libbpf

[Daniel T. Lee]

BPF tail call uses the BPF_MAP_TYPE_PROG_ARRAY type map for calling
into other BPF programs and this PROG_ARRAY should be filled prior to
use. Currently, samples with the PROG_ARRAY type MAP fill this program
array with bpf_load. For bpf_load to fill this map, kernel BPF program
must specify the section with specific format of <prog_type>/<array_idx>
(e.g. SEC("socket/0"))

But by using libbpf instead of bpf_load, user program can specify which
programs should be added to PROG_ARRAY. The advantage of this approach
is that you can selectively add only the programs you want, rather than
adding all of them to PROG_ARRAY, and it's much more intuitive than the
traditional approach.

This commit refactors user programs with the PROG_ARRAY type MAP with
libbpf instead of using bpf_load.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/Makefile       |  4 +--
 samples/bpf/sockex3_user.c | 66 ++++++++++++++++++++++++-----------
 samples/bpf/tracex5_user.c | 70 +++++++++++++++++++++++++++++++++-----
 3 files changed, 110 insertions(+), 30 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 4c91e5914329..8403e4762306 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -63,12 +63,12 @@ TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
 fds_example-objs := fds_example.o
 sockex1-objs := sockex1_user.o
 sockex2-objs := sockex2_user.o
-sockex3-objs := bpf_load.o sockex3_user.o
+sockex3-objs := sockex3_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)
+tracex5-objs := tracex5_user.o $(TRACE_HELPERS)
 tracex6-objs := tracex6_user.o
 tracex7-objs := tracex7_user.o
 test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o
diff --git a/samples/bpf/sockex3_user.c b/samples/bpf/sockex3_user.c
index bbb1cd0666a9..961ae9c7deb9 100644
--- a/samples/bpf/sockex3_user.c
+++ b/samples/bpf/sockex3_user.c
@@ -1,17 +1,15 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <stdio.h>
 #include <assert.h>
-#include <linux/bpf.h>
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
 #include "sock_example.h"
 #include <unistd.h>
 #include <arpa/inet.h>
 #include <sys/resource.h>
 
-#define PARSE_IP 3
-#define PARSE_IP_PROG_FD (prog_fd[0])
-#define PROG_ARRAY_FD (map_fd[0])
+#define __must_check
+#include <linux/err.h>
 
 struct flow_key_record {
 	__be32 src;
@@ -30,31 +28,56 @@ struct pair {
 
 int main(int argc, char **argv)
 {
+	int i, sock, key, fd, main_prog_fd, jmp_table_fd, hash_map_fd;
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
+	const char *title;
 	FILE *f;
-	int i, sock, err, id, key = PARSE_IP;
-	struct bpf_prog_info info = {};
-	uint32_t info_len = sizeof(info);
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	setrlimit(RLIMIT_MEMLOCK, &r);
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	obj = bpf_object__open_file(filename, NULL);
+	if (IS_ERR(obj)) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		obj = NULL;
+		goto cleanup;
 	}
 
-	/* Test fd array lookup which returns the id of the bpf_prog */
-	err = bpf_obj_get_info_by_fd(PARSE_IP_PROG_FD, &info, &info_len);
-	assert(!err);
-	err = bpf_map_lookup_elem(PROG_ARRAY_FD, &key, &id);
-	assert(!err);
-	assert(id == info.id);
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	jmp_table_fd = bpf_object__find_map_fd_by_name(obj, "jmp_table");
+	hash_map_fd = bpf_object__find_map_fd_by_name(obj, "hash_map");
+	if (jmp_table_fd < 0 || hash_map_fd < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		fd = bpf_program__fd(prog);
+
+		title = bpf_program__title(prog, false);
+		if (sscanf(title, "socket/%d", &key) != 1) {
+			fprintf(stderr, "ERROR: finding prog failed\n");
+			goto cleanup;
+		}
+
+		if (key == 0)
+			main_prog_fd = fd;
+		else
+			bpf_map_update_elem(jmp_table_fd, &key, &fd, BPF_ANY);
+	}
 
 	sock = open_raw_sock("lo");
 
-	assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd[4],
+	/* attach BPF program to socket */
+	assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &main_prog_fd,
 			  sizeof(__u32)) == 0);
 
 	if (argc > 1)
@@ -69,8 +92,8 @@ int main(int argc, char **argv)
 
 		sleep(1);
 		printf("IP     src.port -> dst.port               bytes      packets\n");
-		while (bpf_map_get_next_key(map_fd[2], &key, &next_key) == 0) {
-			bpf_map_lookup_elem(map_fd[2], &next_key, &value);
+		while (bpf_map_get_next_key(hash_map_fd, &key, &next_key) == 0) {
+			bpf_map_lookup_elem(hash_map_fd, &next_key, &value);
 			printf("%s.%05d -> %s.%05d %12lld %12lld\n",
 			       inet_ntoa((struct in_addr){htonl(next_key.src)}),
 			       next_key.port16[0],
@@ -80,5 +103,8 @@ int main(int argc, char **argv)
 			key = next_key;
 		}
 	}
+
+cleanup:
+	bpf_object__close(obj);
 	return 0;
 }
diff --git a/samples/bpf/tracex5_user.c b/samples/bpf/tracex5_user.c
index c2317b39e0d2..a88b234fac32 100644
--- a/samples/bpf/tracex5_user.c
+++ b/samples/bpf/tracex5_user.c
@@ -1,15 +1,24 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <stdio.h>
-#include <linux/bpf.h>
+#include <stdlib.h>
 #include <unistd.h>
 #include <linux/filter.h>
 #include <linux/seccomp.h>
 #include <sys/prctl.h>
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
 #include <sys/resource.h>
 #include "trace_helpers.h"
 
+#define __must_check
+#include <linux/err.h>
+
+#ifdef __mips__
+#define	MAX_ENTRIES  6000 /* MIPS n64 syscalls start at 5000 */
+#else
+#define	MAX_ENTRIES  1024
+#endif
+
 /* install fake seccomp program to enable seccomp code path inside the kernel,
  * so that our kprobe attached to seccomp_phase1() can be triggered
  */
@@ -28,16 +37,58 @@ static void install_accept_all_seccomp(void)
 
 int main(int ac, char **argv)
 {
-	FILE *f;
-	char filename[256];
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_link *link = NULL;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	int key, fd, progs_fd;
+	char filename[256];
+	const char *title;
+	FILE *f;
 
-	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	setrlimit(RLIMIT_MEMLOCK, &r);
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	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;
+	}
+
+	prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
+	if (!prog) {
+		printf("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;
+	}
+
+	link = bpf_program__attach(prog);
+	if (IS_ERR(link)) {
+		fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+		link = NULL;
+		goto cleanup;
+	}
+
+	progs_fd = bpf_object__find_map_fd_by_name(obj, "progs");
+	if (progs_fd < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		title = bpf_program__title(prog, false);
+		/* register only syscalls to PROG_ARRAY */
+		if (sscanf(title, "kprobe/%d", &key) != 1)
+			continue;
+
+		fd = bpf_program__fd(prog);
+		bpf_map_update_elem(progs_fd, &key, &fd, BPF_ANY);
 	}
 
 	install_accept_all_seccomp();
@@ -47,5 +98,8 @@ int main(int ac, char **argv)
 
 	read_trace_pipe();
 
+cleanup:
+	bpf_link__destroy(link);
+	bpf_object__close(obj);
 	return 0;
 }
-- 
2.25.1

[PATCH bpf-next 3/3] samples: bpf: refactor kprobe, tail call kern progs map definition

[Daniel T. Lee]

Because the previous two commit replaced the bpf_load implementation of
the user program with libbpf, the corresponding kernel program's MAP
definition can be replaced with new BTF-defined map syntax.

This commit only updates the samples which uses libbpf API for loading
bpf program not with bpf_load.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/sampleip_kern.c    | 12 +++++------
 samples/bpf/sockex3_kern.c     | 36 ++++++++++++++++----------------
 samples/bpf/trace_event_kern.c | 24 ++++++++++-----------
 samples/bpf/tracex2_kern.c     | 24 ++++++++++-----------
 samples/bpf/tracex3_kern.c     | 24 ++++++++++-----------
 samples/bpf/tracex4_kern.c     | 12 +++++------
 samples/bpf/tracex5_kern.c     | 14 ++++++-------
 samples/bpf/tracex6_kern.c     | 38 ++++++++++++++++++----------------
 8 files changed, 93 insertions(+), 91 deletions(-)

diff --git a/samples/bpf/sampleip_kern.c b/samples/bpf/sampleip_kern.c
index e504dc308371..f24806ac24e7 100644
--- a/samples/bpf/sampleip_kern.c
+++ b/samples/bpf/sampleip_kern.c
@@ -13,12 +13,12 @@
 
 #define MAX_IPS		8192
 
-struct bpf_map_def SEC("maps") ip_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u64),
-	.value_size = sizeof(u32),
-	.max_entries = MAX_IPS,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u64);
+	__type(value, u32);
+	__uint(max_entries, MAX_IPS);
+} ip_map SEC(".maps");
 
 SEC("perf_event")
 int do_sample(struct bpf_perf_event_data *ctx)
diff --git a/samples/bpf/sockex3_kern.c b/samples/bpf/sockex3_kern.c
index 36d4dac23549..3304fabd636d 100644
--- a/samples/bpf/sockex3_kern.c
+++ b/samples/bpf/sockex3_kern.c
@@ -19,12 +19,12 @@
 
 #define PROG(F) SEC("socket/"__stringify(F)) int bpf_func_##F
 
-struct bpf_map_def SEC("maps") jmp_table = {
-	.type = BPF_MAP_TYPE_PROG_ARRAY,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(u32),
-	.max_entries = 8,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(key_size, sizeof(u32));
+	__uint(value_size, sizeof(u32));
+	__uint(max_entries, 8);
+} jmp_table SEC(".maps");
 
 #define PARSE_VLAN 1
 #define PARSE_MPLS 2
@@ -92,12 +92,12 @@ struct globals {
 	struct flow_key_record flow;
 };
 
-struct bpf_map_def SEC("maps") percpu_map = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(__u32),
-	.value_size = sizeof(struct globals),
-	.max_entries = 32,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, __u32);
+	__type(value, struct globals);
+	__uint(max_entries, 32);
+} percpu_map SEC(".maps");
 
 /* user poor man's per_cpu until native support is ready */
 static struct globals *this_cpu_globals(void)
@@ -113,12 +113,12 @@ struct pair {
 	__u64 bytes;
 };
 
-struct bpf_map_def SEC("maps") hash_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(struct flow_key_record),
-	.value_size = sizeof(struct pair),
-	.max_entries = 1024,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, struct flow_key_record);
+	__type(value, struct pair);
+	__uint(max_entries, 1024);
+} hash_map SEC(".maps");
 
 static void update_stats(struct __sk_buff *skb, struct globals *g)
 {
diff --git a/samples/bpf/trace_event_kern.c b/samples/bpf/trace_event_kern.c
index da1d69e20645..7d3c66fb3f88 100644
--- a/samples/bpf/trace_event_kern.c
+++ b/samples/bpf/trace_event_kern.c
@@ -18,19 +18,19 @@ struct key_t {
 	u32 userstack;
 };
 
-struct bpf_map_def SEC("maps") counts = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(struct key_t),
-	.value_size = sizeof(u64),
-	.max_entries = 10000,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, struct key_t);
+	__type(value, u64);
+	__uint(max_entries, 10000);
+} counts SEC(".maps");
 
-struct bpf_map_def SEC("maps") stackmap = {
-	.type = BPF_MAP_TYPE_STACK_TRACE,
-	.key_size = sizeof(u32),
-	.value_size = PERF_MAX_STACK_DEPTH * sizeof(u64),
-	.max_entries = 10000,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_STACK_TRACE);
+	__uint(key_size, sizeof(u32));
+	__uint(value_size, PERF_MAX_STACK_DEPTH * sizeof(u64));
+	__uint(max_entries, 10000);
+} stackmap SEC(".maps");
 
 #define KERN_STACKID_FLAGS (0 | BPF_F_FAST_STACK_CMP)
 #define USER_STACKID_FLAGS (0 | BPF_F_FAST_STACK_CMP | BPF_F_USER_STACK)
diff --git a/samples/bpf/tracex2_kern.c b/samples/bpf/tracex2_kern.c
index ff5d00916733..a2409abd7b87 100644
--- a/samples/bpf/tracex2_kern.c
+++ b/samples/bpf/tracex2_kern.c
@@ -17,12 +17,12 @@
 #define SYSCALL
 #endif
 
-struct bpf_map_def SEC("maps") my_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(long),
-	.value_size = sizeof(long),
-	.max_entries = 1024,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, long);
+	__type(value, long);
+	__uint(max_entries, 1024);
+} my_map SEC(".maps");
 
 /* kprobe is NOT a stable ABI. If kernel internals change this bpf+kprobe
  * example will no longer be meaningful
@@ -76,12 +76,12 @@ struct hist_key {
 	u64 index;
 };
 
-struct bpf_map_def SEC("maps") my_hist_map = {
-	.type = BPF_MAP_TYPE_PERCPU_HASH,
-	.key_size = sizeof(struct hist_key),
-	.value_size = sizeof(long),
-	.max_entries = 1024,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_HASH);
+	__uint(key_size, sizeof(struct hist_key));
+	__uint(value_size, sizeof(long));
+	__uint(max_entries, 1024);
+} my_hist_map SEC(".maps");
 
 SEC("kprobe/" SYSCALL "sys_write")
 int bpf_prog3(struct pt_regs *ctx)
diff --git a/samples/bpf/tracex3_kern.c b/samples/bpf/tracex3_kern.c
index fe21c14feb8d..659613c19a82 100644
--- a/samples/bpf/tracex3_kern.c
+++ b/samples/bpf/tracex3_kern.c
@@ -11,12 +11,12 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
 
-struct bpf_map_def SEC("maps") my_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(long),
-	.value_size = sizeof(u64),
-	.max_entries = 4096,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, long);
+	__type(value, u64);
+	__uint(max_entries, 4096);
+} my_map SEC(".maps");
 
 /* kprobe is NOT a stable ABI. If kernel internals change this bpf+kprobe
  * example will no longer be meaningful
@@ -42,12 +42,12 @@ static unsigned int log2l(unsigned long long n)
 
 #define SLOTS 100
 
-struct bpf_map_def SEC("maps") lat_map = {
-	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(u64),
-	.max_entries = SLOTS,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
+	__uint(key_size, sizeof(u32));
+	__uint(value_size, sizeof(u64));
+	__uint(max_entries, SLOTS);
+} lat_map SEC(".maps");
 
 SEC("kprobe/blk_account_io_completion")
 int bpf_prog2(struct pt_regs *ctx)
diff --git a/samples/bpf/tracex4_kern.c b/samples/bpf/tracex4_kern.c
index b1bb9df88f8e..eb0f8fdd14bf 100644
--- a/samples/bpf/tracex4_kern.c
+++ b/samples/bpf/tracex4_kern.c
@@ -15,12 +15,12 @@ struct pair {
 	u64 ip;
 };
 
-struct bpf_map_def SEC("maps") my_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(long),
-	.value_size = sizeof(struct pair),
-	.max_entries = 1000000,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, long);
+	__type(value, struct pair);
+	__uint(max_entries, 1000000);
+} my_map SEC(".maps");
 
 /* kprobe is NOT a stable ABI. If kernel internals change this bpf+kprobe
  * example will no longer be meaningful
diff --git a/samples/bpf/tracex5_kern.c b/samples/bpf/tracex5_kern.c
index 481790fde864..32b49e8ab6bd 100644
--- a/samples/bpf/tracex5_kern.c
+++ b/samples/bpf/tracex5_kern.c
@@ -15,16 +15,16 @@
 
 #define PROG(F) SEC("kprobe/"__stringify(F)) int bpf_func_##F
 
-struct bpf_map_def SEC("maps") progs = {
-	.type = BPF_MAP_TYPE_PROG_ARRAY,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(u32),
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(key_size, sizeof(u32));
+	__uint(value_size, sizeof(u32));
 #ifdef __mips__
-	.max_entries = 6000, /* MIPS n64 syscalls start at 5000 */
+	__uint(max_entries, 6000); /* MIPS n64 syscalls start at 5000 */
 #else
-	.max_entries = 1024,
+	__uint(max_entries, 1024);
 #endif
-};
+} progs SEC(".maps");
 
 SEC("kprobe/__seccomp_filter")
 int bpf_prog1(struct pt_regs *ctx)
diff --git a/samples/bpf/tracex6_kern.c b/samples/bpf/tracex6_kern.c
index 96c234efa852..acad5712d8b4 100644
--- a/samples/bpf/tracex6_kern.c
+++ b/samples/bpf/tracex6_kern.c
@@ -3,24 +3,26 @@
 #include <uapi/linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
-struct bpf_map_def SEC("maps") counters = {
-	.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
-	.key_size = sizeof(int),
-	.value_size = sizeof(u32),
-	.max_entries = 64,
-};
-struct bpf_map_def SEC("maps") values = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(int),
-	.value_size = sizeof(u64),
-	.max_entries = 64,
-};
-struct bpf_map_def SEC("maps") values2 = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(int),
-	.value_size = sizeof(struct bpf_perf_event_value),
-	.max_entries = 64,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__uint(key_size, sizeof(int));
+	__uint(value_size, sizeof(u32));
+	__uint(max_entries, 64);
+} counters SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, int);
+	__type(value, u64);
+	__uint(max_entries, 64);
+} values SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, int);
+	__type(value, struct bpf_perf_event_value);
+	__uint(max_entries, 64);
+} values2 SEC(".maps");
 
 SEC("kprobe/htab_map_get_next_key")
 int bpf_prog1(struct pt_regs *ctx)
-- 
2.25.1

Re: [PATCH bpf-next 1/3] samples: bpf: refactor kprobe tracing user progs with libbpf

[Yonghong Song]

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@gmail.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);
[...]

Re: [PATCH bpf-next 1/3] samples: bpf: refactor kprobe tracing user progs with libbpf

[Daniel T. Lee]

On Wed, May 13, 2020 at 10:40 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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@gmail.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?
>

This approach looks more apparent and can stick with the libbpf API.
I'll update code using this way.

> > +#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.
>

As you said, it would be better to return right away than to proceed
any further. I'll apply the code at next 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.
>

I totally agree that multiple labels are much more intuitive.
But It's not very common to jump to the destroy_link label.

Either when on the routine is completed successfully and jumps to the
destroy_link branch, or an error occurred while bpf_program__attach
was called "several" times and jumps to the destroy_link branch.

Single bpf_program__attach like this tracex1 sample doesn't really have
to destroy link, since the link has been set to NULL on attach error and
bpf_link__destroy() is designed to do nothing if passed NULL to it.

So I think current approach will keep consistent between samples since
most of the sample won't need to jump to destroy_link.

> >       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
>

That was also one of the considerations when writing patches.
I'm planning to refactor most of the programs in the sample using
libbpf, and found out that there are bunch of samples that tracks
syscall with kprobe. Replacing all of them will take lots of macros
and I thought using prefix will be better idea.

Actually, my initial plan was to create macro of SYSCALL()

       #ifdef __x86_64__
       #define PREFIX "__x64_"
       #elif defined(__s390x__)
       #define PREFIX "__s390x_"
       #else
       #define PREFIX ""
       #endif

       #define SYSCALL(SYS) PREFIX ## SYS

And to use this macro universally without creating additional headers,
I was trying to add this to samples/bpf/syscall_nrs.c which later
compiles to samples/bpf/syscall_nrs.h. But it was pretty hacky way and
it won't work properly. So I ended up with just adding prefix to syscall.

Is it necessary to define all of the macro for each architecture?

> > +
> >   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);
> [...]


Thank you for your time and effort for the review :)

Best,
Daniel

Re: [PATCH bpf-next 1/3] samples: bpf: refactor kprobe tracing user progs with libbpf

[Yonghong Song]

On 5/12/20 11:51 PM, Daniel T. Lee wrote:
> On Wed, May 13, 2020 at 10:40 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> 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@gmail.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?
>>
> 
> This approach looks more apparent and can stick with the libbpf API.
> I'll update code using this way.
> 
>>> +#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.
>>
> 
> As you said, it would be better to return right away than to proceed
> any further. I'll apply the code at next 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.
>>
> 
> I totally agree that multiple labels are much more intuitive.
> But It's not very common to jump to the destroy_link label.
> 
> Either when on the routine is completed successfully and jumps to the
> destroy_link branch, or an error occurred while bpf_program__attach
> was called "several" times and jumps to the destroy_link branch.
> 
> Single bpf_program__attach like this tracex1 sample doesn't really have
> to destroy link, since the link has been set to NULL on attach error and
> bpf_link__destroy() is designed to do nothing if passed NULL to it.
> 
> So I think current approach will keep consistent between samples since
> most of the sample won't need to jump to destroy_link.

Since this is the sample code, I won't enforce that. So yes, you can
keep your current approach.

> 
>>>        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
>>
> 
> That was also one of the considerations when writing patches.
> I'm planning to refactor most of the programs in the sample using
> libbpf, and found out that there are bunch of samples that tracks
> syscall with kprobe. Replacing all of them will take lots of macros
> and I thought using prefix will be better idea.
> 
> Actually, my initial plan was to create macro of SYSCALL()
> 
>         #ifdef __x86_64__
>         #define PREFIX "__x64_"
>         #elif defined(__s390x__)
>         #define PREFIX "__s390x_"
>         #else
>         #define PREFIX ""
>         #endif
> 
>         #define SYSCALL(SYS) PREFIX ## SYS
> 
> And to use this macro universally without creating additional headers,
> I was trying to add this to samples/bpf/syscall_nrs.c which later
> compiles to samples/bpf/syscall_nrs.h. But it was pretty hacky way and
> it won't work properly. So I ended up with just adding prefix to syscall.

I think it is okay to create a trace_common.h to have this definition
defined in one place and use them in bpf programs.

> 
> Is it necessary to define all of the macro for each architecture?

Yes, if we define in trace_common.h, let us do for x64/x390x/others
similar to the above.

> 
>>> +
>>>    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);
>> [...]
> 
> 
> Thank you for your time and effort for the review :)
> 
> Best,
> Daniel
> 

Re: [PATCH bpf-next 1/3] samples: bpf: refactor kprobe tracing user progs with libbpf

[Daniel T. Lee]

On Thu, May 14, 2020 at 12:29 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 5/12/20 11:51 PM, Daniel T. Lee wrote:
> > On Wed, May 13, 2020 at 10:40 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> 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@gmail.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?
> >>
> >
> > This approach looks more apparent and can stick with the libbpf API.
> > I'll update code using this way.
> >
> >>> +#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.
> >>
> >
> > As you said, it would be better to return right away than to proceed
> > any further. I'll apply the code at next 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.
> >>
> >
> > I totally agree that multiple labels are much more intuitive.
> > But It's not very common to jump to the destroy_link label.
> >
> > Either when on the routine is completed successfully and jumps to the
> > destroy_link branch, or an error occurred while bpf_program__attach
> > was called "several" times and jumps to the destroy_link branch.
> >
> > Single bpf_program__attach like this tracex1 sample doesn't really have
> > to destroy link, since the link has been set to NULL on attach error and
> > bpf_link__destroy() is designed to do nothing if passed NULL to it.
> >
> > So I think current approach will keep consistent between samples since
> > most of the sample won't need to jump to destroy_link.
>
> Since this is the sample code, I won't enforce that. So yes, you can
> keep your current approach.
>
> >
> >>>        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
> >>
> >
> > That was also one of the considerations when writing patches.
> > I'm planning to refactor most of the programs in the sample using
> > libbpf, and found out that there are bunch of samples that tracks
> > syscall with kprobe. Replacing all of them will take lots of macros
> > and I thought using prefix will be better idea.
> >
> > Actually, my initial plan was to create macro of SYSCALL()
> >
> >         #ifdef __x86_64__
> >         #define PREFIX "__x64_"
> >         #elif defined(__s390x__)
> >         #define PREFIX "__s390x_"
> >         #else
> >         #define PREFIX ""
> >         #endif
> >
> >         #define SYSCALL(SYS) PREFIX ## SYS
> >
> > And to use this macro universally without creating additional headers,
> > I was trying to add this to samples/bpf/syscall_nrs.c which later
> > compiles to samples/bpf/syscall_nrs.h. But it was pretty hacky way and
> > it won't work properly. So I ended up with just adding prefix to syscall.
>
> I think it is okay to create a trace_common.h to have this definition
> defined in one place and use them in bpf programs.
>
> >
> > Is it necessary to define all of the macro for each architecture?
>
> Yes, if we define in trace_common.h, let us do for x64/x390x/others
> similar to the above.
>

Sounds great. I'll add trace_common.h and apply the syscall macro.
I'll send a new version of the patch soon.

> >
> >>> +
> >>>    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);
> >> [...]
> >
> >
> > Thank you for your time and effort for the review :)
> >
> > Best,
> > Daniel
> >

Thank you for your time and effort for the review :)

-- 
Best,
Daniel T. Lee