[RFC 0/5] BPF infrastructure enhancements

[RFC 0/5] BPF infrastructure enhancements

[Stephen Hemminger]

While investigating the BPF program load in TAP device
found a number of minor issues that should be addressed.

Stephen Hemminger (5):
  tap: move forward declaration of bpf_load
  tap: remove unnecessary bzero() calls in bpf api
  tap: remove unnecessary cast in call to bpf_load
  tap: get errors from kernel on bpf load failure
  tap: stop "vendoring" linux bpf header

 drivers/net/tap/bpf/bpf_extract.py |   1 -
 drivers/net/tap/tap_bpf.h          | 121 -----------------------------
 drivers/net/tap/tap_bpf_api.c      |  73 +++++++++++------
 drivers/net/tap/tap_bpf_insns.h    |   1 -
 drivers/net/tap/tap_flow.c         |  16 ++--
 drivers/net/tap/tap_flow.h         |   4 +-
 6 files changed, 60 insertions(+), 156 deletions(-)
 delete mode 100644 drivers/net/tap/tap_bpf.h

-- 
2.43.0

[RFC 1/5] tap: move forward declaration of bpf_load

[Stephen Hemminger]

The local function bpf_load forward declaration should
be in the one file using it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_bpf.h     | 3 ---
 drivers/net/tap/tap_bpf_api.c | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/tap_bpf.h b/drivers/net/tap/tap_bpf.h
index 0d38bc111fe0..aa5a733525e1 100644
--- a/drivers/net/tap/tap_bpf.h
+++ b/drivers/net/tap/tap_bpf.h
@@ -115,7 +115,4 @@ enum {
 	BPF_MAP_ID_SIMPLE,
 };
 
-static int bpf_load(enum bpf_prog_type type, const struct bpf_insn *insns,
-		size_t insns_cnt, const char *license);
-
 #endif /* __TAP_BPF_H__ */
diff --git a/drivers/net/tap/tap_bpf_api.c b/drivers/net/tap/tap_bpf_api.c
index 15283f8917ed..a6adec855dda 100644
--- a/drivers/net/tap/tap_bpf_api.c
+++ b/drivers/net/tap/tap_bpf_api.c
@@ -15,6 +15,9 @@
 #include <tap_bpf.h>
 #include <tap_bpf_insns.h>
 
+static int bpf_load(enum bpf_prog_type type, const struct bpf_insn *insns,
+		    size_t insns_cnt, const char *license);
+
 /**
  * Load BPF program (section cls_q) into the kernel and return a bpf fd
  *
-- 
2.43.0

[RFC 2/5] tap: remove unnecessary bzero() calls in bpf api

[Stephen Hemminger]

The structures are already fully initialized, bzero() or memset
is redundant.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_bpf_api.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/tap/tap_bpf_api.c b/drivers/net/tap/tap_bpf_api.c
index a6adec855dda..d176da0802eb 100644
--- a/drivers/net/tap/tap_bpf_api.c
+++ b/drivers/net/tap/tap_bpf_api.c
@@ -120,7 +120,6 @@ static int bpf_load(enum bpf_prog_type type,
 {
 	union bpf_attr attr = {};
 
-	bzero(&attr, sizeof(attr));
 	attr.prog_type = type;
 	attr.insn_cnt = (__u32)insns_cnt;
 	attr.insns = ptr_to_u64(insns);
@@ -153,7 +152,6 @@ int tap_flow_bpf_rss_map_create(unsigned int key_size,
 {
 	union bpf_attr attr = {};
 
-	bzero(&attr, sizeof(attr));
 	attr.map_type    = BPF_MAP_TYPE_HASH;
 	attr.key_size    = key_size;
 	attr.value_size  = value_size;
@@ -181,8 +179,6 @@ int tap_flow_bpf_update_rss_elem(int fd, void *key, void *value)
 {
 	union bpf_attr attr = {};
 
-	bzero(&attr, sizeof(attr));
-
 	attr.map_type = BPF_MAP_TYPE_HASH;
 	attr.map_fd = fd;
 	attr.key = ptr_to_u64(key);
-- 
2.43.0

[RFC 3/5] tap: remove unnecessary cast in call to bpf_load

[Stephen Hemminger]

The callers already have the correct data type.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_bpf_api.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/tap/tap_bpf_api.c b/drivers/net/tap/tap_bpf_api.c
index d176da0802eb..c754c167a764 100644
--- a/drivers/net/tap/tap_bpf_api.c
+++ b/drivers/net/tap/tap_bpf_api.c
@@ -32,9 +32,8 @@ int tap_flow_bpf_cls_q(__u32 queue_idx)
 	cls_q_insns[1].imm = queue_idx;
 
 	return bpf_load(BPF_PROG_TYPE_SCHED_CLS,
-		(struct bpf_insn *)cls_q_insns,
-		RTE_DIM(cls_q_insns),
-		"Dual BSD/GPL");
+			cls_q_insns, RTE_DIM(cls_q_insns),
+			"Dual BSD/GPL");
 }
 
 /**
@@ -55,9 +54,8 @@ int tap_flow_bpf_calc_l3_l4_hash(__u32 key_idx, int map_fd)
 	l3_l4_hash_insns[9].imm = map_fd;
 
 	return bpf_load(BPF_PROG_TYPE_SCHED_ACT,
-		(struct bpf_insn *)l3_l4_hash_insns,
-		RTE_DIM(l3_l4_hash_insns),
-		"Dual BSD/GPL");
+			l3_l4_hash_insns, RTE_DIM(l3_l4_hash_insns),
+			"Dual BSD/GPL");
 }
 
 /**
-- 
2.43.0

[RFC 4/5] tap: get errors from kernel on bpf load failure

[Stephen Hemminger]

The bpf load kernel API can provide some useful diagnostics
on failure.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_bpf_api.c | 44 +++++++++++++++++++++++++++--------
 drivers/net/tap/tap_flow.c    | 16 ++++++++-----
 drivers/net/tap/tap_flow.h    |  4 ++--
 3 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/drivers/net/tap/tap_bpf_api.c b/drivers/net/tap/tap_bpf_api.c
index c754c167a764..29223b7f0ea7 100644
--- a/drivers/net/tap/tap_bpf_api.c
+++ b/drivers/net/tap/tap_bpf_api.c
@@ -15,8 +15,10 @@
 #include <tap_bpf.h>
 #include <tap_bpf_insns.h>
 
-static int bpf_load(enum bpf_prog_type type, const struct bpf_insn *insns,
-		    size_t insns_cnt, const char *license);
+static int bpf_load(enum bpf_prog_type type,
+		    const struct bpf_insn *insns, size_t insns_cnt,
+		    char *log_buf, size_t log_size,
+		    const char *license);
 
 /**
  * Load BPF program (section cls_q) into the kernel and return a bpf fd
@@ -24,15 +26,22 @@ static int bpf_load(enum bpf_prog_type type, const struct bpf_insn *insns,
  * @param queue_idx
  *   Queue index matching packet cb
  *
+ * @param log_buf
+ *   Buffer to place resulting error message (optional)
+ *
+ * @param log_size
+ *   Size of log_buf
+ *
  * @return
  *   -1 if the BPF program couldn't be loaded. An fd (int) otherwise.
  */
-int tap_flow_bpf_cls_q(__u32 queue_idx)
+int tap_flow_bpf_cls_q(__u32 queue_idx, char *log_buf, size_t log_size)
 {
 	cls_q_insns[1].imm = queue_idx;
 
 	return bpf_load(BPF_PROG_TYPE_SCHED_CLS,
 			cls_q_insns, RTE_DIM(cls_q_insns),
+			log_buf, log_size,
 			"Dual BSD/GPL");
 }
 
@@ -45,16 +54,23 @@ int tap_flow_bpf_cls_q(__u32 queue_idx)
  * @param[in] map_fd
  *   BPF RSS map file descriptor
  *
+ * @param log_buf
+ *   Buffer to place resulting error message (optional)
+ *
+ * @param log_size
+ *   Size of log_buf
+ *
  * @return
  *   -1 if the BPF program couldn't be loaded. An fd (int) otherwise.
  */
-int tap_flow_bpf_calc_l3_l4_hash(__u32 key_idx, int map_fd)
+int tap_flow_bpf_calc_l3_l4_hash(__u32 key_idx, int map_fd, char *log_buf, size_t log_size)
 {
 	l3_l4_hash_insns[4].imm = key_idx;
 	l3_l4_hash_insns[9].imm = map_fd;
 
 	return bpf_load(BPF_PROG_TYPE_SCHED_ACT,
 			l3_l4_hash_insns, RTE_DIM(l3_l4_hash_insns),
+			log_buf, log_size,
 			"Dual BSD/GPL");
 }
 
@@ -105,6 +121,12 @@ static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
  * @param[in] insns_cnt
  *   Number of BPF instructions (size of array)
  *
+ * @param[out] log_buf
+ *   Space for log message
+ *
+ * @param[in] log_size
+ *   Number of characters available in log_buf
+ *
  * @param[in] license
  *   License string that must be acknowledged by the kernel
  *
@@ -112,9 +134,8 @@ static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
  *   -1 if the BPF program couldn't be loaded, fd (file descriptor) otherwise
  */
 static int bpf_load(enum bpf_prog_type type,
-		  const struct bpf_insn *insns,
-		  size_t insns_cnt,
-		  const char *license)
+		    const struct bpf_insn *insns, size_t insns_cnt,
+		    char *log_buf, size_t log_size, const char *license)
 {
 	union bpf_attr attr = {};
 
@@ -122,9 +143,12 @@ static int bpf_load(enum bpf_prog_type type,
 	attr.insn_cnt = (__u32)insns_cnt;
 	attr.insns = ptr_to_u64(insns);
 	attr.license = ptr_to_u64(license);
-	attr.log_buf = ptr_to_u64(NULL);
-	attr.log_level = 0;
-	attr.kern_version = 0;
+
+	if (log_size > 0) {
+		attr.log_level = 2;
+		attr.log_buf = ptr_to_u64(log_buf);
+		attr.log_size = log_size;
+	}
 
 	return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
 }
diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index ed4d42f92f9f..897d71acbad1 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -18,6 +18,8 @@
 #include <tap_tcmsgs.h>
 #include <tap_rss.h>
 
+#define BPF_LOG_BUFSIZ	1024
+
 #ifndef HAVE_TC_FLOWER
 /*
  * For kernels < 4.2, this enum is not defined. Runtime checks will be made to
@@ -1885,11 +1887,13 @@ static int rss_enable(struct pmd_internals *pmd,
 	 * the correct queue.
 	 */
 	for (i = 0; i < pmd->dev->data->nb_rx_queues; i++) {
-		pmd->bpf_fd[i] = tap_flow_bpf_cls_q(i);
+		char log_buf[BPF_LOG_BUFSIZ];
+
+		pmd->bpf_fd[i] = tap_flow_bpf_cls_q(i, log_buf, sizeof(log_buf));
 		if (pmd->bpf_fd[i] < 0) {
 			TAP_LOG(ERR,
-				"Failed to load BPF section %s for queue %d",
-				SEC_NAME_CLS_Q, i);
+				"Failed to load BPF section %s for queue %u: %s",
+				SEC_NAME_CLS_Q, i, log_buf);
 			rte_flow_error_set(
 				error, ENOTSUP, RTE_FLOW_ERROR_TYPE_HANDLE,
 				NULL,
@@ -2074,6 +2078,7 @@ static int rss_add_actions(struct rte_flow *flow, struct pmd_internals *pmd,
 	/* 4096 is the maximum number of instructions for a BPF program */
 	unsigned int i;
 	int err;
+	char log_buf[BPF_LOG_BUFSIZ];
 	struct rss_key rss_entry = { .hash_fields = 0,
 				     .key_size = 0 };
 
@@ -2124,9 +2129,8 @@ static int rss_add_actions(struct rte_flow *flow, struct pmd_internals *pmd,
 	/*
 	 * Load bpf rules to calculate hash for this key_idx
 	 */
-
-	flow->bpf_fd[SEC_L3_L4] =
-		tap_flow_bpf_calc_l3_l4_hash(flow->key_idx, pmd->map_fd);
+	flow->bpf_fd[SEC_L3_L4] = tap_flow_bpf_calc_l3_l4_hash(flow->key_idx, pmd->map_fd,
+							       log_buf, sizeof(log_buf));
 	if (flow->bpf_fd[SEC_L3_L4] < 0) {
 		TAP_LOG(ERR,
 			"Failed to load BPF section %s (%d): %s",
diff --git a/drivers/net/tap/tap_flow.h b/drivers/net/tap/tap_flow.h
index 240fbc3dfaef..fb30b495fda1 100644
--- a/drivers/net/tap/tap_flow.h
+++ b/drivers/net/tap/tap_flow.h
@@ -57,8 +57,8 @@ int tap_flow_implicit_destroy(struct pmd_internals *pmd,
 int tap_flow_implicit_flush(struct pmd_internals *pmd,
 			    struct rte_flow_error *error);
 
-int tap_flow_bpf_cls_q(__u32 queue_idx);
-int tap_flow_bpf_calc_l3_l4_hash(__u32 key_idx, int map_fd);
+int tap_flow_bpf_cls_q(__u32 queue_idx, char *log_buf, size_t log_size);
+int tap_flow_bpf_calc_l3_l4_hash(__u32 key_idx, int map_fd, char *log_buf, size_t log_size);
 int tap_flow_bpf_rss_map_create(unsigned int key_size, unsigned int value_size,
 			unsigned int max_entries);
 int tap_flow_bpf_update_rss_elem(int fd, void *key, void *value);
-- 
2.43.0

[RFC 5/5] tap: stop "vendoring" linux bpf header

[Stephen Hemminger]

The proper place for finding bpf structures and functions is
in linux/bpf.h. The original version was trying to workaround the
case where the build environment was running on old pre BPF
version of Glibc, but the target environment had BPF. This is not
a supportable build method, and not how rest of DPDK works.

Having own private (and divergent) version headers leads to future
problems when BPF definitions evolve.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/bpf/bpf_extract.py |   1 -
 drivers/net/tap/tap_bpf.h          | 118 -----------------------------
 drivers/net/tap/tap_bpf_api.c      |  16 ++--
 drivers/net/tap/tap_bpf_insns.h    |   1 -
 4 files changed, 9 insertions(+), 127 deletions(-)
 delete mode 100644 drivers/net/tap/tap_bpf.h

diff --git a/drivers/net/tap/bpf/bpf_extract.py b/drivers/net/tap/bpf/bpf_extract.py
index b630c42b809f..73c4dafe4eca 100644
--- a/drivers/net/tap/bpf/bpf_extract.py
+++ b/drivers/net/tap/bpf/bpf_extract.py
@@ -65,7 +65,6 @@ def write_header(out, source):
         print(f' * Auto-generated from {source}', file=out)
     print(" * This not the original source file. Do NOT edit it.", file=out)
     print(" */\n", file=out)
-    print("#include <tap_bpf.h>", file=out)
 
 
 def main():
diff --git a/drivers/net/tap/tap_bpf.h b/drivers/net/tap/tap_bpf.h
deleted file mode 100644
index aa5a733525e1..000000000000
--- a/drivers/net/tap/tap_bpf.h
+++ /dev/null
@@ -1,118 +0,0 @@
-/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
- * Copyright 2017 Mellanox Technologies, Ltd
- */
-
-#ifndef __TAP_BPF_H__
-#define __TAP_BPF_H__
-
-#include <tap_autoconf.h>
-
-/* Do not #include <linux/bpf.h> since eBPF must compile on different
- * distros which may include partial definitions for eBPF (while the
- * kernel itself may support eBPF). Instead define here all that is needed
- */
-
-/* BPF_MAP_UPDATE_ELEM command flags */
-#define	BPF_ANY	0 /* create a new element or update an existing */
-
-/* BPF architecture instruction struct */
-struct bpf_insn {
-	__u8	code;
-	__u8	dst_reg:4;
-	__u8	src_reg:4;
-	__s16	off;
-	__s32	imm; /* immediate value */
-};
-
-/* BPF program types */
-enum bpf_prog_type {
-	BPF_PROG_TYPE_UNSPEC,
-	BPF_PROG_TYPE_SOCKET_FILTER,
-	BPF_PROG_TYPE_KPROBE,
-	BPF_PROG_TYPE_SCHED_CLS,
-	BPF_PROG_TYPE_SCHED_ACT,
-};
-
-/* BPF commands types */
-enum bpf_cmd {
-	BPF_MAP_CREATE,
-	BPF_MAP_LOOKUP_ELEM,
-	BPF_MAP_UPDATE_ELEM,
-	BPF_MAP_DELETE_ELEM,
-	BPF_MAP_GET_NEXT_KEY,
-	BPF_PROG_LOAD,
-};
-
-/* BPF maps types */
-enum bpf_map_type {
-	BPF_MAP_TYPE_UNSPEC,
-	BPF_MAP_TYPE_HASH,
-};
-
-/* union of anonymous structs used with TAP BPF commands */
-union bpf_attr {
-	/* BPF_MAP_CREATE command */
-	struct {
-		__u32	map_type;
-		__u32	key_size;
-		__u32	value_size;
-		__u32	max_entries;
-		__u32	map_flags;
-		__u32	inner_map_fd;
-	};
-
-	/* BPF_MAP_UPDATE_ELEM, BPF_MAP_DELETE_ELEM commands */
-	struct {
-		__u32		map_fd;
-		__aligned_u64	key;
-		union {
-			__aligned_u64 value;
-			__aligned_u64 next_key;
-		};
-		__u64		flags;
-	};
-
-	/* BPF_PROG_LOAD command */
-	struct {
-		__u32		prog_type;
-		__u32		insn_cnt;
-		__aligned_u64	insns;
-		__aligned_u64	license;
-		__u32		log_level;
-		__u32		log_size;
-		__aligned_u64	log_buf;
-		__u32		kern_version;
-		__u32		prog_flags;
-	};
-} __rte_aligned(8);
-
-#ifndef __NR_bpf
-# if defined(__i386__)
-#  define __NR_bpf 357
-# elif defined(__x86_64__)
-#  define __NR_bpf 321
-# elif defined(__arm__)
-#  define __NR_bpf 386
-# elif defined(__aarch64__)
-#  define __NR_bpf 280
-# elif defined(__sparc__)
-#  define __NR_bpf 349
-# elif defined(__s390__)
-#  define __NR_bpf 351
-# elif defined(__powerpc__)
-#  define __NR_bpf 361
-# elif defined(__riscv)
-#  define __NR_bpf 280
-# elif defined(__loongarch__)
-#  define __NR_bpf 280
-# else
-#  error __NR_bpf not defined
-# endif
-#endif
-
-enum {
-	BPF_MAP_ID_KEY,
-	BPF_MAP_ID_SIMPLE,
-};
-
-#endif /* __TAP_BPF_H__ */
diff --git a/drivers/net/tap/tap_bpf_api.c b/drivers/net/tap/tap_bpf_api.c
index 29223b7f0ea7..54e469ebf5ff 100644
--- a/drivers/net/tap/tap_bpf_api.c
+++ b/drivers/net/tap/tap_bpf_api.c
@@ -2,17 +2,13 @@
  * Copyright 2017 Mellanox Technologies, Ltd
  */
 
-#include <errno.h>
-#include <string.h>
 #include <unistd.h>
-#include <sys/queue.h>
+#include <syscall.h>
+#include <linux/bpf.h>
 
-#include <rte_malloc.h>
-#include <rte_eth_tap.h>
 #include <tap_flow.h>
 #include <tap_autoconf.h>
-#include <tap_tcmsgs.h>
-#include <tap_bpf.h>
+
 #include <tap_bpf_insns.h>
 
 static int bpf_load(enum bpf_prog_type type,
@@ -106,7 +102,13 @@ static inline __u64 ptr_to_u64(const void *ptr)
 static inline int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
 			unsigned int size)
 {
+#ifdef __NR_bpf
 	return syscall(__NR_bpf, cmd, attr, size);
+#else
+	fprintf(stderr, "No bpf syscall, kernel headers too old?\n");
+	errno = ENOSYS;
+	return -1;
+#endif
 }
 
 /**
diff --git a/drivers/net/tap/tap_bpf_insns.h b/drivers/net/tap/tap_bpf_insns.h
index 53fa76c4e6b0..88aec6885a06 100644
--- a/drivers/net/tap/tap_bpf_insns.h
+++ b/drivers/net/tap/tap_bpf_insns.h
@@ -3,7 +3,6 @@
  * This not the original source file. Do NOT edit it.
  */
 
-#include <tap_bpf.h>
 
 static struct bpf_insn cls_q_insns[] = {
 	{0x61,    2,    1,       52, 0x00000000},
-- 
2.43.0