* [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers
@ 2019-10-07 3:07 Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs Andrii Nakryiko
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-10-07 3:07 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, quentin.monnet
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
This patch set adds ability to auto-generate list of BPF helper definitions.
It relies on existing scripts/bpf_helpers_doc.py and include/uapi/linux/bpf.h
having a well-defined set of comments. bpf_helper_defs.h contains all BPF
helper signatures which stay in sync with latest bpf.h UAPI. This
auto-generated header is included from bpf_helpers.h, while all previously
hand-written BPF helper definitions are simultaneously removed in patch #3.
The end result is less manually maintained and redundant boilerplate code,
while also more consistent and well-documented set of BPF helpers. Generated
helper definitions are completely independent from a specific bpf.h on
a target system, because it doesn't use BPF_FUNC_xxx enums.
v3->v4:
- instead of libbpf's Makefile, integrate with selftest/bpf's Makefile (Alexei);
v2->v3:
- delete bpf_helper_defs.h properly (Alexei);
v1->v2:
- add bpf_helper_defs.h to .gitignore and `make clean` (Alexei).
Andrii Nakryiko (3):
uapi/bpf: fix helper docs
scripts/bpf: teach bpf_helpers_doc.py to dump BPF helper definitions
libbpf: auto-generate list of BPF helper definitions
include/uapi/linux/bpf.h | 32 +--
scripts/bpf_helpers_doc.py | 155 ++++++++++++-
tools/include/uapi/linux/bpf.h | 32 +--
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 8 +-
tools/testing/selftests/bpf/bpf_helpers.h | 264 +---------------------
6 files changed, 195 insertions(+), 297 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs
2019-10-07 3:07 [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Andrii Nakryiko
@ 2019-10-07 3:07 ` Andrii Nakryiko
2019-10-07 9:43 ` Daniel Borkmann
2019-10-07 3:07 ` [PATCH v4 bpf-next 2/3] scripts/bpf: teach bpf_helpers_doc.py to dump BPF helper definitions Andrii Nakryiko
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2019-10-07 3:07 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, quentin.monnet
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Various small fixes to BPF helper documentation comments, enabling
automatic header generation with a list of BPF helpers.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
tools/include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
2 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 77c6be96d676..a65c3b0c6935 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -794,7 +794,7 @@ union bpf_attr {
* A 64-bit integer containing the current GID and UID, and
* created as such: *current_gid* **<< 32 \|** *current_uid*.
*
- * int bpf_get_current_comm(char *buf, u32 size_of_buf)
+ * int bpf_get_current_comm(void *buf, u32 size_of_buf)
* Description
* Copy the **comm** attribute of the current task into *buf* of
* *size_of_buf*. The **comm** attribute contains the name of
@@ -1023,7 +1023,7 @@ union bpf_attr {
* The realm of the route for the packet associated to *skb*, or 0
* if none was found.
*
- * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
+ * int bpf_perf_event_output(void *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
* Description
* Write raw *data* blob into a special BPF perf event held by
* *map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
@@ -1068,7 +1068,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len)
+ * int bpf_skb_load_bytes(const void *skb, u32 offset, void *to, u32 len)
* Description
* This helper was provided as an easy way to load data from a
* packet. It can be used to load *len* bytes from *offset* from
@@ -1085,7 +1085,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
+ * int bpf_get_stackid(void *ctx, struct bpf_map *map, u64 flags)
* Description
* Walk a user or a kernel stack and return its id. To achieve
* this, the helper needs *ctx*, which is a pointer to the context
@@ -1154,7 +1154,7 @@ union bpf_attr {
* The checksum result, or a negative error code in case of
* failure.
*
- * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
+ * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)
* Description
* Retrieve tunnel options metadata for the packet associated to
* *skb*, and store the raw tunnel option data to the buffer *opt*
@@ -1172,7 +1172,7 @@ union bpf_attr {
* Return
* The size of the option data retrieved.
*
- * int bpf_skb_set_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
+ * int bpf_skb_set_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)
* Description
* Set tunnel options metadata for the packet associated to *skb*
* to the option data contained in the raw buffer *opt* of *size*.
@@ -1511,7 +1511,7 @@ union bpf_attr {
* Return
* 0
*
- * int bpf_setsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, char *optval, int optlen)
+ * int bpf_setsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, void *optval, int optlen)
* Description
* Emulate a call to **setsockopt()** on the socket associated to
* *bpf_socket*, which must be a full socket. The *level* at
@@ -1595,7 +1595,7 @@ union bpf_attr {
* Return
* **XDP_REDIRECT** on success, or **XDP_ABORTED** on error.
*
- * int bpf_sk_redirect_map(struct bpf_map *map, u32 key, u64 flags)
+ * int bpf_sk_redirect_map(struct sk_buff *skb, struct bpf_map *map, u32 key, u64 flags)
* Description
* Redirect the packet to the socket referenced by *map* (of type
* **BPF_MAP_TYPE_SOCKMAP**) at index *key*. Both ingress and
@@ -1715,7 +1715,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_getsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, char *optval, int optlen)
+ * int bpf_getsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, void *optval, int optlen)
* Description
* Emulate a call to **getsockopt()** on the socket associated to
* *bpf_socket*, which must be a full socket. The *level* at
@@ -1947,7 +1947,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_get_stack(struct pt_regs *regs, void *buf, u32 size, u64 flags)
+ * int bpf_get_stack(void *ctx, void *buf, u32 size, u64 flags)
* Description
* Return a user or a kernel stack in bpf program provided buffer.
* To achieve this, the helper needs *ctx*, which is a pointer
@@ -1980,7 +1980,7 @@ union bpf_attr {
* A non-negative value equal to or less than *size* on success,
* or a negative error in case of failure.
*
- * int bpf_skb_load_bytes_relative(const struct sk_buff *skb, u32 offset, void *to, u32 len, u32 start_header)
+ * int bpf_skb_load_bytes_relative(const void *skb, u32 offset, void *to, u32 len, u32 start_header)
* Description
* This helper is similar to **bpf_skb_load_bytes**\ () in that
* it provides an easy way to load *len* bytes from *offset*
@@ -2033,7 +2033,7 @@ union bpf_attr {
* * > 0 one of **BPF_FIB_LKUP_RET_** codes explaining why the
* packet is not forwarded or needs assist from full stack
*
- * int bpf_sock_hash_update(struct bpf_sock_ops_kern *skops, struct bpf_map *map, void *key, u64 flags)
+ * int bpf_sock_hash_update(struct bpf_sock_ops *skops, struct bpf_map *map, void *key, u64 flags)
* Description
* Add an entry to, or update a sockhash *map* referencing sockets.
* The *skops* is used as a new value for the entry associated to
@@ -2392,7 +2392,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_msg_push_data(struct sk_buff *skb, u32 start, u32 len, u64 flags)
+ * int bpf_msg_push_data(struct sk_msg_buff *msg, u32 start, u32 len, u64 flags)
* Description
* For socket policies, insert *len* bytes into *msg* at offset
* *start*.
@@ -2408,9 +2408,9 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_msg_pop_data(struct sk_msg_buff *msg, u32 start, u32 pop, u64 flags)
+ * int bpf_msg_pop_data(struct sk_msg_buff *msg, u32 start, u32 len, u64 flags)
* Description
- * Will remove *pop* bytes from a *msg* starting at byte *start*.
+ * Will remove *len* bytes from a *msg* starting at byte *start*.
* This may result in **ENOMEM** errors under certain situations if
* an allocation and copy are required due to a full ring buffer.
* However, the helper will try to avoid doing the allocation
@@ -2505,7 +2505,7 @@ union bpf_attr {
* A **struct bpf_tcp_sock** pointer on success, or **NULL** in
* case of failure.
*
- * int bpf_skb_ecn_set_ce(struct sk_buf *skb)
+ * int bpf_skb_ecn_set_ce(struct sk_buff *skb)
* Description
* Set ECN (Explicit Congestion Notification) field of IP header
* to **CE** (Congestion Encountered) if current value is **ECT**
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 77c6be96d676..a65c3b0c6935 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -794,7 +794,7 @@ union bpf_attr {
* A 64-bit integer containing the current GID and UID, and
* created as such: *current_gid* **<< 32 \|** *current_uid*.
*
- * int bpf_get_current_comm(char *buf, u32 size_of_buf)
+ * int bpf_get_current_comm(void *buf, u32 size_of_buf)
* Description
* Copy the **comm** attribute of the current task into *buf* of
* *size_of_buf*. The **comm** attribute contains the name of
@@ -1023,7 +1023,7 @@ union bpf_attr {
* The realm of the route for the packet associated to *skb*, or 0
* if none was found.
*
- * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
+ * int bpf_perf_event_output(void *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
* Description
* Write raw *data* blob into a special BPF perf event held by
* *map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
@@ -1068,7 +1068,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len)
+ * int bpf_skb_load_bytes(const void *skb, u32 offset, void *to, u32 len)
* Description
* This helper was provided as an easy way to load data from a
* packet. It can be used to load *len* bytes from *offset* from
@@ -1085,7 +1085,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
+ * int bpf_get_stackid(void *ctx, struct bpf_map *map, u64 flags)
* Description
* Walk a user or a kernel stack and return its id. To achieve
* this, the helper needs *ctx*, which is a pointer to the context
@@ -1154,7 +1154,7 @@ union bpf_attr {
* The checksum result, or a negative error code in case of
* failure.
*
- * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
+ * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)
* Description
* Retrieve tunnel options metadata for the packet associated to
* *skb*, and store the raw tunnel option data to the buffer *opt*
@@ -1172,7 +1172,7 @@ union bpf_attr {
* Return
* The size of the option data retrieved.
*
- * int bpf_skb_set_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
+ * int bpf_skb_set_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)
* Description
* Set tunnel options metadata for the packet associated to *skb*
* to the option data contained in the raw buffer *opt* of *size*.
@@ -1511,7 +1511,7 @@ union bpf_attr {
* Return
* 0
*
- * int bpf_setsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, char *optval, int optlen)
+ * int bpf_setsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, void *optval, int optlen)
* Description
* Emulate a call to **setsockopt()** on the socket associated to
* *bpf_socket*, which must be a full socket. The *level* at
@@ -1595,7 +1595,7 @@ union bpf_attr {
* Return
* **XDP_REDIRECT** on success, or **XDP_ABORTED** on error.
*
- * int bpf_sk_redirect_map(struct bpf_map *map, u32 key, u64 flags)
+ * int bpf_sk_redirect_map(struct sk_buff *skb, struct bpf_map *map, u32 key, u64 flags)
* Description
* Redirect the packet to the socket referenced by *map* (of type
* **BPF_MAP_TYPE_SOCKMAP**) at index *key*. Both ingress and
@@ -1715,7 +1715,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_getsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, char *optval, int optlen)
+ * int bpf_getsockopt(struct bpf_sock_ops *bpf_socket, int level, int optname, void *optval, int optlen)
* Description
* Emulate a call to **getsockopt()** on the socket associated to
* *bpf_socket*, which must be a full socket. The *level* at
@@ -1947,7 +1947,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_get_stack(struct pt_regs *regs, void *buf, u32 size, u64 flags)
+ * int bpf_get_stack(void *ctx, void *buf, u32 size, u64 flags)
* Description
* Return a user or a kernel stack in bpf program provided buffer.
* To achieve this, the helper needs *ctx*, which is a pointer
@@ -1980,7 +1980,7 @@ union bpf_attr {
* A non-negative value equal to or less than *size* on success,
* or a negative error in case of failure.
*
- * int bpf_skb_load_bytes_relative(const struct sk_buff *skb, u32 offset, void *to, u32 len, u32 start_header)
+ * int bpf_skb_load_bytes_relative(const void *skb, u32 offset, void *to, u32 len, u32 start_header)
* Description
* This helper is similar to **bpf_skb_load_bytes**\ () in that
* it provides an easy way to load *len* bytes from *offset*
@@ -2033,7 +2033,7 @@ union bpf_attr {
* * > 0 one of **BPF_FIB_LKUP_RET_** codes explaining why the
* packet is not forwarded or needs assist from full stack
*
- * int bpf_sock_hash_update(struct bpf_sock_ops_kern *skops, struct bpf_map *map, void *key, u64 flags)
+ * int bpf_sock_hash_update(struct bpf_sock_ops *skops, struct bpf_map *map, void *key, u64 flags)
* Description
* Add an entry to, or update a sockhash *map* referencing sockets.
* The *skops* is used as a new value for the entry associated to
@@ -2392,7 +2392,7 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_msg_push_data(struct sk_buff *skb, u32 start, u32 len, u64 flags)
+ * int bpf_msg_push_data(struct sk_msg_buff *msg, u32 start, u32 len, u64 flags)
* Description
* For socket policies, insert *len* bytes into *msg* at offset
* *start*.
@@ -2408,9 +2408,9 @@ union bpf_attr {
* Return
* 0 on success, or a negative error in case of failure.
*
- * int bpf_msg_pop_data(struct sk_msg_buff *msg, u32 start, u32 pop, u64 flags)
+ * int bpf_msg_pop_data(struct sk_msg_buff *msg, u32 start, u32 len, u64 flags)
* Description
- * Will remove *pop* bytes from a *msg* starting at byte *start*.
+ * Will remove *len* bytes from a *msg* starting at byte *start*.
* This may result in **ENOMEM** errors under certain situations if
* an allocation and copy are required due to a full ring buffer.
* However, the helper will try to avoid doing the allocation
@@ -2505,7 +2505,7 @@ union bpf_attr {
* A **struct bpf_tcp_sock** pointer on success, or **NULL** in
* case of failure.
*
- * int bpf_skb_ecn_set_ce(struct sk_buf *skb)
+ * int bpf_skb_ecn_set_ce(struct sk_buff *skb)
* Description
* Set ECN (Explicit Congestion Notification) field of IP header
* to **CE** (Congestion Encountered) if current value is **ECT**
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 bpf-next 2/3] scripts/bpf: teach bpf_helpers_doc.py to dump BPF helper definitions
2019-10-07 3:07 [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs Andrii Nakryiko
@ 2019-10-07 3:07 ` Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 3/3] libbpf: auto-generate list of " Andrii Nakryiko
2019-10-07 5:35 ` [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Alexei Starovoitov
3 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-10-07 3:07 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, quentin.monnet
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Enhance scripts/bpf_helpers_doc.py to emit C header with BPF helper
definitions (to be included from libbpf's bpf_helpers.h).
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
scripts/bpf_helpers_doc.py | 155 ++++++++++++++++++++++++++++++++++++-
1 file changed, 154 insertions(+), 1 deletion(-)
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 894cc58c1a03..15d3d83d6297 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -391,6 +391,154 @@ SEE ALSO
print('')
+class PrinterHelpers(Printer):
+ """
+ A printer for dumping collected information about helpers as C header to
+ be included from BPF program.
+ @helpers: array of Helper objects to print to standard output
+ """
+
+ type_fwds = [
+ 'struct bpf_fib_lookup',
+ 'struct bpf_perf_event_data',
+ 'struct bpf_perf_event_value',
+ 'struct bpf_sock',
+ 'struct bpf_sock_addr',
+ 'struct bpf_sock_ops',
+ 'struct bpf_sock_tuple',
+ 'struct bpf_spin_lock',
+ 'struct bpf_sysctl',
+ 'struct bpf_tcp_sock',
+ 'struct bpf_tunnel_key',
+ 'struct bpf_xfrm_state',
+ 'struct pt_regs',
+ 'struct sk_reuseport_md',
+ 'struct sockaddr',
+ 'struct tcphdr',
+
+ 'struct __sk_buff',
+ 'struct sk_msg_md',
+ 'struct xpd_md',
+ ]
+ known_types = {
+ '...',
+ 'void',
+ 'const void',
+ 'char',
+ 'const char',
+ 'int',
+ 'long',
+ 'unsigned long',
+
+ '__be16',
+ '__be32',
+ '__wsum',
+
+ 'struct bpf_fib_lookup',
+ 'struct bpf_perf_event_data',
+ 'struct bpf_perf_event_value',
+ 'struct bpf_sock',
+ 'struct bpf_sock_addr',
+ 'struct bpf_sock_ops',
+ 'struct bpf_sock_tuple',
+ 'struct bpf_spin_lock',
+ 'struct bpf_sysctl',
+ 'struct bpf_tcp_sock',
+ 'struct bpf_tunnel_key',
+ 'struct bpf_xfrm_state',
+ 'struct pt_regs',
+ 'struct sk_reuseport_md',
+ 'struct sockaddr',
+ 'struct tcphdr',
+ }
+ mapped_types = {
+ 'u8': '__u8',
+ 'u16': '__u16',
+ 'u32': '__u32',
+ 'u64': '__u64',
+ 's8': '__s8',
+ 's16': '__s16',
+ 's32': '__s32',
+ 's64': '__s64',
+ 'size_t': 'unsigned long',
+ 'struct bpf_map': 'void',
+ 'struct sk_buff': 'struct __sk_buff',
+ 'const struct sk_buff': 'const struct __sk_buff',
+ 'struct sk_msg_buff': 'struct sk_msg_md',
+ 'struct xdp_buff': 'struct xdp_md',
+ }
+
+ def print_header(self):
+ header = '''\
+/* This is auto-generated file. See bpf_helpers_doc.py for details. */
+
+/* Forward declarations of BPF structs */'''
+
+ print(header)
+ for fwd in self.type_fwds:
+ print('%s;' % fwd)
+ print('')
+
+ def print_footer(self):
+ footer = ''
+ print(footer)
+
+ def map_type(self, t):
+ if t in self.known_types:
+ return t
+ if t in self.mapped_types:
+ return self.mapped_types[t]
+ print("")
+ print("Unrecognized type '%s', please add it to known types!" % t)
+ sys.exit(1)
+
+ seen_helpers = set()
+
+ def print_one(self, helper):
+ proto = helper.proto_break_down()
+
+ if proto['name'] in self.seen_helpers:
+ return
+ self.seen_helpers.add(proto['name'])
+
+ print('/*')
+ print(" * %s" % proto['name'])
+ print(" *")
+ if (helper.desc):
+ # Do not strip all newline characters: formatted code at the end of
+ # a section must be followed by a blank line.
+ for line in re.sub('\n$', '', helper.desc, count=1).split('\n'):
+ print(' *{}{}'.format(' \t' if line else '', line))
+
+ if (helper.ret):
+ print(' *')
+ print(' * Returns')
+ for line in helper.ret.rstrip().split('\n'):
+ print(' *{}{}'.format(' \t' if line else '', line))
+
+ print(' */')
+ print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
+ proto['ret_star'], proto['name']), end='')
+ comma = ''
+ for i, a in enumerate(proto['args']):
+ t = a['type']
+ n = a['name']
+ if proto['name'] == 'bpf_get_socket_cookie' and i == 0:
+ t = 'void'
+ n = 'ctx'
+ one_arg = '{}{}'.format(comma, self.map_type(t))
+ if n:
+ if a['star']:
+ one_arg += ' {}'.format(a['star'])
+ else:
+ one_arg += ' '
+ one_arg += '{}'.format(n)
+ comma = ', '
+ print(one_arg, end='')
+
+ print(') = (void *) %d;' % len(self.seen_helpers))
+ print('')
+
###############################################################################
# If script is launched from scripts/ from kernel tree and can access
@@ -405,6 +553,8 @@ Parse eBPF header file and generate documentation for eBPF helper functions.
The RST-formatted output produced can be turned into a manual page with the
rst2man utility.
""")
+argParser.add_argument('--header', action='store_true',
+ help='generate C header file')
if (os.path.isfile(bpfh)):
argParser.add_argument('--filename', help='path to include/uapi/linux/bpf.h',
default=bpfh)
@@ -417,5 +567,8 @@ headerParser = HeaderParser(args.filename)
headerParser.run()
# Print formatted output to standard output.
-printer = PrinterRST(headerParser.helpers)
+if args.header:
+ printer = PrinterHelpers(headerParser.helpers)
+else:
+ printer = PrinterRST(headerParser.helpers)
printer.print_all()
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 bpf-next 3/3] libbpf: auto-generate list of BPF helper definitions
2019-10-07 3:07 [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 2/3] scripts/bpf: teach bpf_helpers_doc.py to dump BPF helper definitions Andrii Nakryiko
@ 2019-10-07 3:07 ` Andrii Nakryiko
2019-10-07 5:35 ` [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Alexei Starovoitov
3 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2019-10-07 3:07 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, quentin.monnet
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Get rid of list of BPF helpers in bpf_helpers.h (irony...) and
auto-generate it into bpf_helpers_defs.h, which is now included from
bpf_helpers.h.
Suggested-by: Alexei Starovoitov <ast@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 8 +-
tools/testing/selftests/bpf/bpf_helpers.h | 264 +---------------------
3 files changed, 9 insertions(+), 264 deletions(-)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 7470327edcfe..50063f66539d 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -39,3 +39,4 @@ libbpf.so.*
test_hashmap
test_btf_dump
xdping
+/bpf_helper_defs.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 294d7472dad7..b59fb4e8afaf 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -90,6 +90,10 @@ include ../lib.mk
TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
all: $(TEST_CUSTOM_PROGS)
+bpf_helper_defs.h: $(APIDIR)/linux/bpf.h
+ $(BPFDIR)/../../../scripts/bpf_helpers_doc.py --header \
+ --file $(APIDIR)/linux/bpf.h > bpf_helper_defs.h
+
$(OUTPUT)/urandom_read: $(OUTPUT)/%: %.c
$(CC) -o $@ $< -Wl,--build-id
@@ -123,7 +127,7 @@ $(OUTPUT)/test_cgroup_attach: cgroup_helpers.c
# force a rebuild of BPFOBJ when its dependencies are updated
force:
-$(BPFOBJ): force
+$(BPFOBJ): force bpf_helper_defs.h
$(MAKE) -C $(BPFDIR) OUTPUT=$(OUTPUT)/
PROBE := $(shell $(LLC) -march=bpf -mcpu=probe -filetype=null /dev/null 2>&1)
@@ -319,4 +323,4 @@ $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(ALU32_BUILD_DIR) $(BPF_GCC_BUILD_DIR) \
$(VERIFIER_TESTS_H) $(PROG_TESTS_H) $(MAP_TESTS_H) \
- feature
+ feature bpf_helper_defs.h
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 9f77cbaac01c..15152280db6f 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -2,6 +2,8 @@
#ifndef __BPF_HELPERS__
#define __BPF_HELPERS__
+#include "bpf_helper_defs.h"
+
#define __uint(name, val) int (*name)[val]
#define __type(name, val) typeof(val) *name
@@ -21,219 +23,6 @@
*/
#define SEC(NAME) __attribute__((section(NAME), used))
-/* helper functions called from eBPF programs written in C */
-static void *(*bpf_map_lookup_elem)(void *map, const void *key) =
- (void *) BPF_FUNC_map_lookup_elem;
-static int (*bpf_map_update_elem)(void *map, const void *key, const void *value,
- unsigned long long flags) =
- (void *) BPF_FUNC_map_update_elem;
-static int (*bpf_map_delete_elem)(void *map, const void *key) =
- (void *) BPF_FUNC_map_delete_elem;
-static int (*bpf_map_push_elem)(void *map, const void *value,
- unsigned long long flags) =
- (void *) BPF_FUNC_map_push_elem;
-static int (*bpf_map_pop_elem)(void *map, void *value) =
- (void *) BPF_FUNC_map_pop_elem;
-static int (*bpf_map_peek_elem)(void *map, void *value) =
- (void *) BPF_FUNC_map_peek_elem;
-static int (*bpf_probe_read)(void *dst, int size, const void *unsafe_ptr) =
- (void *) BPF_FUNC_probe_read;
-static unsigned long long (*bpf_ktime_get_ns)(void) =
- (void *) BPF_FUNC_ktime_get_ns;
-static int (*bpf_trace_printk)(const char *fmt, int fmt_size, ...) =
- (void *) BPF_FUNC_trace_printk;
-static void (*bpf_tail_call)(void *ctx, void *map, int index) =
- (void *) BPF_FUNC_tail_call;
-static unsigned long long (*bpf_get_smp_processor_id)(void) =
- (void *) BPF_FUNC_get_smp_processor_id;
-static unsigned long long (*bpf_get_current_pid_tgid)(void) =
- (void *) BPF_FUNC_get_current_pid_tgid;
-static unsigned long long (*bpf_get_current_uid_gid)(void) =
- (void *) BPF_FUNC_get_current_uid_gid;
-static int (*bpf_get_current_comm)(void *buf, int buf_size) =
- (void *) BPF_FUNC_get_current_comm;
-static unsigned long long (*bpf_perf_event_read)(void *map,
- unsigned long long flags) =
- (void *) BPF_FUNC_perf_event_read;
-static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
- (void *) BPF_FUNC_clone_redirect;
-static int (*bpf_redirect)(int ifindex, int flags) =
- (void *) BPF_FUNC_redirect;
-static int (*bpf_redirect_map)(void *map, int key, int flags) =
- (void *) BPF_FUNC_redirect_map;
-static int (*bpf_perf_event_output)(void *ctx, void *map,
- unsigned long long flags, void *data,
- int size) =
- (void *) BPF_FUNC_perf_event_output;
-static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
- (void *) BPF_FUNC_get_stackid;
-static int (*bpf_probe_write_user)(void *dst, const void *src, int size) =
- (void *) BPF_FUNC_probe_write_user;
-static int (*bpf_current_task_under_cgroup)(void *map, int index) =
- (void *) BPF_FUNC_current_task_under_cgroup;
-static int (*bpf_skb_get_tunnel_key)(void *ctx, void *key, int size, int flags) =
- (void *) BPF_FUNC_skb_get_tunnel_key;
-static int (*bpf_skb_set_tunnel_key)(void *ctx, void *key, int size, int flags) =
- (void *) BPF_FUNC_skb_set_tunnel_key;
-static int (*bpf_skb_get_tunnel_opt)(void *ctx, void *md, int size) =
- (void *) BPF_FUNC_skb_get_tunnel_opt;
-static int (*bpf_skb_set_tunnel_opt)(void *ctx, void *md, int size) =
- (void *) BPF_FUNC_skb_set_tunnel_opt;
-static unsigned long long (*bpf_get_prandom_u32)(void) =
- (void *) BPF_FUNC_get_prandom_u32;
-static int (*bpf_xdp_adjust_head)(void *ctx, int offset) =
- (void *) BPF_FUNC_xdp_adjust_head;
-static int (*bpf_xdp_adjust_meta)(void *ctx, int offset) =
- (void *) BPF_FUNC_xdp_adjust_meta;
-static int (*bpf_get_socket_cookie)(void *ctx) =
- (void *) BPF_FUNC_get_socket_cookie;
-static int (*bpf_setsockopt)(void *ctx, int level, int optname, void *optval,
- int optlen) =
- (void *) BPF_FUNC_setsockopt;
-static int (*bpf_getsockopt)(void *ctx, int level, int optname, void *optval,
- int optlen) =
- (void *) BPF_FUNC_getsockopt;
-static int (*bpf_sock_ops_cb_flags_set)(void *ctx, int flags) =
- (void *) BPF_FUNC_sock_ops_cb_flags_set;
-static int (*bpf_sk_redirect_map)(void *ctx, void *map, int key, int flags) =
- (void *) BPF_FUNC_sk_redirect_map;
-static int (*bpf_sk_redirect_hash)(void *ctx, void *map, void *key, int flags) =
- (void *) BPF_FUNC_sk_redirect_hash;
-static int (*bpf_sock_map_update)(void *map, void *key, void *value,
- unsigned long long flags) =
- (void *) BPF_FUNC_sock_map_update;
-static int (*bpf_sock_hash_update)(void *map, void *key, void *value,
- unsigned long long flags) =
- (void *) BPF_FUNC_sock_hash_update;
-static int (*bpf_perf_event_read_value)(void *map, unsigned long long flags,
- void *buf, unsigned int buf_size) =
- (void *) BPF_FUNC_perf_event_read_value;
-static int (*bpf_perf_prog_read_value)(void *ctx, void *buf,
- unsigned int buf_size) =
- (void *) BPF_FUNC_perf_prog_read_value;
-static int (*bpf_override_return)(void *ctx, unsigned long rc) =
- (void *) BPF_FUNC_override_return;
-static int (*bpf_msg_redirect_map)(void *ctx, void *map, int key, int flags) =
- (void *) BPF_FUNC_msg_redirect_map;
-static int (*bpf_msg_redirect_hash)(void *ctx,
- void *map, void *key, int flags) =
- (void *) BPF_FUNC_msg_redirect_hash;
-static int (*bpf_msg_apply_bytes)(void *ctx, int len) =
- (void *) BPF_FUNC_msg_apply_bytes;
-static int (*bpf_msg_cork_bytes)(void *ctx, int len) =
- (void *) BPF_FUNC_msg_cork_bytes;
-static int (*bpf_msg_pull_data)(void *ctx, int start, int end, int flags) =
- (void *) BPF_FUNC_msg_pull_data;
-static int (*bpf_msg_push_data)(void *ctx, int start, int end, int flags) =
- (void *) BPF_FUNC_msg_push_data;
-static int (*bpf_msg_pop_data)(void *ctx, int start, int cut, int flags) =
- (void *) BPF_FUNC_msg_pop_data;
-static int (*bpf_bind)(void *ctx, void *addr, int addr_len) =
- (void *) BPF_FUNC_bind;
-static int (*bpf_xdp_adjust_tail)(void *ctx, int offset) =
- (void *) BPF_FUNC_xdp_adjust_tail;
-static int (*bpf_skb_get_xfrm_state)(void *ctx, int index, void *state,
- int size, int flags) =
- (void *) BPF_FUNC_skb_get_xfrm_state;
-static int (*bpf_sk_select_reuseport)(void *ctx, void *map, void *key, __u32 flags) =
- (void *) BPF_FUNC_sk_select_reuseport;
-static int (*bpf_get_stack)(void *ctx, void *buf, int size, int flags) =
- (void *) BPF_FUNC_get_stack;
-static int (*bpf_fib_lookup)(void *ctx, struct bpf_fib_lookup *params,
- int plen, __u32 flags) =
- (void *) BPF_FUNC_fib_lookup;
-static int (*bpf_lwt_push_encap)(void *ctx, unsigned int type, void *hdr,
- unsigned int len) =
- (void *) BPF_FUNC_lwt_push_encap;
-static int (*bpf_lwt_seg6_store_bytes)(void *ctx, unsigned int offset,
- void *from, unsigned int len) =
- (void *) BPF_FUNC_lwt_seg6_store_bytes;
-static int (*bpf_lwt_seg6_action)(void *ctx, unsigned int action, void *param,
- unsigned int param_len) =
- (void *) BPF_FUNC_lwt_seg6_action;
-static int (*bpf_lwt_seg6_adjust_srh)(void *ctx, unsigned int offset,
- unsigned int len) =
- (void *) BPF_FUNC_lwt_seg6_adjust_srh;
-static int (*bpf_rc_repeat)(void *ctx) =
- (void *) BPF_FUNC_rc_repeat;
-static int (*bpf_rc_keydown)(void *ctx, unsigned int protocol,
- unsigned long long scancode, unsigned int toggle) =
- (void *) BPF_FUNC_rc_keydown;
-static unsigned long long (*bpf_get_current_cgroup_id)(void) =
- (void *) BPF_FUNC_get_current_cgroup_id;
-static void *(*bpf_get_local_storage)(void *map, unsigned long long flags) =
- (void *) BPF_FUNC_get_local_storage;
-static unsigned long long (*bpf_skb_cgroup_id)(void *ctx) =
- (void *) BPF_FUNC_skb_cgroup_id;
-static unsigned long long (*bpf_skb_ancestor_cgroup_id)(void *ctx, int level) =
- (void *) BPF_FUNC_skb_ancestor_cgroup_id;
-static struct bpf_sock *(*bpf_sk_lookup_tcp)(void *ctx,
- struct bpf_sock_tuple *tuple,
- int size, unsigned long long netns_id,
- unsigned long long flags) =
- (void *) BPF_FUNC_sk_lookup_tcp;
-static struct bpf_sock *(*bpf_skc_lookup_tcp)(void *ctx,
- struct bpf_sock_tuple *tuple,
- int size, unsigned long long netns_id,
- unsigned long long flags) =
- (void *) BPF_FUNC_skc_lookup_tcp;
-static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
- struct bpf_sock_tuple *tuple,
- int size, unsigned long long netns_id,
- unsigned long long flags) =
- (void *) BPF_FUNC_sk_lookup_udp;
-static int (*bpf_sk_release)(struct bpf_sock *sk) =
- (void *) BPF_FUNC_sk_release;
-static int (*bpf_skb_vlan_push)(void *ctx, __be16 vlan_proto, __u16 vlan_tci) =
- (void *) BPF_FUNC_skb_vlan_push;
-static int (*bpf_skb_vlan_pop)(void *ctx) =
- (void *) BPF_FUNC_skb_vlan_pop;
-static int (*bpf_rc_pointer_rel)(void *ctx, int rel_x, int rel_y) =
- (void *) BPF_FUNC_rc_pointer_rel;
-static void (*bpf_spin_lock)(struct bpf_spin_lock *lock) =
- (void *) BPF_FUNC_spin_lock;
-static void (*bpf_spin_unlock)(struct bpf_spin_lock *lock) =
- (void *) BPF_FUNC_spin_unlock;
-static struct bpf_sock *(*bpf_sk_fullsock)(struct bpf_sock *sk) =
- (void *) BPF_FUNC_sk_fullsock;
-static struct bpf_tcp_sock *(*bpf_tcp_sock)(struct bpf_sock *sk) =
- (void *) BPF_FUNC_tcp_sock;
-static struct bpf_sock *(*bpf_get_listener_sock)(struct bpf_sock *sk) =
- (void *) BPF_FUNC_get_listener_sock;
-static int (*bpf_skb_ecn_set_ce)(void *ctx) =
- (void *) BPF_FUNC_skb_ecn_set_ce;
-static int (*bpf_tcp_check_syncookie)(struct bpf_sock *sk,
- void *ip, int ip_len, void *tcp, int tcp_len) =
- (void *) BPF_FUNC_tcp_check_syncookie;
-static int (*bpf_sysctl_get_name)(void *ctx, char *buf,
- unsigned long long buf_len,
- unsigned long long flags) =
- (void *) BPF_FUNC_sysctl_get_name;
-static int (*bpf_sysctl_get_current_value)(void *ctx, char *buf,
- unsigned long long buf_len) =
- (void *) BPF_FUNC_sysctl_get_current_value;
-static int (*bpf_sysctl_get_new_value)(void *ctx, char *buf,
- unsigned long long buf_len) =
- (void *) BPF_FUNC_sysctl_get_new_value;
-static int (*bpf_sysctl_set_new_value)(void *ctx, const char *buf,
- unsigned long long buf_len) =
- (void *) BPF_FUNC_sysctl_set_new_value;
-static int (*bpf_strtol)(const char *buf, unsigned long long buf_len,
- unsigned long long flags, long *res) =
- (void *) BPF_FUNC_strtol;
-static int (*bpf_strtoul)(const char *buf, unsigned long long buf_len,
- unsigned long long flags, unsigned long *res) =
- (void *) BPF_FUNC_strtoul;
-static void *(*bpf_sk_storage_get)(void *map, struct bpf_sock *sk,
- void *value, __u64 flags) =
- (void *) BPF_FUNC_sk_storage_get;
-static int (*bpf_sk_storage_delete)(void *map, struct bpf_sock *sk) =
- (void *)BPF_FUNC_sk_storage_delete;
-static int (*bpf_send_signal)(unsigned sig) = (void *)BPF_FUNC_send_signal;
-static long long (*bpf_tcp_gen_syncookie)(struct bpf_sock *sk, void *ip,
- int ip_len, void *tcp, int tcp_len) =
- (void *) BPF_FUNC_tcp_gen_syncookie;
-
/* llvm builtin functions that eBPF C program may use to
* emit BPF_LD_ABS and BPF_LD_IND instructions
*/
@@ -273,55 +62,6 @@ struct bpf_map_def {
__attribute__ ((section(".maps." #name), used)) \
____btf_map_##name = { }
-static int (*bpf_skb_load_bytes)(void *ctx, int off, void *to, int len) =
- (void *) BPF_FUNC_skb_load_bytes;
-static int (*bpf_skb_load_bytes_relative)(void *ctx, int off, void *to, int len, __u32 start_header) =
- (void *) BPF_FUNC_skb_load_bytes_relative;
-static int (*bpf_skb_store_bytes)(void *ctx, int off, void *from, int len, int flags) =
- (void *) BPF_FUNC_skb_store_bytes;
-static int (*bpf_l3_csum_replace)(void *ctx, int off, int from, int to, int flags) =
- (void *) BPF_FUNC_l3_csum_replace;
-static int (*bpf_l4_csum_replace)(void *ctx, int off, int from, int to, int flags) =
- (void *) BPF_FUNC_l4_csum_replace;
-static int (*bpf_csum_diff)(void *from, int from_size, void *to, int to_size, int seed) =
- (void *) BPF_FUNC_csum_diff;
-static int (*bpf_skb_under_cgroup)(void *ctx, void *map, int index) =
- (void *) BPF_FUNC_skb_under_cgroup;
-static int (*bpf_skb_change_head)(void *, int len, int flags) =
- (void *) BPF_FUNC_skb_change_head;
-static int (*bpf_skb_pull_data)(void *, int len) =
- (void *) BPF_FUNC_skb_pull_data;
-static unsigned int (*bpf_get_cgroup_classid)(void *ctx) =
- (void *) BPF_FUNC_get_cgroup_classid;
-static unsigned int (*bpf_get_route_realm)(void *ctx) =
- (void *) BPF_FUNC_get_route_realm;
-static int (*bpf_skb_change_proto)(void *ctx, __be16 proto, __u64 flags) =
- (void *) BPF_FUNC_skb_change_proto;
-static int (*bpf_skb_change_type)(void *ctx, __u32 type) =
- (void *) BPF_FUNC_skb_change_type;
-static unsigned int (*bpf_get_hash_recalc)(void *ctx) =
- (void *) BPF_FUNC_get_hash_recalc;
-static unsigned long long (*bpf_get_current_task)(void) =
- (void *) BPF_FUNC_get_current_task;
-static int (*bpf_skb_change_tail)(void *ctx, __u32 len, __u64 flags) =
- (void *) BPF_FUNC_skb_change_tail;
-static long long (*bpf_csum_update)(void *ctx, __u32 csum) =
- (void *) BPF_FUNC_csum_update;
-static void (*bpf_set_hash_invalid)(void *ctx) =
- (void *) BPF_FUNC_set_hash_invalid;
-static int (*bpf_get_numa_node_id)(void) =
- (void *) BPF_FUNC_get_numa_node_id;
-static int (*bpf_probe_read_str)(void *ctx, __u32 size,
- const void *unsafe_ptr) =
- (void *) BPF_FUNC_probe_read_str;
-static unsigned int (*bpf_get_socket_uid)(void *ctx) =
- (void *) BPF_FUNC_get_socket_uid;
-static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
- (void *) BPF_FUNC_set_hash;
-static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
- unsigned long long flags) =
- (void *) BPF_FUNC_skb_adjust_room;
-
/* Scan the ARCH passed in from ARCH env variable (see Makefile) */
#if defined(__TARGET_ARCH_x86)
#define bpf_target_x86
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers
2019-10-07 3:07 [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Andrii Nakryiko
` (2 preceding siblings ...)
2019-10-07 3:07 ` [PATCH v4 bpf-next 3/3] libbpf: auto-generate list of " Andrii Nakryiko
@ 2019-10-07 5:35 ` Alexei Starovoitov
3 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2019-10-07 5:35 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
Quentin Monnet, Andrii Nakryiko, Kernel Team
On Sun, Oct 6, 2019 at 8:08 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> This patch set adds ability to auto-generate list of BPF helper definitions.
> It relies on existing scripts/bpf_helpers_doc.py and include/uapi/linux/bpf.h
> having a well-defined set of comments. bpf_helper_defs.h contains all BPF
> helper signatures which stay in sync with latest bpf.h UAPI. This
> auto-generated header is included from bpf_helpers.h, while all previously
> hand-written BPF helper definitions are simultaneously removed in patch #3.
> The end result is less manually maintained and redundant boilerplate code,
> while also more consistent and well-documented set of BPF helpers. Generated
> helper definitions are completely independent from a specific bpf.h on
> a target system, because it doesn't use BPF_FUNC_xxx enums.
Applied. Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs
2019-10-07 3:07 ` [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs Andrii Nakryiko
@ 2019-10-07 9:43 ` Daniel Borkmann
2019-10-07 17:47 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Borkmann @ 2019-10-07 9:43 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, netdev, ast, quentin.monnet, andrii.nakryiko, kernel-team
On Sun, Oct 06, 2019 at 08:07:36PM -0700, Andrii Nakryiko wrote:
> Various small fixes to BPF helper documentation comments, enabling
> automatic header generation with a list of BPF helpers.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
> tools/include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
> 2 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 77c6be96d676..a65c3b0c6935 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -794,7 +794,7 @@ union bpf_attr {
> * A 64-bit integer containing the current GID and UID, and
> * created as such: *current_gid* **<< 32 \|** *current_uid*.
Overall, I do like the approach that we keep generating the BPF helpers header
file from this documentation as it really enforces that the signatures here
must be 100% correct, and given this also lands in the man page it is /always/
in sync.
> - * int bpf_get_current_comm(char *buf, u32 size_of_buf)
> + * int bpf_get_current_comm(void *buf, u32 size_of_buf)
You did not elaborate why this needs to change from char * to void *. What is
the reason? Those rules should probably be documented somewhere, otherwise
people might keep adding them.
> * Description
> * Copy the **comm** attribute of the current task into *buf* of
> * *size_of_buf*. The **comm** attribute contains the name of
> @@ -1023,7 +1023,7 @@ union bpf_attr {
> * The realm of the route for the packet associated to *skb*, or 0
> * if none was found.
> *
> - * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
> + * int bpf_perf_event_output(void *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
This one here is because we have multiple program types with different input context.
> * Description
> * Write raw *data* blob into a special BPF perf event held by
> * *map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
> @@ -1068,7 +1068,7 @@ union bpf_attr {
> * Return
> * 0 on success, or a negative error in case of failure.
> *
> - * int bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len)
> + * int bpf_skb_load_bytes(const void *skb, u32 offset, void *to, u32 len)
Changing from struct sk_buff * to void * here, again due to struct sk_reuseport_kern *?
I'm wondering whether it would simply be much better to always just use 'void *ctx'
for everything that is BPF context as it may be just confusing to people why different
types are chosen sometimes leading to buggy drive-by attempts to 'fix' them back into
struct sk_buff * et al.
> * Description
> * This helper was provided as an easy way to load data from a
> * packet. It can be used to load *len* bytes from *offset* from
> @@ -1085,7 +1085,7 @@ union bpf_attr {
> * Return
> * 0 on success, or a negative error in case of failure.
> *
> - * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
> + * int bpf_get_stackid(void *ctx, struct bpf_map *map, u64 flags)
> * Description
> * Walk a user or a kernel stack and return its id. To achieve
> * this, the helper needs *ctx*, which is a pointer to the context
> @@ -1154,7 +1154,7 @@ union bpf_attr {
> * The checksum result, or a negative error code in case of
> * failure.
> *
> - * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
> + * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)
Same here and in more places in this patch, why u8 * -> void * and the like?
> * Description
> * Retrieve tunnel options metadata for the packet associated to
> * *skb*, and store the raw tunnel option data to the buffer *opt*
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs
2019-10-07 9:43 ` Daniel Borkmann
@ 2019-10-07 17:47 ` Andrii Nakryiko
2019-10-08 21:49 ` Daniel Borkmann
0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2019-10-07 17:47 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Quentin Monnet, Kernel Team
On Mon, Oct 7, 2019 at 2:43 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Sun, Oct 06, 2019 at 08:07:36PM -0700, Andrii Nakryiko wrote:
> > Various small fixes to BPF helper documentation comments, enabling
> > automatic header generation with a list of BPF helpers.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
> > tools/include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
> > 2 files changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 77c6be96d676..a65c3b0c6935 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -794,7 +794,7 @@ union bpf_attr {
> > * A 64-bit integer containing the current GID and UID, and
> > * created as such: *current_gid* **<< 32 \|** *current_uid*.
>
> Overall, I do like the approach that we keep generating the BPF helpers header
> file from this documentation as it really enforces that the signatures here
> must be 100% correct, and given this also lands in the man page it is /always/
> in sync.
>
> > - * int bpf_get_current_comm(char *buf, u32 size_of_buf)
> > + * int bpf_get_current_comm(void *buf, u32 size_of_buf)
>
> You did not elaborate why this needs to change from char * to void *. What is
> the reason? Those rules should probably be documented somewhere, otherwise
> people might keep adding them.
So here and below for __u8*, compiler is much more strict about
**exact** type of pointer passed by program into helpers. E.g, in one
selftest, we had struct like this
struct s {
char a[16];
};
struct s = {};
bpf_get_current_comm(&s.a);
and compiler was complaining that program passes char (*)[16] (pointer
to an array) instead of char *. So instead of forcing all the correct
program to do extra casts, I think it's better to stick to void * for
all the "pointer to a chunk of memory" use cases. With void *,
usability is much better.
>
> > * Description
> > * Copy the **comm** attribute of the current task into *buf* of
> > * *size_of_buf*. The **comm** attribute contains the name of
> > @@ -1023,7 +1023,7 @@ union bpf_attr {
> > * The realm of the route for the packet associated to *skb*, or 0
> > * if none was found.
> > *
> > - * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
> > + * int bpf_perf_event_output(void *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
>
> This one here is because we have multiple program types with different input context.
Yes.
>
> > * Description
> > * Write raw *data* blob into a special BPF perf event held by
> > * *map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
> > @@ -1068,7 +1068,7 @@ union bpf_attr {
> > * Return
> > * 0 on success, or a negative error in case of failure.
> > *
> > - * int bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len)
> > + * int bpf_skb_load_bytes(const void *skb, u32 offset, void *to, u32 len)
>
> Changing from struct sk_buff * to void * here, again due to struct sk_reuseport_kern *?
Yep.
>
> I'm wondering whether it would simply be much better to always just use 'void *ctx'
> for everything that is BPF context as it may be just confusing to people why different
> types are chosen sometimes leading to buggy drive-by attempts to 'fix' them back into
> struct sk_buff * et al.
I'm impartial on this issue. In some cases it might be helpful to
specify what is the expected type of the context, if it's only ever
one type, but there are lots of helpers that accept various contexts,
so for consistency its better to just have "void *context".
>
> > * Description
> > * This helper was provided as an easy way to load data from a
> > * packet. It can be used to load *len* bytes from *offset* from
> > @@ -1085,7 +1085,7 @@ union bpf_attr {
> > * Return
> > * 0 on success, or a negative error in case of failure.
> > *
> > - * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
> > + * int bpf_get_stackid(void *ctx, struct bpf_map *map, u64 flags)
> > * Description
> > * Walk a user or a kernel stack and return its id. To achieve
> > * this, the helper needs *ctx*, which is a pointer to the context
> > @@ -1154,7 +1154,7 @@ union bpf_attr {
> > * The checksum result, or a negative error code in case of
> > * failure.
> > *
> > - * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
> > + * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)
>
> Same here and in more places in this patch, why u8 * -> void * and the like?
See above, making compiler less picky about pointer to a memory buffer.
>
> > * Description
> > * Retrieve tunnel options metadata for the packet associated to
> > * *skb*, and store the raw tunnel option data to the buffer *opt*
> [...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs
2019-10-07 17:47 ` Andrii Nakryiko
@ 2019-10-08 21:49 ` Daniel Borkmann
2019-10-08 23:32 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Borkmann @ 2019-10-08 21:49 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Quentin Monnet, Kernel Team
On Mon, Oct 07, 2019 at 10:47:19AM -0700, Andrii Nakryiko wrote:
> On Mon, Oct 7, 2019 at 2:43 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > On Sun, Oct 06, 2019 at 08:07:36PM -0700, Andrii Nakryiko wrote:
> > > Various small fixes to BPF helper documentation comments, enabling
> > > automatic header generation with a list of BPF helpers.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
[...]
> > I'm wondering whether it would simply be much better to always just use 'void *ctx'
> > for everything that is BPF context as it may be just confusing to people why different
> > types are chosen sometimes leading to buggy drive-by attempts to 'fix' them back into
> > struct sk_buff * et al.
>
> I'm impartial on this issue. In some cases it might be helpful to
> specify what is the expected type of the context, if it's only ever
> one type, but there are lots of helpers that accept various contexts,
> so for consistency its better to just have "void *context".
I would favor consistency here to always have "void *context". One
additional issue I could see happening otherwise on top of the 'fix'
attempts is that if existing helpers get enabled for multiple program
types and these have different BPF context, then it might be quite
easy to forget converting struct __sk_buff * and whatnot to void * in
the helper API doc, so the auto-generated BPF helpers will continue
to have only the old type.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs
2019-10-08 21:49 ` Daniel Borkmann
@ 2019-10-08 23:32 ` Andrii Nakryiko
2019-10-10 23:55 ` Daniel Borkmann
0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2019-10-08 23:32 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Quentin Monnet, Kernel Team
On Tue, Oct 8, 2019 at 2:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Mon, Oct 07, 2019 at 10:47:19AM -0700, Andrii Nakryiko wrote:
> > On Mon, Oct 7, 2019 at 2:43 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > On Sun, Oct 06, 2019 at 08:07:36PM -0700, Andrii Nakryiko wrote:
> > > > Various small fixes to BPF helper documentation comments, enabling
> > > > automatic header generation with a list of BPF helpers.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> [...]
> > > I'm wondering whether it would simply be much better to always just use 'void *ctx'
> > > for everything that is BPF context as it may be just confusing to people why different
> > > types are chosen sometimes leading to buggy drive-by attempts to 'fix' them back into
> > > struct sk_buff * et al.
> >
> > I'm impartial on this issue. In some cases it might be helpful to
> > specify what is the expected type of the context, if it's only ever
> > one type, but there are lots of helpers that accept various contexts,
> > so for consistency its better to just have "void *context".
>
> I would favor consistency here to always have "void *context". One
> additional issue I could see happening otherwise on top of the 'fix'
> attempts is that if existing helpers get enabled for multiple program
> types and these have different BPF context, then it might be quite
> easy to forget converting struct __sk_buff * and whatnot to void * in
> the helper API doc, so the auto-generated BPF helpers will continue
> to have only the old type.
Ok, I can create a follow-up clean up patch changing all of them to
void *. There is also a weird singular case of having three
declarations of bpf_get_socket_cookie() with different contexts. I
assume I should just combine them into a single
declaration/description, right?
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs
2019-10-08 23:32 ` Andrii Nakryiko
@ 2019-10-10 23:55 ` Daniel Borkmann
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2019-10-10 23:55 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Quentin Monnet, Kernel Team
On Tue, Oct 08, 2019 at 04:32:01PM -0700, Andrii Nakryiko wrote:
> On Tue, Oct 8, 2019 at 2:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > On Mon, Oct 07, 2019 at 10:47:19AM -0700, Andrii Nakryiko wrote:
> > > On Mon, Oct 7, 2019 at 2:43 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > On Sun, Oct 06, 2019 at 08:07:36PM -0700, Andrii Nakryiko wrote:
> > > > > Various small fixes to BPF helper documentation comments, enabling
> > > > > automatic header generation with a list of BPF helpers.
> > > > >
> > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > [...]
> > > > I'm wondering whether it would simply be much better to always just use 'void *ctx'
> > > > for everything that is BPF context as it may be just confusing to people why different
> > > > types are chosen sometimes leading to buggy drive-by attempts to 'fix' them back into
> > > > struct sk_buff * et al.
> > >
> > > I'm impartial on this issue. In some cases it might be helpful to
> > > specify what is the expected type of the context, if it's only ever
> > > one type, but there are lots of helpers that accept various contexts,
> > > so for consistency its better to just have "void *context".
> >
> > I would favor consistency here to always have "void *context". One
> > additional issue I could see happening otherwise on top of the 'fix'
> > attempts is that if existing helpers get enabled for multiple program
> > types and these have different BPF context, then it might be quite
> > easy to forget converting struct __sk_buff * and whatnot to void * in
> > the helper API doc, so the auto-generated BPF helpers will continue
> > to have only the old type.
>
> Ok, I can create a follow-up clean up patch changing all of them to
> void *. There is also a weird singular case of having three
> declarations of bpf_get_socket_cookie() with different contexts. I
> assume I should just combine them into a single
> declaration/description, right?
Yes, definitely.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-10 23:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 3:07 [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs Andrii Nakryiko
2019-10-07 9:43 ` Daniel Borkmann
2019-10-07 17:47 ` Andrii Nakryiko
2019-10-08 21:49 ` Daniel Borkmann
2019-10-08 23:32 ` Andrii Nakryiko
2019-10-10 23:55 ` Daniel Borkmann
2019-10-07 3:07 ` [PATCH v4 bpf-next 2/3] scripts/bpf: teach bpf_helpers_doc.py to dump BPF helper definitions Andrii Nakryiko
2019-10-07 3:07 ` [PATCH v4 bpf-next 3/3] libbpf: auto-generate list of " Andrii Nakryiko
2019-10-07 5:35 ` [PATCH v4 bpf-next 0/3] Auto-generate list of BPF helpers Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).