tools lib bpf: fixes and functional upgrade

PATCH In review Eric Leblond 3 participants 20 messages Oct 16, 2016 – Nov 8, 2016

Subsystems (4)

Networking Network drivers BPF libbpf

lore

Share & tools

lore

Eric Leblond

author

2016-10-16 21:21 UTC #1 lore raw

PATCH 1/8 tools lib bpf: add error functions

The include of err.h is not explicitely needed in exported functions and it was causing include conflict with some existing code due to redefining some macros.

To fix this, let's have error handling functions provided by the library. Furthermore this will allow user to have an homogeneous API.

Signed-off-by Eric Leblond <[email protected]>

---
 tools/lib/bpf/libbpf.c | 11 +++++++++++
 tools/lib/bpf/libbpf.h |  4 +++-
 2 files changed, 14 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +11 -0

index b699aea..90932f1 100644
@@ -31,6 +31,7 @@
 #include <linux/kernel.h>
 #include <linux/bpf.h>
 #include <linux/list.h>
+#include <linux/err.h>
 #include <libelf.h>
 #include <gelf.h>

@@ -1447,3 +1448,13 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
 	}
 	return NULL;
 }
+
+bool bpf__is_error(const void *ptr)
+{
+	return IS_ERR(ptr);
+}
+
+long bpf__get_error(const void *ptr)
+{
+	return PTR_ERR(ptr);
+}

tools/lib/bpf/libbpf.h +3 -1

index dd7a513..e40c8d3 100644
@@ -23,7 +23,6 @@
 
 #include <stdio.h>
 #include <stdbool.h>
-#include <linux/err.h>
 
 enum libbpf_errno {
 	__LIBBPF_ERRNO__START = 4000,

@@ -211,4 +210,7 @@ int bpf_map__set_priv(struct bpf_map *map, void *priv,
 		      bpf_map_clear_priv_t clear_priv);
 void *bpf_map__priv(struct bpf_map *map);
 
+bool bpf__is_error(const void *ptr);
+long bpf__get_error(const void *ptr);
+
 #endif

Signature · 1 line

2.9.3

Wangnan (F)

2016-10-17 01:47 UTC #2 lore raw

Re: [PATCH 1/8] tools lib bpf: add error functions

Quoted 27 lines · Eric Leblond

The include of err.h is not explicitely needed in exported functions and it was causing include conflict with some existing code due to redefining some macros.
To fix this, let's have error handling functions provided by the library. Furthermore this will allow user to have an homogeneous API.
Signed-off-by Eric Leblond <[email protected]>
---
  tools/lib/bpf/libbpf.c | 11 +++++++++++
  tools/lib/bpf/libbpf.h |  4 +++-
  2 files changed, 14 insertions(+), 1 deletion(-)
tools/lib/bpf/libbpf.c +1 -0
index b699aea..90932f1 100644
@@ -31,6 +31,7 @@
  #include <linux/kernel.h>
  #include <linux/bpf.h>
  #include <linux/list.h>
+#include <linux/err.h>
  #include <libelf.h>
  #include <gelf.h>
 

tools/lib/bpf/libbpf.c bpf_object__find_map_by_name(struct bpf_object *obj, const char *name) · line 1452

Quoted 5 lines

tools/lib/bpf/libbpf.c +2 -0

@@ -1447,3 +1448,13 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
  	}
  	return NULL;
  }
+
+bool bpf__is_error(const void *ptr)

Please use libbpf_is_error(), like libbpf_set_print. We use '__' because we want to use the OO concept. This utility is not OO.

tools/lib/bpf/libbpf.c bpf_object__find_map_by_name(struct bpf_object *obj, const char *name) · line 1457

Quoted 5 lines

tools/lib/bpf/libbpf.c +5 -0

+{
+	return IS_ERR(ptr);
+}
+
+long bpf__get_error(const void *ptr)

Same, please call it libbpf_get_error().

Thank you.

Joe Stringer

2016-10-18 22:57 UTC #3 lore raw

Re: [PATCH 1/8] tools lib bpf: add error functions

In reply to Eric Leblond

Quoted 9 lines · Eric Leblond

The include of err.h is not explicitely needed in exported functions and it was causing include conflict with some existing code due to redefining some macros.
To fix this, let's have error handling functions provided by the library. Furthermore this will allow user to have an homogeneous API.
Signed-off-by Eric Leblond <[email protected]>

Does it need to return the error like this or should we just fix up the bpf_object__open() API to return errors in a simpler form?

There's already libbpf_set_print(...) for outputting errors, is it reasonable to just change the library to return NULLs in error cases instead?

Wangnan (F)

2016-10-19 01:53 UTC #4 lore raw

Re: [PATCH 1/8] tools lib bpf: add error functions

Quoted 9 lines · Joe Stringer

Quoted 8 lines · Eric Leblond
The include of err.h is not explicitely needed in exported functions and it was causing include conflict with some existing code due to redefining some macros.
To fix this, let's have error handling functions provided by the library. Furthermore this will allow user to have an homogeneous API.

Quoted 7 lines

Quoted 1 line
Signed-off-by Eric Leblond <[email protected]>
Does it need to return the error like this or should we just fix up the bpf_object__open() API to return errors in a simpler form?
There's already libbpf_set_print(...) for outputting errors, is it reasonable to just change the library to return NULLs in error cases instead?

Returning error code to caller so caller knows what happen. Other subsystems in perf also do this.

Perf hides libbpf's error output (make it silent unless -v), so it needs a way for receiving libbpf's error code.

I think this patch is good, decouple libbpf.h and kernel headers.

Thank you.

Eric Leblond

author

2016-10-16 21:21 UTC #5 lore raw

PATCH 2/8 uapi linux bpf: add max value to enum

In reply to Eric Leblond

It will be used to detect userspace trying to set invalid value.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/bpf.h | 1 +
 1 file changed, 1 insertion(+)

include/uapi/linux/bpf.h +1 -0

index f09c70b..570287f 100644
@@ -96,6 +96,7 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_TRACEPOINT,
 	BPF_PROG_TYPE_XDP,
 	BPF_PROG_TYPE_PERF_EVENT,
+	__MAX_BPF_PROG_TYPE,
 };
 
 #define BPF_PSEUDO_MAP_FD	1

Signature · 1 line

2.9.3

Eric Leblond

author

2016-10-16 21:21 UTC #6 lore raw

PATCH 3/8 tools: Sync tools/include/uapi/linux/bpf.h with the kernel

In reply to Eric Leblond

Signed-off-by Eric Leblond <[email protected]>

---
 tools/include/uapi/linux/bpf.h | 52 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

tools/include/uapi/linux/bpf.h +52 -0

index 9e5fc16..570287f 100644
@@ -95,6 +95,8 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_SCHED_ACT,
 	BPF_PROG_TYPE_TRACEPOINT,
 	BPF_PROG_TYPE_XDP,
+	BPF_PROG_TYPE_PERF_EVENT,
+	__MAX_BPF_PROG_TYPE,
 };
 
 #define BPF_PSEUDO_MAP_FD	1

@@ -375,6 +377,56 @@ enum bpf_func_id {
 	 */
 	BPF_FUNC_probe_write_user,
 
+	/**
+	 * bpf_current_task_under_cgroup(map, index) - Check cgroup2 membership of current task
+	 * @map: pointer to bpf_map in BPF_MAP_TYPE_CGROUP_ARRAY type
+	 * @index: index of the cgroup in the bpf_map
+	 * Return:
+	 *   == 0 current failed the cgroup2 descendant test
+	 *   == 1 current succeeded the cgroup2 descendant test
+	 *    < 0 error
+	 */
+	BPF_FUNC_current_task_under_cgroup,
+
+	/**
+	 * bpf_skb_change_tail(skb, len, flags)
+	 * The helper will resize the skb to the given new size,
+	 * to be used f.e. with control messages.
+	 * @skb: pointer to skb
+	 * @len: new skb length
+	 * @flags: reserved
+	 * Return: 0 on success or negative error
+	 */
+	BPF_FUNC_skb_change_tail,
+
+	/**
+	 * bpf_skb_pull_data(skb, len)
+	 * The helper will pull in non-linear data in case the
+	 * skb is non-linear and not all of len are part of the
+	 * linear section. Only needed for read/write with direct
+	 * packet access.
+	 * @skb: pointer to skb
+	 * @len: len to make read/writeable
+	 * Return: 0 on success or negative error
+	 */
+	BPF_FUNC_skb_pull_data,
+
+	/**
+	 * bpf_csum_update(skb, csum)
+	 * Adds csum into skb->csum in case of CHECKSUM_COMPLETE.
+	 * @skb: pointer to skb
+	 * @csum: csum to add
+	 * Return: csum on success or negative error
+	 */
+	BPF_FUNC_csum_update,
+
+	/**
+	 * bpf_set_hash_invalid(skb)
+	 * Invalidate current skb>hash.
+	 * @skb: pointer to skb
+	 */
+	BPF_FUNC_set_hash_invalid,
+
 	__BPF_FUNC_MAX_ID,
 };

Signature · 1 line

2.9.3

Wangnan (F)

2016-10-17 01:48 UTC #7 lore raw

Re: [PATCH 3/8] tools: Sync tools/include/uapi/linux/bpf.h with the kernel

Quoted 1 line · Eric Leblond

Signed-off-by Eric Leblond <[email protected]>

Commit message is required.

Thank you.

Quoted 74 lines

---
  tools/include/uapi/linux/bpf.h | 52 ++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 52 insertions(+)

tools/include/uapi/linux/bpf.h +52 -0

index 9e5fc16..570287f 100644
@@ -95,6 +95,8 @@ enum bpf_prog_type {
  	BPF_PROG_TYPE_SCHED_ACT,
  	BPF_PROG_TYPE_TRACEPOINT,
  	BPF_PROG_TYPE_XDP,
+	BPF_PROG_TYPE_PERF_EVENT,
+	__MAX_BPF_PROG_TYPE,
  };
 
  #define BPF_PSEUDO_MAP_FD	1

@@ -375,6 +377,56 @@ enum bpf_func_id {
  	 */
  	BPF_FUNC_probe_write_user,
 
+	/**
+	 * bpf_current_task_under_cgroup(map, index) - Check cgroup2 membership of current task
+	 * @map: pointer to bpf_map in BPF_MAP_TYPE_CGROUP_ARRAY type
+	 * @index: index of the cgroup in the bpf_map
+	 * Return:
+	 *   == 0 current failed the cgroup2 descendant test
+	 *   == 1 current succeeded the cgroup2 descendant test
+	 *    < 0 error
+	 */
+	BPF_FUNC_current_task_under_cgroup,
+
+	/**
+	 * bpf_skb_change_tail(skb, len, flags)
+	 * The helper will resize the skb to the given new size,
+	 * to be used f.e. with control messages.
+	 * @skb: pointer to skb
+	 * @len: new skb length
+	 * @flags: reserved
+	 * Return: 0 on success or negative error
+	 */
+	BPF_FUNC_skb_change_tail,
+
+	/**
+	 * bpf_skb_pull_data(skb, len)
+	 * The helper will pull in non-linear data in case the
+	 * skb is non-linear and not all of len are part of the
+	 * linear section. Only needed for read/write with direct
+	 * packet access.
+	 * @skb: pointer to skb
+	 * @len: len to make read/writeable
+	 * Return: 0 on success or negative error
+	 */
+	BPF_FUNC_skb_pull_data,
+
+	/**
+	 * bpf_csum_update(skb, csum)
+	 * Adds csum into skb->csum in case of CHECKSUM_COMPLETE.
+	 * @skb: pointer to skb
+	 * @csum: csum to add
+	 * Return: csum on success or negative error
+	 */
+	BPF_FUNC_csum_update,
+
+	/**
+	 * bpf_set_hash_invalid(skb)
+	 * Invalidate current skb>hash.
+	 * @skb: pointer to skb
+	 */
+	BPF_FUNC_set_hash_invalid,
+
  	__BPF_FUNC_MAX_ID,
  };

Eric Leblond

author

2016-10-16 21:21 UTC #8 lore raw

PATCH 4/8 tools lib bpf: export function to set type

In reply to Eric Leblond

Current API was not allowing the user to set a type like socket filter. To avoid a setter function for each type, the patch simply exports a set function that takes the type in parameter.

Signed-off-by Eric Leblond <[email protected]>

---
 tools/lib/bpf/libbpf.c | 19 +++++++++----------
 tools/lib/bpf/libbpf.h |  3 +++
 2 files changed, 12 insertions(+), 10 deletions(-)

tools/lib/bpf/libbpf.c +9 ~~-10~~

index 90932f1..7cd341e 100644
@@ -1336,26 +1336,25 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n)
 	return fd;
 }
 
-static void bpf_program__set_type(struct bpf_program *prog,
-				  enum bpf_prog_type type)
+int bpf_program__set_type(struct bpf_program *prog, unsigned int type)
 {
+	if (!prog)
+		return -EINVAL;
+	if (type >= __MAX_BPF_PROG_TYPE)
+		return -EINVAL;
+
 	prog->type = type;
+	return 0;
 }
 
 int bpf_program__set_tracepoint(struct bpf_program *prog)
 {
-	if (!prog)
-		return -EINVAL;
-	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
-	return 0;
+	return bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
 }
 
 int bpf_program__set_kprobe(struct bpf_program *prog)
 {
-	if (!prog)
-		return -EINVAL;
-	bpf_program__set_type(prog, BPF_PROG_TYPE_KPROBE);
-	return 0;
+	return bpf_program__set_type(prog, BPF_PROG_TYPE_KPROBE);
 }
 
 static bool bpf_program__is_type(struct bpf_program *prog,

tools/lib/bpf/libbpf.h +3 -0

index e40c8d3..a18783b 100644
@@ -173,6 +173,9 @@ int bpf_program__set_kprobe(struct bpf_program *prog);
 bool bpf_program__is_tracepoint(struct bpf_program *prog);
 bool bpf_program__is_kprobe(struct bpf_program *prog);
 
+int bpf_program__set_type(struct bpf_program *prog,
+				  unsigned int type);
+
 /*
  * We don't need __attribute__((packed)) now since it is
  * unnecessary for 'bpf_map_def' because they are all aligned.

Signature · 1 line

2.9.3

Wangnan (F)

2016-10-17 01:56 UTC #9 lore raw

Re: [PATCH 4/8] tools lib bpf: export function to set type

Quoted 55 lines · Eric Leblond

Current API was not allowing the user to set a type like socket filter. To avoid a setter function for each type, the patch simply exports a set function that takes the type in parameter.

Signed-off-by Eric Leblond <[email protected]>

---
  tools/lib/bpf/libbpf.c | 19 +++++++++----------
  tools/lib/bpf/libbpf.h |  3 +++
  2 files changed, 12 insertions(+), 10 deletions(-)

tools/lib/bpf/libbpf.c +9 ~~-10~~

index 90932f1..7cd341e 100644
@@ -1336,26 +1336,25 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n)
  	return fd;
  }
 
-static void bpf_program__set_type(struct bpf_program *prog,
-				  enum bpf_prog_type type)
+int bpf_program__set_type(struct bpf_program *prog, unsigned int type)
  {
+	if (!prog)
+		return -EINVAL;
+	if (type >= __MAX_BPF_PROG_TYPE)
+		return -EINVAL;
+
  	prog->type = type;
+	return 0;
  }
 
  int bpf_program__set_tracepoint(struct bpf_program *prog)
  {
-	if (!prog)
-		return -EINVAL;
-	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
-	return 0;
+	return bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
  }
 
  int bpf_program__set_kprobe(struct bpf_program *prog)
  {
-	if (!prog)
-		return -EINVAL;
-	bpf_program__set_type(prog, BPF_PROG_TYPE_KPROBE);
-	return 0;
+	return bpf_program__set_type(prog, BPF_PROG_TYPE_KPROBE);
  }
 
  static bool bpf_program__is_type(struct bpf_program *prog,

tools/lib/bpf/libbpf.h +0 -0

index e40c8d3..a18783b 100644

tools/lib/bpf/libbpf.h int bpf_program__set_kprobe(struct bpf_program *prog); · line 178

Quoted 6 lines

tools/lib/bpf/libbpf.h +3 -0

@@ -173,6 +173,9 @@ int bpf_program__set_kprobe(struct bpf_program *prog);
  bool bpf_program__is_tracepoint(struct bpf_program *prog);
  bool bpf_program__is_kprobe(struct bpf_program *prog);
 
+int bpf_program__set_type(struct bpf_program *prog,
+				  unsigned int type);
+

Although you don't include uapi/linux/bpf.h in this patch, logically you add this dependency.

Please continously add bpf_program__set_socket_filter() and bpf_program__is_socket_filter() like what we do for tracepoint. This way libbpf.h is indenpendent from kernel header.

We can use macro in both .h and .c.

Thank you.

Eric Leblond

author

2016-10-16 21:21 UTC #10 lore raw

PATCH 5/8 tools lib bpf: add missing functions

In reply to Eric Leblond

Some functions were missing in the library to be able to use it in the case where the userspace is handling the maps in kernel.

The patch also renames functions to have a homogeneous naming convention.

Signed-off-by Eric Leblond <[email protected]>

---
 tools/lib/bpf/bpf.c    | 35 ++++++++++++++++++++++++++++++++++-
 tools/lib/bpf/bpf.h    |  2 --
 tools/lib/bpf/libbpf.h |  5 +++++
 3 files changed, 39 insertions(+), 3 deletions(-)

tools/lib/bpf/bpf.c +34 -1

index 4212ed6..c0e07bd 100644
@@ -25,6 +25,7 @@
 #include <asm/unistd.h>
 #include <linux/bpf.h>
 #include "bpf.h"
+#include "libbpf.h"
 
 /*
  * When building perf, unistd.h is overrided. __NR_bpf is

@@ -97,7 +98,7 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
 	return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
 }
 
-int bpf_map_update_elem(int fd, void *key, void *value,
+int bpf_map__update_elem(int fd, void *key, void *value,
 			u64 flags)
 {
 	union bpf_attr attr;

@@ -110,3 +111,35 @@ int bpf_map_update_elem(int fd, void *key, void *value,
 
 	return sys_bpf(BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr));
 }
+
+int bpf_map__lookup_elem(int fd, void *key, void *value)
+{
+	union bpf_attr attr = {
+		.map_fd = fd,
+		.key = ptr_to_u64(key),
+		.value = ptr_to_u64(value),
+	};
+
+	return sys_bpf(BPF_MAP_LOOKUP_ELEM, &attr, sizeof(attr));
+}
+
+int bpf_map__delete_elem(int fd, void *key)
+{
+	union bpf_attr attr = {
+		.map_fd = fd,
+		.key = ptr_to_u64(key),
+	};
+
+	return sys_bpf(BPF_MAP_DELETE_ELEM, &attr, sizeof(attr));
+}
+
+int bpf_map__get_next_key(int fd, void *key, void *next_key)
+{
+	union bpf_attr attr = {
+		.map_fd = fd,
+		.key = ptr_to_u64(key),
+		.next_key = ptr_to_u64(next_key),
+	};
+
+	return sys_bpf(BPF_MAP_GET_NEXT_KEY, &attr, sizeof(attr));
+}

tools/lib/bpf/bpf.h +0 -2

index e8ba540..5ca834a 100644
@@ -33,6 +33,4 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
 		     u32 kern_version, char *log_buf,
 		     size_t log_buf_sz);
 
-int bpf_map_update_elem(int fd, void *key, void *value,
-			u64 flags);
 #endif

tools/lib/bpf/libbpf.h +5 -0

index a18783b..dfb46d0 100644
@@ -207,6 +207,11 @@ bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
 int bpf_map__fd(struct bpf_map *map);
 const struct bpf_map_def *bpf_map__def(struct bpf_map *map);
 const char *bpf_map__name(struct bpf_map *map);
+int bpf_map__update_elem(int fd, void *key, void *value,
+			uint64_t flags);
+int bpf_map__lookup_elem(int fd, void *key, void *value);
+int bpf_map__delete_elem(int fd, void *key);
+int bpf_map__get_next_key(int fd, void *key, void *next_key);
 
 typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
 int bpf_map__set_priv(struct bpf_map *map, void *priv,

Signature · 1 line

2.9.3

Wangnan (F)

2016-10-17 02:18 UTC #11 lore raw

Re: [PATCH 5/8] tools lib bpf: add missing functions

Quoted 26 lines · Eric Leblond

Some functions were missing in the library to be able to use it in the case where the userspace is handling the maps in kernel.
The patch also renames functions to have a homogeneous naming convention.
Signed-off-by Eric Leblond <[email protected]>
---
  tools/lib/bpf/bpf.c    | 35 ++++++++++++++++++++++++++++++++++-
  tools/lib/bpf/bpf.h    |  2 --
  tools/lib/bpf/libbpf.h |  5 +++++
  3 files changed, 39 insertions(+), 3 deletions(-)
tools/lib/bpf/bpf.c +1 -0
index 4212ed6..c0e07bd 100644
@@ -25,6 +25,7 @@
  #include <asm/unistd.h>
  #include <linux/bpf.h>
  #include "bpf.h"
+#include "libbpf.h"
 
  /*
   * When building perf, unistd.h is overrided. __NR_bpf is

tools/lib/bpf/bpf.c int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns, · line 102

Quoted 6 lines

tools/lib/bpf/bpf.c +1 -1

@@ -97,7 +98,7 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
  	return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
  }
 
-int bpf_map_update_elem(int fd, void *key, void *value,
+int bpf_map__update_elem(int fd, void *key, void *value,
  			u64 flags)

Please don't use '__' style API here. It is easily be confused with bpf_map__*() in libbpf.h. They are APIs at different level.

bpf_map__*() are APIs for 'struct bpf_map's, they are object introduced by libbpf, defined in libbpf.h. bpf_map_*() APIs operate on fd, they are objects defined by kernel. bpf_map_*() APIs are declared in bpf.h.

In libbpf, bpf.h directly operates on kernel objects (fd), APIs in it are named bpf_map_*(); libbpf.h operates on 'struct bpf_map' object, APIs in it are named using bpf_map__*(). libbpf.h and bpf.h are independent with each other.

Quoted 54 lines

tools/lib/bpf/bpf.c +32 -0

  {
  	union bpf_attr attr;

@@ -110,3 +111,35 @@ int bpf_map_update_elem(int fd, void *key, void *value,
 
  	return sys_bpf(BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr));
  }
+
+int bpf_map__lookup_elem(int fd, void *key, void *value)
+{
+	union bpf_attr attr = {
+		.map_fd = fd,
+		.key = ptr_to_u64(key),
+		.value = ptr_to_u64(value),
+	};
+
+	return sys_bpf(BPF_MAP_LOOKUP_ELEM, &attr, sizeof(attr));
+}
+
+int bpf_map__delete_elem(int fd, void *key)
+{
+	union bpf_attr attr = {
+		.map_fd = fd,
+		.key = ptr_to_u64(key),
+	};
+
+	return sys_bpf(BPF_MAP_DELETE_ELEM, &attr, sizeof(attr));
+}
+
+int bpf_map__get_next_key(int fd, void *key, void *next_key)
+{
+	union bpf_attr attr = {
+		.map_fd = fd,
+		.key = ptr_to_u64(key),
+		.next_key = ptr_to_u64(next_key),
+	};
+
+	return sys_bpf(BPF_MAP_GET_NEXT_KEY, &attr, sizeof(attr));
+}

tools/lib/bpf/bpf.h +0 -2

index e8ba540..5ca834a 100644
@@ -33,6 +33,4 @@ int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
  		     u32 kern_version, char *log_buf,
  		     size_t log_buf_sz);
 
-int bpf_map_update_elem(int fd, void *key, void *value,
-			u64 flags);
  #endif

tools/lib/bpf/libbpf.h +0 -0

index a18783b..dfb46d0 100644

tools/lib/bpf/libbpf.h bpf_map__next(struct bpf_map *map, struct bpf_object *obj); · line 214

Quoted 8 lines

tools/lib/bpf/libbpf.h +5 -0

@@ -207,6 +207,11 @@ bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
  int bpf_map__fd(struct bpf_map *map);
  const struct bpf_map_def *bpf_map__def(struct bpf_map *map);
  const char *bpf_map__name(struct bpf_map *map);
+int bpf_map__update_elem(int fd, void *key, void *value,
+			uint64_t flags);
+int bpf_map__lookup_elem(int fd, void *key, void *value);
+int bpf_map__delete_elem(int fd, void *key);
+int bpf_map__get_next_key(int fd, void *key, void *next_key);

As what we have discussed, the newly introduced functions should be added in bpf.h.

Thank you.

Eric Leblond

author

2016-10-16 21:21 UTC #12 lore raw

PATCH 6/8 tools lib bpf: improve warning

In reply to Eric Leblond

Signed-off-by Eric Leblond <[email protected]>

---
 tools/lib/bpf/libbpf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index 7cd341e..1fe4532 100644
@@ -802,7 +802,8 @@ bpf_object__create_maps(struct bpf_object *obj)
 			size_t j;
 			int err = *pfd;
 
-			pr_warning("failed to create map: %s\n",
+			pr_warning("failed to create map (name: '%s'): %s\n",
+				   obj->maps[i].name,
 				   strerror(errno));
 			for (j = 0; j < i; j++)
 				zclose(obj->maps[j].fd);

Signature · 1 line

2.9.3

Wangnan (F)

2016-10-17 02:17 UTC #13 lore raw

Re: [PATCH 6/8] tools lib bpf: improve warning

Quoted 1 line · Eric Leblond

Signed-off-by Eric Leblond <[email protected]>

Please add some commit messages.

Thank you.

Quoted 18 lines

---
  tools/lib/bpf/libbpf.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

tools/lib/bpf/libbpf.c +2 -1

index 7cd341e..1fe4532 100644
@@ -802,7 +802,8 @@ bpf_object__create_maps(struct bpf_object *obj)
  			size_t j;
  			int err = *pfd;
 
-			pr_warning("failed to create map: %s\n",
+			pr_warning("failed to create map (name: '%s'): %s\n",
+				   obj->maps[i].name,
  				   strerror(errno));
  			for (j = 0; j < i; j++)
  				zclose(obj->maps[j].fd);

Eric Leblond

author

2016-10-16 21:21 UTC #14 lore raw

PATCH 7/8 tools lib bpf: fix maps resolution

In reply to Eric Leblond

It is not correct to assimilate the elf data of the maps section to an array of map definition. In fact the sizes differ. The offset provided in the symbol section has to be used instead.

This patch fixes a bug causing a elf with two maps not to load correctly.

Signed-off-by Eric Leblond <[email protected]>

---
 tools/lib/bpf/libbpf.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

tools/lib/bpf/libbpf.c +35 ~~-15~~

index 1fe4532..f72628b 100644
@@ -186,6 +186,7 @@ struct bpf_program {
 struct bpf_map {
 	int fd;
 	char *name;
+	size_t offset;
 	struct bpf_map_def def;
 	void *priv;
 	bpf_map_clear_priv_t clear_priv;

@@ -529,13 +530,6 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
 
 	pr_debug("maps in %s: %zd bytes\n", obj->path, size);
 
-	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
-	if (!obj->maps) {
-		pr_warning("alloc maps for object failed\n");
-		return -ENOMEM;
-	}
-	obj->nr_maps = nr_maps;
-
 	for (i = 0; i < nr_maps; i++) {
 		struct bpf_map_def *def = &obj->maps[i].def;

@@ -547,23 +541,42 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
 		obj->maps[i].fd = -1;
 
 		/* Save map definition into obj->maps */
-		*def = ((struct bpf_map_def *)data)[i];
+		*def = *(struct bpf_map_def *)(data + obj->maps[i].offset);
 	}
 	return 0;
 }
 
 static int
-bpf_object__init_maps_name(struct bpf_object *obj)
+bpf_object__init_maps_symbol(struct bpf_object *obj)
 {
 	int i;
+	int nr_maps = 0;
 	Elf_Data *symbols = obj->efile.symbols;
+	size_t map_idx = 0;
 
 	if (!symbols || obj->efile.maps_shndx < 0)
 		return -EINVAL;
 
+	/* get the number of maps */
+	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
+		GElf_Sym sym;
+
+		if (!gelf_getsym(symbols, i, &sym))
+			continue;
+		if (sym.st_shndx != obj->efile.maps_shndx)
+			continue;
+		nr_maps++;
+	}
+
+	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
+	if (!obj->maps) {
+		pr_warning("alloc maps for object failed\n");
+		return -ENOMEM;
+	}
+	obj->nr_maps = nr_maps;
+
 	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
 		GElf_Sym sym;
-		size_t map_idx;
 		const char *map_name;
 
 		if (!gelf_getsym(symbols, i, &sym))

@@ -574,12 +587,12 @@ bpf_object__init_maps_name(struct bpf_object *obj)
 		map_name = elf_strptr(obj->efile.elf,
 				      obj->efile.strtabidx,
 				      sym.st_name);
-		map_idx = sym.st_value / sizeof(struct bpf_map_def);
 		if (map_idx >= obj->nr_maps) {
 			pr_warning("index of map \"%s\" is buggy: %zu > %zu\n",
 				   map_name, map_idx, obj->nr_maps);
 			continue;
 		}
+		obj->maps[map_idx].offset = sym.st_value;
 		obj->maps[map_idx].name = strdup(map_name);
 		if (!obj->maps[map_idx].name) {
 			pr_warning("failed to alloc map name\n");

@@ -587,6 +600,7 @@ bpf_object__init_maps_name(struct bpf_object *obj)
 		}
 		pr_debug("map %zu is \"%s\"\n", map_idx,
 			 obj->maps[map_idx].name);
+		map_idx++;
 	}
 	return 0;
 }

@@ -647,8 +661,6 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 							data->d_buf,
 							data->d_size);
 		else if (strcmp(name, "maps") == 0) {
-			err = bpf_object__init_maps(obj, data->d_buf,
-						    data->d_size);
 			obj->efile.maps_shndx = idx;
 		} else if (sh.sh_type == SHT_SYMTAB) {
 			if (obj->efile.symbols) {

@@ -698,8 +710,16 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 		pr_warning("Corrupted ELF file: index of strtab invalid\n");
 		return LIBBPF_ERRNO__FORMAT;
 	}
-	if (obj->efile.maps_shndx >= 0)
-		err = bpf_object__init_maps_name(obj);
+	if (obj->efile.maps_shndx >= 0) {
+		Elf_Data *data;
+		err = bpf_object__init_maps_symbol(obj);
+		if (err)
+			goto out;
+
+		scn = elf_getscn(elf, obj->efile.maps_shndx);
+		data = elf_getdata(scn, 0);
+		err = bpf_object__init_maps(obj, data->d_buf, data->d_size);
+	}
 out:
 	return err;
 }

Signature · 1 line

2.9.3

Wangnan (F)

2016-10-17 02:23 UTC #15 lore raw

Re: [PATCH 7/8] tools lib bpf: fix maps resolution

Quoted 6 lines · Eric Leblond

It is not correct to assimilate the elf data of the maps section to an array of map definition. In fact the sizes differ. The offset provided in the symbol section has to be used instead.
This patch fixes a bug causing a elf with two maps not to load correctly.

Could you please give an example so we can understand why section 'maps' is not an array?

Thank you.

Wangnan (F)

2016-11-07 18:23 UTC #16 lore raw

Re: [PATCH 7/8] tools lib bpf: fix maps resolution

In reply to Eric Leblond

Hi Eric,

Are you still working in this patch set?

Now I know why maps section is not a simple array
from a patch set from Joe Stringer:

https://www.mail-archive.com/[email protected]/msg135088.html

So I think this patch is really useful.

Are you going to resend the whole patch set? If not, let me collect this patch 7/8 into my local code base and send to Arnaldo with my other patches.

Thank you.

Quoted 134 lines · Eric Leblond

It is not correct to assimilate the elf data of the maps section to an array of map definition. In fact the sizes differ. The offset provided in the symbol section has to be used instead.

This patch fixes a bug causing a elf with two maps not to load correctly.

Signed-off-by Eric Leblond <[email protected]>

---
  tools/lib/bpf/libbpf.c | 50 +++++++++++++++++++++++++++++++++++---------------
  1 file changed, 35 insertions(+), 15 deletions(-)

tools/lib/bpf/libbpf.c +35 ~~-15~~

index 1fe4532..f72628b 100644
@@ -186,6 +186,7 @@ struct bpf_program {
  struct bpf_map {
  	int fd;
  	char *name;
+	size_t offset;
  	struct bpf_map_def def;
  	void *priv;
  	bpf_map_clear_priv_t clear_priv;

@@ -529,13 +530,6 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
 
  	pr_debug("maps in %s: %zd bytes\n", obj->path, size);
 
-	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
-	if (!obj->maps) {
-		pr_warning("alloc maps for object failed\n");
-		return -ENOMEM;
-	}
-	obj->nr_maps = nr_maps;
-
  	for (i = 0; i < nr_maps; i++) {
  		struct bpf_map_def *def = &obj->maps[i].def;

@@ -547,23 +541,42 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
  		obj->maps[i].fd = -1;
 
  		/* Save map definition into obj->maps */
-		*def = ((struct bpf_map_def *)data)[i];
+		*def = *(struct bpf_map_def *)(data + obj->maps[i].offset);
  	}
  	return 0;
  }
 
  static int
-bpf_object__init_maps_name(struct bpf_object *obj)
+bpf_object__init_maps_symbol(struct bpf_object *obj)
  {
  	int i;
+	int nr_maps = 0;
  	Elf_Data *symbols = obj->efile.symbols;
+	size_t map_idx = 0;
 
  	if (!symbols || obj->efile.maps_shndx < 0)
  		return -EINVAL;
 
+	/* get the number of maps */
+	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
+		GElf_Sym sym;
+
+		if (!gelf_getsym(symbols, i, &sym))
+			continue;
+		if (sym.st_shndx != obj->efile.maps_shndx)
+			continue;
+		nr_maps++;
+	}
+
+	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
+	if (!obj->maps) {
+		pr_warning("alloc maps for object failed\n");
+		return -ENOMEM;
+	}
+	obj->nr_maps = nr_maps;
+
  	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
  		GElf_Sym sym;
-		size_t map_idx;
  		const char *map_name;
 
  		if (!gelf_getsym(symbols, i, &sym))

@@ -574,12 +587,12 @@ bpf_object__init_maps_name(struct bpf_object *obj)
  		map_name = elf_strptr(obj->efile.elf,
  				      obj->efile.strtabidx,
  				      sym.st_name);
-		map_idx = sym.st_value / sizeof(struct bpf_map_def);
  		if (map_idx >= obj->nr_maps) {
  			pr_warning("index of map \"%s\" is buggy: %zu > %zu\n",
  				   map_name, map_idx, obj->nr_maps);
  			continue;
  		}
+		obj->maps[map_idx].offset = sym.st_value;
  		obj->maps[map_idx].name = strdup(map_name);
  		if (!obj->maps[map_idx].name) {
  			pr_warning("failed to alloc map name\n");

@@ -587,6 +600,7 @@ bpf_object__init_maps_name(struct bpf_object *obj)
  		}
  		pr_debug("map %zu is \"%s\"\n", map_idx,
  			 obj->maps[map_idx].name);
+		map_idx++;
  	}
  	return 0;
  }

@@ -647,8 +661,6 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
  							data->d_buf,
  							data->d_size);
  		else if (strcmp(name, "maps") == 0) {
-			err = bpf_object__init_maps(obj, data->d_buf,
-						    data->d_size);
  			obj->efile.maps_shndx = idx;
  		} else if (sh.sh_type == SHT_SYMTAB) {
  			if (obj->efile.symbols) {

@@ -698,8 +710,16 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
  		pr_warning("Corrupted ELF file: index of strtab invalid\n");
  		return LIBBPF_ERRNO__FORMAT;
  	}
-	if (obj->efile.maps_shndx >= 0)
-		err = bpf_object__init_maps_name(obj);
+	if (obj->efile.maps_shndx >= 0) {
+		Elf_Data *data;
+		err = bpf_object__init_maps_symbol(obj);
+		if (err)
+			goto out;
+
+		scn = elf_getscn(elf, obj->efile.maps_shndx);
+		data = elf_getdata(scn, 0);
+		err = bpf_object__init_maps(obj, data->d_buf, data->d_size);
+	}
  out:
  	return err;
  }

Eric Leblond

author

2016-11-07 18:40 UTC #17 lore raw

Re: [PATCH 7/8] tools lib bpf: fix maps resolution

Hi,

Quoted 3 lines · Wangnan (F)

Hi Eric,
Are you still working in this patch set?

Sorry to lag on this, I've been taken by a series of other projects. I did not yet reworked it yet but I was planning to do a bit on it this week.

Quoted 11 lines

Now I know why maps section is not a simple array
from a patch set from Joe Stringer:
https://www.mail-archive.com/[email protected]/msg135088.html
So I think this patch is really useful.
Are you going to resend the whole patch set? If not, let me collect this patch 7/8 into my local code base and send to Arnaldo with my other patches.

If ok with you, I propose that you collect patch 7/8 it you have no news from me on Friday. If an issue for you, just collect it now and I will synchronize with updated code when resending my patchset.

BR,

Signature · 2 lines

Eric Leblond <[email protected]>
Blog: https://home.regit.org/

Wangnan (F)

2016-11-08 22:09 UTC #18 lore raw

Re: [PATCH 7/8] tools lib bpf: fix maps resolution

In reply to Eric Leblond

Hi Eric,

During testing this patch I find a segfault, please see inline comment.

In addition, since both the BPF map array and map names should be done after symbol table is collected, merging bpf_object__init_maps and bpf_object__init_maps_name would be a good practice, making code simpler.

So I prepare a new patch. Please have a look at:

http://lkml.kernel.org/g/[email protected]

New version ensure not crashing in any case user provides a corrupted maps section, including array of bpf maps, maps with different definition structures and very short map definition.

Thank you.

Quoted 39 lines · Eric Leblond

It is not correct to assimilate the elf data of the maps section to an array of map definition. In fact the sizes differ. The offset provided in the symbol section has to be used instead.

This patch fixes a bug causing a elf with two maps not to load correctly.

Signed-off-by Eric Leblond <[email protected]>

---
  tools/lib/bpf/libbpf.c | 50 +++++++++++++++++++++++++++++++++++---------------
  1 file changed, 35 insertions(+), 15 deletions(-)

tools/lib/bpf/libbpf.c +1 -7

index 1fe4532..f72628b 100644
@@ -186,6 +186,7 @@ struct bpf_program {
  struct bpf_map {
  	int fd;
  	char *name;
+	size_t offset;
  	struct bpf_map_def def;
  	void *priv;
  	bpf_map_clear_priv_t clear_priv;

@@ -529,13 +530,6 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
 
  	pr_debug("maps in %s: %zd bytes\n", obj->path, size);
 
-	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
-	if (!obj->maps) {
-		pr_warning("alloc maps for object failed\n");
-		return -ENOMEM;
-	}
-	obj->nr_maps = nr_maps;
-
  	for (i = 0; i < nr_maps; i++) {
  		struct bpf_map_def *def = &obj->maps[i].def;

tools/lib/bpf/libbpf.c bpf_object__init_maps(struct bpf_object *obj, void *data, · line 545

Quoted 6 lines

tools/lib/bpf/libbpf.c +1 -1

@@ -547,23 +541,42 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
  		obj->maps[i].fd = -1;
 
  		/* Save map definition into obj->maps */
-		*def = ((struct bpf_map_def *)data)[i];
+		*def = *(struct bpf_map_def *)(data + obj->maps[i].offset);
  	}

Here, nr_maps is still size / sizeof(struct bpf_map_def), so obj->maps[i] can be invalid.

Quoted 89 lines

tools/lib/bpf/libbpf.c +33 -7

  	return 0;
  }
 
  static int
-bpf_object__init_maps_name(struct bpf_object *obj)
+bpf_object__init_maps_symbol(struct bpf_object *obj)
  {
  	int i;
+	int nr_maps = 0;
  	Elf_Data *symbols = obj->efile.symbols;
+	size_t map_idx = 0;
 
  	if (!symbols || obj->efile.maps_shndx < 0)
  		return -EINVAL;
 
+	/* get the number of maps */
+	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
+		GElf_Sym sym;
+
+		if (!gelf_getsym(symbols, i, &sym))
+			continue;
+		if (sym.st_shndx != obj->efile.maps_shndx)
+			continue;
+		nr_maps++;
+	}
+
+	obj->maps = calloc(nr_maps, sizeof(obj->maps[0]));
+	if (!obj->maps) {
+		pr_warning("alloc maps for object failed\n");
+		return -ENOMEM;
+	}
+	obj->nr_maps = nr_maps;
+
  	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
  		GElf_Sym sym;
-		size_t map_idx;
  		const char *map_name;
 
  		if (!gelf_getsym(symbols, i, &sym))

@@ -574,12 +587,12 @@ bpf_object__init_maps_name(struct bpf_object *obj)
  		map_name = elf_strptr(obj->efile.elf,
  				      obj->efile.strtabidx,
  				      sym.st_name);
-		map_idx = sym.st_value / sizeof(struct bpf_map_def);
  		if (map_idx >= obj->nr_maps) {
  			pr_warning("index of map \"%s\" is buggy: %zu > %zu\n",
  				   map_name, map_idx, obj->nr_maps);
  			continue;
  		}
+		obj->maps[map_idx].offset = sym.st_value;
  		obj->maps[map_idx].name = strdup(map_name);
  		if (!obj->maps[map_idx].name) {
  			pr_warning("failed to alloc map name\n");

@@ -587,6 +600,7 @@ bpf_object__init_maps_name(struct bpf_object *obj)
  		}
  		pr_debug("map %zu is \"%s\"\n", map_idx,
  			 obj->maps[map_idx].name);
+		map_idx++;
  	}
  	return 0;
  }

@@ -647,8 +661,6 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
  							data->d_buf,
  							data->d_size);
  		else if (strcmp(name, "maps") == 0) {
-			err = bpf_object__init_maps(obj, data->d_buf,
-						    data->d_size);
  			obj->efile.maps_shndx = idx;
  		} else if (sh.sh_type == SHT_SYMTAB) {
  			if (obj->efile.symbols) {

@@ -698,8 +710,16 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
  		pr_warning("Corrupted ELF file: index of strtab invalid\n");
  		return LIBBPF_ERRNO__FORMAT;
  	}
-	if (obj->efile.maps_shndx >= 0)
-		err = bpf_object__init_maps_name(obj);
+	if (obj->efile.maps_shndx >= 0) {
+		Elf_Data *data;
+		err = bpf_object__init_maps_symbol(obj);
+		if (err)
+			goto out;
+
+		scn = elf_getscn(elf, obj->efile.maps_shndx);
+		data = elf_getdata(scn, 0);
+		err = bpf_object__init_maps(obj, data->d_buf, data->d_size);
+	}
  out:
  	return err;
  }

Eric Leblond

author

2016-10-16 21:21 UTC #19 lore raw

PATCH 8/8 tools lib bpf: install header file

In reply to Eric Leblond

Makefile was not installing the header file of the library and a manual copy was needed to have a usable library on the system.

Signed-off-by Eric Leblond <[email protected]>

---
 tools/lib/bpf/Makefile | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

tools/lib/bpf/Makefile +9 -2

index 62d89d5..9525956 100644
@@ -47,6 +47,7 @@ endif
 
 prefix ?= /usr/local
 libdir = $(prefix)/$(libdir_relative)
+includedir = $(prefix)/include/bpf
 man_dir = $(prefix)/share/man
 man_dir_SQ = '$(subst ','\'',$(man_dir))'

@@ -87,14 +88,16 @@ include $(FEATURES_DUMP)
 endif
 endif
 
-export prefix libdir src obj
+export prefix libdir includedir src obj
 
 # Shell quotes
 libdir_SQ = $(subst ','\'',$(libdir))
 libdir_relative_SQ = $(subst ','\'',$(libdir_relative))
+includedir_SQ = $(subst ','\'',$(includedir))
 plugin_dir_SQ = $(subst ','\'',$(plugin_dir))
 
 LIB_FILE = libbpf.a libbpf.so
+HEADER_FILE = libbpf.h
 
 VERSION		= $(BPF_VERSION)
 PATCHLEVEL	= $(BPF_PATCHLEVEL)

@@ -189,7 +192,11 @@ install_lib: all_cmd
 	$(call QUIET_INSTALL, $(LIB_FILE)) \
 		$(call do_install,$(LIB_FILE),$(libdir_SQ))
 
-install: install_lib
+install_header: all_cmd
+	$(call QUIET_INSTALL, $(HEADER_FILE)) \
+		$(call do_install,$(HEADER_FILE),$(includedir_SQ))
+
+install: install_lib install_header
 
 ### Cleaning rules

Signature · 1 line

2.9.3