[PATCH 1/3] samples: bpf: fix bpf programs with kprobe/sys_connect event

[PATCH 1/3] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Daniel T. Lee]

Currently, BPF programs with kprobe/sys_connect does not work properly.

Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
This commit modifies the bpf_load behavior of kprobe events in the x64
architecture. If the current kprobe event target starts with "sys_*",
add the prefix "__x64_" to the front of the event.

Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
solution to most of the problems caused by the commit below.

    commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
    pt_regs-based sys_*() to __x64_sys_*()")

However, there is a problem with the sys_connect kprobe event that does
not work properly. For __sys_connect event, parameters can be fetched
normally, but for __x64_sys_connect, parameters cannot be fetched.

Because of this problem, this commit fixes the sys_connect event by
specifying the __sys_connect directly and this will bypass the
"__x64_" appending rule of bpf_load.

Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/map_perf_test_kern.c         | 2 +-
 samples/bpf/test_map_in_map_kern.c       | 2 +-
 samples/bpf/test_probe_write_user_kern.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
index 1bb1fdb9cdf8..69ecd717d998 100644
--- a/samples/bpf/map_perf_test_kern.c
+++ b/samples/bpf/map_perf_test_kern.c
@@ -159,7 +159,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_connect")
+SEC("kprobe/__sys_connect")
 int stress_lru_hmap_alloc(struct pt_regs *ctx)
 {
 	char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
index 6cee61e8ce9b..b1562ba2f025 100644
--- a/samples/bpf/test_map_in_map_kern.c
+++ b/samples/bpf/test_map_in_map_kern.c
@@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
 	return result ? *result : -ENOENT;
 }
 
-SEC("kprobe/sys_connect")
+SEC("kprobe/__sys_connect")
 int trace_sys_connect(struct pt_regs *ctx)
 {
 	struct sockaddr_in6 *in6;
diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
index 6579639a83b2..9b3c3918c37d 100644
--- a/samples/bpf/test_probe_write_user_kern.c
+++ b/samples/bpf/test_probe_write_user_kern.c
@@ -26,7 +26,7 @@ struct {
  * This example sits on a syscall, and the syscall ABI is relatively stable
  * of course, across platforms, and over time, the ABI may change.
  */
-SEC("kprobe/sys_connect")
+SEC("kprobe/__sys_connect")
 int bpf_prog1(struct pt_regs *ctx)
 {
 	struct sockaddr_in new_addr, orig_addr = {};
-- 
2.25.1

[PATCH 2/3] samples: bpf: cleanup pointer error check with libbpf

[Daniel T. Lee]

Libbpf has its own helper function to check for errors in the bpf
data structure (pointer). And Some codes do not use this libbbpf
helper function and check the pointer's error directly.

This commit clean up the existing pointer error check logic with
libbpf.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/sampleip_user.c    | 2 +-
 samples/bpf/trace_event_user.c | 2 +-
 samples/bpf/tracex1_user.c     | 2 +-
 samples/bpf/tracex5_user.c     | 2 +-
 samples/bpf/tracex7_user.c     | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index 921c505bb567..554dfa1cb34d 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c
@@ -186,7 +186,7 @@ int main(int argc, char **argv)
 	}
 
 	prog = bpf_object__find_program_by_name(obj, "do_sample");
-	if (!prog) {
+	if (libbpf_get_error(prog)) {
 		fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
 		goto cleanup;
 	}
diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index ac1ba368195c..8bcb9b4bfbe6 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c
@@ -318,7 +318,7 @@ int main(int argc, char **argv)
 	}
 
 	prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
-	if (!prog) {
+	if (libbpf_get_error(prog)) {
 		printf("finding a prog in obj file failed\n");
 		goto cleanup;
 	}
diff --git a/samples/bpf/tracex1_user.c b/samples/bpf/tracex1_user.c
index 9d4adb7fd834..d3e01807dcd0 100644
--- a/samples/bpf/tracex1_user.c
+++ b/samples/bpf/tracex1_user.c
@@ -20,7 +20,7 @@ int main(int ac, char **argv)
 	}
 
 	prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
-	if (!prog) {
+	if (libbpf_get_error(prog)) {
 		fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
 		goto cleanup;
 	}
diff --git a/samples/bpf/tracex5_user.c b/samples/bpf/tracex5_user.c
index 98dad57a96c4..65c753b30121 100644
--- a/samples/bpf/tracex5_user.c
+++ b/samples/bpf/tracex5_user.c
@@ -53,7 +53,7 @@ int main(int ac, char **argv)
 	}
 
 	prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
-	if (!prog) {
+	if (libbpf_get_error(prog)) {
 		printf("finding a prog in obj file failed\n");
 		goto cleanup;
 	}
diff --git a/samples/bpf/tracex7_user.c b/samples/bpf/tracex7_user.c
index fdcd6580dd73..10c896732139 100644
--- a/samples/bpf/tracex7_user.c
+++ b/samples/bpf/tracex7_user.c
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
 	}
 
 	prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
-	if (!prog) {
+	if (libbpf_get_error(prog)) {
 		fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
 		goto cleanup;
 	}
-- 
2.25.1

Re: [PATCH 2/3] samples: bpf: cleanup pointer error check with libbpf

[Andrii Nakryiko]

On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> Libbpf has its own helper function to check for errors in the bpf
> data structure (pointer). And Some codes do not use this libbbpf
> helper function and check the pointer's error directly.
>
> This commit clean up the existing pointer error check logic with
> libbpf.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---

This entire patch is wrong. bpf_object__find_program_by_name() returns
NULL if the program is not found, not an error code.

>  samples/bpf/sampleip_user.c    | 2 +-
>  samples/bpf/trace_event_user.c | 2 +-
>  samples/bpf/tracex1_user.c     | 2 +-
>  samples/bpf/tracex5_user.c     | 2 +-
>  samples/bpf/tracex7_user.c     | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
>

[...]

Re: [PATCH 2/3] samples: bpf: cleanup pointer error check with libbpf

[Daniel T. Lee]

2020년 6월 27일 (토) 05:25, Andrii Nakryiko <andrii.nakryiko@gmail.com>님이 작성:
>
> On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > Libbpf has its own helper function to check for errors in the bpf
> > data structure (pointer). And Some codes do not use this libbbpf
> > helper function and check the pointer's error directly.
> >
> > This commit clean up the existing pointer error check logic with
> > libbpf.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
>
> This entire patch is wrong. bpf_object__find_program_by_name() returns
> NULL if the program is not found, not an error code.
>

Oops, I'll drop the patch and resend with the next version.

Thanks for your time and effort for the review.
Daniel

> >  samples/bpf/sampleip_user.c    | 2 +-
> >  samples/bpf/trace_event_user.c | 2 +-
> >  samples/bpf/tracex1_user.c     | 2 +-
> >  samples/bpf/tracex5_user.c     | 2 +-
> >  samples/bpf/tracex7_user.c     | 2 +-
> >  5 files changed, 5 insertions(+), 5 deletions(-)
> >
>
> [...]

[PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

[Daniel T. Lee]

From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
support"), a way to define internal map in BTF-defined map has been
added.

Instead of using previous 'inner_map_idx' definition, the structure to
be used for the inner map can be directly defined using array directive.

    __array(values, struct inner_map)

This commit refactors map in map test program with libbpf by explicitly
defining inner map with BTF-defined format.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/Makefile               |  2 +-
 samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
 samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
 3 files changed, 91 insertions(+), 49 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ffd0fda536da..78678d4e6842 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -93,7 +93,7 @@ sampleip-objs := sampleip_user.o $(TRACE_HELPERS)
 tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o
 lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o
 xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o
-test_map_in_map-objs := bpf_load.o test_map_in_map_user.o
+test_map_in_map-objs := test_map_in_map_user.o
 per_socket_stats_example-objs := cookie_uid_helper_example.o
 xdp_redirect-objs := xdp_redirect_user.o
 xdp_redirect_map-objs := xdp_redirect_map_user.o
diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
index b1562ba2f025..d3f56ed78541 100644
--- a/samples/bpf/test_map_in_map_kern.c
+++ b/samples/bpf/test_map_in_map_kern.c
@@ -11,66 +11,65 @@
 #include <uapi/linux/bpf.h>
 #include <uapi/linux/in6.h>
 #include <bpf/bpf_helpers.h>
-#include "bpf_legacy.h"
 #include <bpf/bpf_tracing.h>
 
 #define MAX_NR_PORTS 65536
 
 /* map #0 */
-struct bpf_map_def_legacy SEC("maps") port_a = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = MAX_NR_PORTS,
-};
+struct inner_a {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, MAX_NR_PORTS);
+} port_a SEC(".maps");
 
 /* map #1 */
-struct bpf_map_def_legacy SEC("maps") port_h = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = 1,
-};
+struct inner_h {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, 1);
+} port_h SEC(".maps");
 
 /* map #2 */
-struct bpf_map_def_legacy SEC("maps") reg_result_h = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, 1);
+} reg_result_h SEC(".maps");
 
 /* map #3 */
-struct bpf_map_def_legacy SEC("maps") inline_result_h = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, 1);
+} inline_result_h SEC(".maps");
 
 /* map #4 */ /* Test case #0 */
-struct bpf_map_def_legacy SEC("maps") a_of_port_a = {
-	.type = BPF_MAP_TYPE_ARRAY_OF_MAPS,
-	.key_size = sizeof(u32),
-	.inner_map_idx = 0, /* map_fd[0] is port_a */
-	.max_entries = MAX_NR_PORTS,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, MAX_NR_PORTS);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_a); /* use inner_a as inner map */
+} a_of_port_a SEC(".maps");
 
 /* map #5 */ /* Test case #1 */
-struct bpf_map_def_legacy SEC("maps") h_of_port_a = {
-	.type = BPF_MAP_TYPE_HASH_OF_MAPS,
-	.key_size = sizeof(u32),
-	.inner_map_idx = 0, /* map_fd[0] is port_a */
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_a); /* use inner_a as inner map */
+} h_of_port_a SEC(".maps");
 
 /* map #6 */ /* Test case #2 */
-struct bpf_map_def_legacy SEC("maps") h_of_port_h = {
-	.type = BPF_MAP_TYPE_HASH_OF_MAPS,
-	.key_size = sizeof(u32),
-	.inner_map_idx = 1, /* map_fd[1] is port_h */
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_h); /* use inner_h as inner map */
+} h_of_port_h SEC(".maps");
 
 static __always_inline int do_reg_lookup(void *inner_map, u32 port)
 {
diff --git a/samples/bpf/test_map_in_map_user.c b/samples/bpf/test_map_in_map_user.c
index eb29bcb76f3f..e5bddfff696f 100644
--- a/samples/bpf/test_map_in_map_user.c
+++ b/samples/bpf/test_map_in_map_user.c
@@ -11,7 +11,9 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
+
+static int map_fd[7];
 
 #define PORT_A		(map_fd[0])
 #define PORT_H		(map_fd[1])
@@ -113,18 +115,59 @@ static void test_map_in_map(void)
 int main(int argc, char **argv)
 {
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_link *link = NULL;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
 
-	assert(!setrlimit(RLIMIT_MEMLOCK, &r));
+	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 (libbpf_get_error(obj)) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		return 0;
+	}
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
+	if (libbpf_get_error(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;
+	}
+
+	map_fd[0] = bpf_object__find_map_fd_by_name(obj, "port_a");
+	map_fd[1] = bpf_object__find_map_fd_by_name(obj, "port_h");
+	map_fd[2] = bpf_object__find_map_fd_by_name(obj, "reg_result_h");
+	map_fd[3] = bpf_object__find_map_fd_by_name(obj, "inline_result_h");
+	map_fd[4] = bpf_object__find_map_fd_by_name(obj, "a_of_port_a");
+	map_fd[5] = bpf_object__find_map_fd_by_name(obj, "h_of_port_a");
+	map_fd[6] = bpf_object__find_map_fd_by_name(obj, "h_of_port_h");
+	if (map_fd[0] < 0 || map_fd[1] < 0 || map_fd[2] < 0 ||
+	    map_fd[3] < 0 || map_fd[4] < 0 || map_fd[5] < 0 || map_fd[6] < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	link = bpf_program__attach(prog);
+	if (libbpf_get_error(link)) {
+		fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+		link = NULL;
+		goto cleanup;
 	}
 
 	test_map_in_map();
 
+cleanup:
+	bpf_link__destroy(link);
+	bpf_object__close(obj);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

[Andrii Nakryiko]

On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> support"), a way to define internal map in BTF-defined map has been
> added.
>
> Instead of using previous 'inner_map_idx' definition, the structure to
> be used for the inner map can be directly defined using array directive.
>
>     __array(values, struct inner_map)
>
> This commit refactors map in map test program with libbpf by explicitly
> defining inner map with BTF-defined format.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---

Thanks for the clean up, looks good except that prog NULL check.

It also seems like this is the last use of bpf_map_def_legacy, do you
mind removing it as well?


>  samples/bpf/Makefile               |  2 +-
>  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
>  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
>  3 files changed, 91 insertions(+), 49 deletions(-)
>

[...]

>
>         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> +       obj = bpf_object__open_file(filename, NULL);
> +       if (libbpf_get_error(obj)) {

this is right, but...

> +               fprintf(stderr, "ERROR: opening BPF object file failed\n");
> +               return 0;
> +       }
>
> -       if (load_bpf_file(filename)) {
> -               printf("%s", bpf_log_buf);
> -               return 1;
> +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> +       if (libbpf_get_error(prog)) {

this is wrong. Just NULL check. libbpf APIs are not very consistent
with what they return, unfortunately.

> +               printf("finding a prog in obj file failed\n");
> +               goto cleanup;
> +       }
> +

[...]

Re: [PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

[Daniel T. Lee]

On Sat, Jun 27, 2020 at 5:30 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> > support"), a way to define internal map in BTF-defined map has been
> > added.
> >
> > Instead of using previous 'inner_map_idx' definition, the structure to
> > be used for the inner map can be directly defined using array directive.
> >
> >     __array(values, struct inner_map)
> >
> > This commit refactors map in map test program with libbpf by explicitly
> > defining inner map with BTF-defined format.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
>
> Thanks for the clean up, looks good except that prog NULL check.
>

I'll fix this NULL check as well too.

> It also seems like this is the last use of bpf_map_def_legacy, do you
> mind removing it as well?
>

Actually, there is one more place that uses bpf_map_def_legacy.
map_perf_test_kern.c is the one, and I'm currently working on it, but
I'm having difficulty with refactoring this file at the moment.

It has a hash_map map definition named inner_lru_hash_map with
BPF_F_NUMA_NODE flag and '.numa_node = 0'.

The bpf_map_def in libbpf has the attribute name map_flags but
it does not have the numa_node attribute. Because the numa node
for bpf_map_def cannot be explicitly specified, this means that there
is no way to set the numa node where the map will be placed at the
time of bpf_object__load.

The only approach currently available is not to use libbbpf to handle
everything (bpf_object_load), but instead to create a map directly with
specifying numa node (bpf_load approach).

    bpf_create_map_in_map_node
    bpf_create_map_node

I'm trying to stick with the libbpf implementation only, and I'm wondering
If I have to create bpf maps manually at _user.c program.

Any advice and suggestions will be greatly appreciated.

Thanks for your time and effort for the review.
Daniel.

>
> >  samples/bpf/Makefile               |  2 +-
> >  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
> >  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
> >  3 files changed, 91 insertions(+), 49 deletions(-)
> >
>
> [...]
>
> >
> >         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > +       obj = bpf_object__open_file(filename, NULL);
> > +       if (libbpf_get_error(obj)) {
>
> this is right, but...
>
> > +               fprintf(stderr, "ERROR: opening BPF object file failed\n");
> > +               return 0;
> > +       }
> >
> > -       if (load_bpf_file(filename)) {
> > -               printf("%s", bpf_log_buf);
> > -               return 1;
> > +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> > +       if (libbpf_get_error(prog)) {
>
> this is wrong. Just NULL check. libbpf APIs are not very consistent
> with what they return, unfortunately.
>
> > +               printf("finding a prog in obj file failed\n");
> > +               goto cleanup;
> > +       }
> > +
>
> [...]

Re: [PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

[Andrii Nakryiko]

On Fri, Jun 26, 2020 at 3:14 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Sat, Jun 27, 2020 at 5:30 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > >
> > > From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> > > support"), a way to define internal map in BTF-defined map has been
> > > added.
> > >
> > > Instead of using previous 'inner_map_idx' definition, the structure to
> > > be used for the inner map can be directly defined using array directive.
> > >
> > >     __array(values, struct inner_map)
> > >
> > > This commit refactors map in map test program with libbpf by explicitly
> > > defining inner map with BTF-defined format.
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > ---
> >
> > Thanks for the clean up, looks good except that prog NULL check.
> >
>
> I'll fix this NULL check as well too.
>
> > It also seems like this is the last use of bpf_map_def_legacy, do you
> > mind removing it as well?
> >
>
> Actually, there is one more place that uses bpf_map_def_legacy.
> map_perf_test_kern.c is the one, and I'm currently working on it, but
> I'm having difficulty with refactoring this file at the moment.
>
> It has a hash_map map definition named inner_lru_hash_map with
> BPF_F_NUMA_NODE flag and '.numa_node = 0'.
>
> The bpf_map_def in libbpf has the attribute name map_flags but
> it does not have the numa_node attribute. Because the numa node

It does since 1 or 2 days ago ([0])

  [0] https://patchwork.ozlabs.org/project/netdev/patch/20200621062112.3006313-1-andriin@fb.com/


> for bpf_map_def cannot be explicitly specified, this means that there
> is no way to set the numa node where the map will be placed at the
> time of bpf_object__load.
>
> The only approach currently available is not to use libbbpf to handle
> everything (bpf_object_load), but instead to create a map directly with
> specifying numa node (bpf_load approach).
>
>     bpf_create_map_in_map_node
>     bpf_create_map_node
>
> I'm trying to stick with the libbpf implementation only, and I'm wondering
> If I have to create bpf maps manually at _user.c program.
>
> Any advice and suggestions will be greatly appreciated.
>

It should be super straightforward now with a BTF-defined map
supporting numa_node attribute.

> Thanks for your time and effort for the review.
> Daniel.
>
> >
> > >  samples/bpf/Makefile               |  2 +-
> > >  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
> > >  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
> > >  3 files changed, 91 insertions(+), 49 deletions(-)
> > >
> >
> > [...]
> >
> > >
> > >         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > > +       obj = bpf_object__open_file(filename, NULL);
> > > +       if (libbpf_get_error(obj)) {
> >
> > this is right, but...
> >
> > > +               fprintf(stderr, "ERROR: opening BPF object file failed\n");
> > > +               return 0;
> > > +       }
> > >
> > > -       if (load_bpf_file(filename)) {
> > > -               printf("%s", bpf_log_buf);
> > > -               return 1;
> > > +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> > > +       if (libbpf_get_error(prog)) {
> >
> > this is wrong. Just NULL check. libbpf APIs are not very consistent
> > with what they return, unfortunately.
> >
> > > +               printf("finding a prog in obj file failed\n");
> > > +               goto cleanup;
> > > +       }
> > > +
> >
> > [...]

Re: [PATCH 3/3] samples: bpf: refactor BPF map in map test with libbpf

[Daniel T. Lee]

On Sat, Jun 27, 2020 at 7:19 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 3:14 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Sat, Jun 27, 2020 at 5:30 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > > >
> > > > From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> > > > support"), a way to define internal map in BTF-defined map has been
> > > > added.
> > > >
> > > > Instead of using previous 'inner_map_idx' definition, the structure to
> > > > be used for the inner map can be directly defined using array directive.
> > > >
> > > >     __array(values, struct inner_map)
> > > >
> > > > This commit refactors map in map test program with libbpf by explicitly
> > > > defining inner map with BTF-defined format.
> > > >
> > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > > ---
> > >
> > > Thanks for the clean up, looks good except that prog NULL check.
> > >
> >
> > I'll fix this NULL check as well too.
> >
> > > It also seems like this is the last use of bpf_map_def_legacy, do you
> > > mind removing it as well?
> > >
> >
> > Actually, there is one more place that uses bpf_map_def_legacy.
> > map_perf_test_kern.c is the one, and I'm currently working on it, but
> > I'm having difficulty with refactoring this file at the moment.
> >
> > It has a hash_map map definition named inner_lru_hash_map with
> > BPF_F_NUMA_NODE flag and '.numa_node = 0'.
> >
> > The bpf_map_def in libbpf has the attribute name map_flags but
> > it does not have the numa_node attribute. Because the numa node
>
> It does since 1 or 2 days ago ([0])
>
>   [0] https://patchwork.ozlabs.org/project/netdev/patch/20200621062112.3006313-1-andriin@fb.com/
>
>
> > for bpf_map_def cannot be explicitly specified, this means that there
> > is no way to set the numa node where the map will be placed at the
> > time of bpf_object__load.
> >
> > The only approach currently available is not to use libbbpf to handle
> > everything (bpf_object_load), but instead to create a map directly with
> > specifying numa node (bpf_load approach).
> >
> >     bpf_create_map_in_map_node
> >     bpf_create_map_node
> >
> > I'm trying to stick with the libbpf implementation only, and I'm wondering
> > If I have to create bpf maps manually at _user.c program.
> >
> > Any advice and suggestions will be greatly appreciated.
> >
>
> It should be super straightforward now with a BTF-defined map
> supporting numa_node attribute.
>

Awesome, thanks for letting me know!

I will use this new attribute for the map_perf_test refactoring.
Problem Solved!

Thanks.

> > Thanks for your time and effort for the review.
> > Daniel.
> >
> > >
> > > >  samples/bpf/Makefile               |  2 +-
> > > >  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
> > > >  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
> > > >  3 files changed, 91 insertions(+), 49 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > >
> > > >         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> > > > +       obj = bpf_object__open_file(filename, NULL);
> > > > +       if (libbpf_get_error(obj)) {
> > >
> > > this is right, but...
> > >
> > > > +               fprintf(stderr, "ERROR: opening BPF object file failed\n");
> > > > +               return 0;
> > > > +       }
> > > >
> > > > -       if (load_bpf_file(filename)) {
> > > > -               printf("%s", bpf_log_buf);
> > > > -               return 1;
> > > > +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> > > > +       if (libbpf_get_error(prog)) {
> > >
> > > this is wrong. Just NULL check. libbpf APIs are not very consistent
> > > with what they return, unfortunately.
> > >
> > > > +               printf("finding a prog in obj file failed\n");
> > > > +               goto cleanup;
> > > > +       }
> > > > +
> > >
> > > [...]

Re: [PATCH 1/3] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Andrii Nakryiko]

On Fri, Jun 26, 2020 at 1:18 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> Currently, BPF programs with kprobe/sys_connect does not work properly.
>
> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> This commit modifies the bpf_load behavior of kprobe events in the x64
> architecture. If the current kprobe event target starts with "sys_*",
> add the prefix "__x64_" to the front of the event.
>
> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> solution to most of the problems caused by the commit below.
>
>     commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
>     pt_regs-based sys_*() to __x64_sys_*()")
>
> However, there is a problem with the sys_connect kprobe event that does
> not work properly. For __sys_connect event, parameters can be fetched
> normally, but for __x64_sys_connect, parameters cannot be fetched.
>
> Because of this problem, this commit fixes the sys_connect event by
> specifying the __sys_connect directly and this will bypass the
> "__x64_" appending rule of bpf_load.
>
> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  samples/bpf/map_perf_test_kern.c         | 2 +-
>  samples/bpf/test_map_in_map_kern.c       | 2 +-
>  samples/bpf/test_probe_write_user_kern.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>

[...]