[PATCH bpf-next 0/3] Revamp and fix libbpf's feature-probing APIs

[PATCH bpf-next 0/3] Revamp and fix libbpf's feature-probing APIs

[Andrii Nakryiko]

Fix and improve libbpf feature-probing APIs. Name them consistently and
deprecated previous inconsistently named APIs.

Cc: Dave Marchevsky <davemarchevsky@fb.com>

Andrii Nakryiko (3):
  libbpf: rework feature-probing APIs
  selftests/bpf: add libbpf feature-probing API selftests
  bpftool: reimplement large insn size limit feature probing

 tools/bpf/bpftool/feature.c                   |  26 +-
 tools/lib/bpf/libbpf.h                        |  52 +++-
 tools/lib/bpf/libbpf.map                      |   3 +
 tools/lib/bpf/libbpf_probes.c                 | 235 ++++++++++++++----
 tools/testing/selftests/bpf/config            |   2 +
 .../selftests/bpf/prog_tests/libbpf_probes.c  | 124 +++++++++
 6 files changed, 385 insertions(+), 57 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/libbpf_probes.c

-- 
2.30.2

[PATCH bpf-next 1/3] libbpf: rework feature-probing APIs

[Andrii Nakryiko]

Create three extensible alternatives to inconsistently named
feature-probing APIs:
  - libbpf_probe_bpf_prog_type() instead of bpf_probe_prog_type();
  - libbpf_probe_bpf_map_type() instead of bpf_probe_map_type();
  - libbpf_probe_bpf_helper() instead of bpf_probe_helper().

Set up return values such that libbpf can report errors (e.g., if some
combination of input arguments isn't possible to validate, etc), in
addition to whether the feature is supported (return value 1) or not
supported (return value 0).

Also schedule deprecation of those three APIs. Also schedule deprecation
of bpf_probe_large_insn_limit().

Also fix all the existing detection logic for various program and map
types that never worked:
  - BPF_PROG_TYPE_LIRC_MODE2;
  - BPF_PROG_TYPE_TRACING;
  - BPF_PROG_TYPE_LSM;
  - BPF_PROG_TYPE_EXT;
  - BPF_PROG_TYPE_SYSCALL;
  - BPF_PROG_TYPE_STRUCT_OPS;
  - BPF_MAP_TYPE_STRUCT_OPS;
  - BPF_MAP_TYPE_BLOOM_FILTER.

Above prog/map types needed special setups and detection logic to work.
Subsequent patch adds selftests that will make sure that all the
detection logic keeps working for all current and future program and map
types, avoiding otherwise inevitable bit rot.

  [0] Closes: https://github.com/libbpf/libbpf/issues/312

Cc: Dave Marchevsky <davemarchevsky@fb.com>
Cc: Julia Kartseva <hex@fb.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.h        |  52 +++++++-
 tools/lib/bpf/libbpf.map      |   3 +
 tools/lib/bpf/libbpf_probes.c | 235 ++++++++++++++++++++++++++--------
 3 files changed, 236 insertions(+), 54 deletions(-)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 42b2f36fd9f0..85dfef88b3d2 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1052,13 +1052,57 @@ bpf_prog_linfo__lfind(const struct bpf_prog_linfo *prog_linfo,
  * user, causing subsequent probes to fail. In this case, the caller may want
  * to adjust that limit with setrlimit().
  */
-LIBBPF_API bool bpf_probe_prog_type(enum bpf_prog_type prog_type,
-				    __u32 ifindex);
+LIBBPF_DEPRECATED_SINCE(0, 8, "use libbpf_probe_bpf_prog_type() instead")
+LIBBPF_API bool bpf_probe_prog_type(enum bpf_prog_type prog_type, __u32 ifindex);
+LIBBPF_DEPRECATED_SINCE(0, 8, "use libbpf_probe_bpf_map_type() instead")
 LIBBPF_API bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex);
-LIBBPF_API bool bpf_probe_helper(enum bpf_func_id id,
-				 enum bpf_prog_type prog_type, __u32 ifindex);
+LIBBPF_DEPRECATED_SINCE(0, 8, "use libbpf_probe_bpf_helper() instead")
+LIBBPF_API bool bpf_probe_helper(enum bpf_func_id id, enum bpf_prog_type prog_type, __u32 ifindex);
+LIBBPF_DEPRECATED_SINCE(0, 8, "implement your own or use bpftool for feature detection")
 LIBBPF_API bool bpf_probe_large_insn_limit(__u32 ifindex);
 
+/**
+ * @brief **libbpf_probe_bpf_prog_type()** detects if host kernel supports
+ * BPF programs of a given type.
+ * @param prog_type BPF program type to detect kernel support for
+ * @param opts reserved for future extensibility, should be NULL
+ * @return 1, if given program type is supported; 0, if given program type is
+ * not supported; negative error code if feature detection failed or can't be
+ * performed
+ *
+ * Make sure the process has required set of CAP_* permissions (or runs as
+ * root) when performing feature checking.
+ */
+LIBBPF_API int libbpf_probe_bpf_prog_type(enum bpf_prog_type prog_type, const void *opts);
+/**
+ * @brief **libbpf_probe_bpf_map_type()** detects if host kernel supports
+ * BPF maps of a given type.
+ * @param map_type BPF map type to detect kernel support for
+ * @param opts reserved for future extensibility, should be NULL
+ * @return 1, if given map type is supported; 0, if given map type is
+ * not supported; negative error code if feature detection failed or can't be
+ * performed
+ *
+ * Make sure the process has required set of CAP_* permissions (or runs as
+ * root) when performing feature checking.
+ */
+LIBBPF_API int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts);
+/**
+ * @brief **libbpf_probe_bpf_helper()** detects if host kernel supports the
+ * use of a given BPF helper from specified BPF program type.
+ * @param prog_type BPF program type used to check the support of BPF helper
+ * @param helper_id BPF helper ID (enum bpf_func_id) to check support for
+ * @param opts reserved for future extensibility, should be NULL
+ * @return 1, if given combination of program type and helper is supported; 0,
+ * if the combination is not supported; negative error code if feature
+ * detection for provided input arguments failed or can't be performed
+ *
+ * Make sure the process has required set of CAP_* permissions (or runs as
+ * root) when performing feature checking.
+ */
+LIBBPF_API int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type,
+				       enum bpf_func_id helper_id, const void *opts);
+
 /*
  * Get bpf_prog_info in continuous memory
  *
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index b3938b3f8fc9..529783967793 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -427,5 +427,8 @@ LIBBPF_0.7.0 {
 		bpf_program__log_level;
 		bpf_program__set_log_buf;
 		bpf_program__set_log_level;
+		libbpf_probe_bpf_helper;
+		libbpf_probe_bpf_map_type;
+		libbpf_probe_bpf_prog_type;
 		libbpf_set_memlock_rlim_max;
 };
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index 4bdec69523a7..65232bcaa84c 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -64,12 +64,20 @@ static int get_kernel_version(void)
 	return (version << 16) + (subversion << 8) + patchlevel;
 }
 
-static void
-probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
-	   size_t insns_cnt, char *buf, size_t buf_len, __u32 ifindex)
+static int probe_prog_load(enum bpf_prog_type prog_type,
+			   const struct bpf_insn *insns, size_t insns_cnt,
+			   char *log_buf, size_t log_buf_sz,
+			   __u32 ifindex)
 {
-	LIBBPF_OPTS(bpf_prog_load_opts, opts);
-	int fd;
+	LIBBPF_OPTS(bpf_prog_load_opts, opts,
+		.log_buf = log_buf,
+		.log_size = log_buf_sz,
+		.log_level = log_buf ? 1 : 0,
+		.prog_ifindex = ifindex,
+	);
+	int fd, err, exp_err = 0;
+	const char *exp_msg = NULL;
+	char buf[4096];
 
 	switch (prog_type) {
 	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
@@ -84,6 +92,38 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
 	case BPF_PROG_TYPE_KPROBE:
 		opts.kern_version = get_kernel_version();
 		break;
+	case BPF_PROG_TYPE_LIRC_MODE2:
+		opts.expected_attach_type = BPF_LIRC_MODE2;
+		break;
+	case BPF_PROG_TYPE_TRACING:
+	case BPF_PROG_TYPE_LSM:
+		opts.log_buf = buf;
+		opts.log_size = sizeof(buf);
+		opts.log_level = 1;
+		if (prog_type == BPF_PROG_TYPE_TRACING)
+			opts.expected_attach_type = BPF_TRACE_FENTRY;
+		else
+			opts.expected_attach_type = BPF_MODIFY_RETURN;
+		opts.attach_btf_id = 1;
+
+		exp_err = -EINVAL;
+		exp_msg = "attach_btf_id 1 is not a function";
+		break;
+	case BPF_PROG_TYPE_EXT:
+		opts.log_buf = buf;
+		opts.log_size = sizeof(buf);
+		opts.log_level = 1;
+		opts.attach_btf_id = 1;
+
+		exp_err = -EINVAL;
+		exp_msg = "Cannot replace kernel functions";
+		break;
+	case BPF_PROG_TYPE_SYSCALL:
+		opts.prog_flags = BPF_F_SLEEPABLE;
+		break;
+	case BPF_PROG_TYPE_STRUCT_OPS:
+		exp_err = -524; /* -EOPNOTSUPP */
+		break;
 	case BPF_PROG_TYPE_UNSPEC:
 	case BPF_PROG_TYPE_SOCKET_FILTER:
 	case BPF_PROG_TYPE_SCHED_CLS:
@@ -103,25 +143,42 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
 	case BPF_PROG_TYPE_RAW_TRACEPOINT:
 	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
 	case BPF_PROG_TYPE_LWT_SEG6LOCAL:
-	case BPF_PROG_TYPE_LIRC_MODE2:
 	case BPF_PROG_TYPE_SK_REUSEPORT:
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 	case BPF_PROG_TYPE_CGROUP_SYSCTL:
-	case BPF_PROG_TYPE_TRACING:
-	case BPF_PROG_TYPE_STRUCT_OPS:
-	case BPF_PROG_TYPE_EXT:
-	case BPF_PROG_TYPE_LSM:
-	default:
 		break;
+	default:
+		return -EOPNOTSUPP;
 	}
 
-	opts.prog_ifindex = ifindex;
-	opts.log_buf = buf;
-	opts.log_size = buf_len;
-
-	fd = bpf_prog_load(prog_type, NULL, "GPL", insns, insns_cnt, NULL);
+	fd = bpf_prog_load(prog_type, NULL, "GPL", insns, insns_cnt, &opts);
+	err = -errno;
 	if (fd >= 0)
 		close(fd);
+	if (exp_err) {
+		if (fd >= 0 || err != exp_err)
+			return 0;
+		if (exp_msg && !strstr(buf, exp_msg))
+			return 0;
+		return 1;
+	}
+	return fd >= 0 ? 1 : 0;
+}
+
+int libbpf_probe_bpf_prog_type(enum bpf_prog_type prog_type, const void *opts)
+{
+	struct bpf_insn insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN()
+	};
+	const size_t insn_cnt = ARRAY_SIZE(insns);
+	int ret;
+
+	if (opts)
+		return libbpf_err(-EINVAL);
+
+	ret = probe_prog_load(prog_type, insns, insn_cnt, NULL, 0, 0);
+	return libbpf_err(ret);
 }
 
 bool bpf_probe_prog_type(enum bpf_prog_type prog_type, __u32 ifindex)
@@ -131,12 +188,16 @@ bool bpf_probe_prog_type(enum bpf_prog_type prog_type, __u32 ifindex)
 		BPF_EXIT_INSN()
 	};
 
+	/* prefer libbpf_probe_bpf_prog_type() unless offload is requested */
+	if (ifindex == 0)
+		return libbpf_probe_bpf_prog_type(prog_type, NULL) == 1;
+
 	if (ifindex && prog_type == BPF_PROG_TYPE_SCHED_CLS)
 		/* nfp returns -EINVAL on exit(0) with TC offload */
 		insns[0].imm = 2;
 
 	errno = 0;
-	probe_load(prog_type, insns, ARRAY_SIZE(insns), NULL, 0, ifindex);
+	probe_prog_load(prog_type, insns, ARRAY_SIZE(insns), NULL, 0, ifindex);
 
 	return errno != EINVAL && errno != EOPNOTSUPP;
 }
@@ -197,16 +258,18 @@ static int load_local_storage_btf(void)
 				     strs, sizeof(strs));
 }
 
-bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
+static int probe_map_create(enum bpf_map_type map_type, __u32 ifindex)
 {
-	int key_size, value_size, max_entries, map_flags;
+	LIBBPF_OPTS(bpf_map_create_opts, opts);
+	int key_size, value_size, max_entries;
 	__u32 btf_key_type_id = 0, btf_value_type_id = 0;
-	int fd = -1, btf_fd = -1, fd_inner;
+	int fd = -1, btf_fd = -1, fd_inner = -1, exp_err = 0, err;
+
+	opts.map_ifindex = ifindex;
 
 	key_size	= sizeof(__u32);
 	value_size	= sizeof(__u32);
 	max_entries	= 1;
-	map_flags	= 0;
 
 	switch (map_type) {
 	case BPF_MAP_TYPE_STACK_TRACE:
@@ -215,7 +278,7 @@ bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
 	case BPF_MAP_TYPE_LPM_TRIE:
 		key_size	= sizeof(__u64);
 		value_size	= sizeof(__u64);
-		map_flags	= BPF_F_NO_PREALLOC;
+		opts.map_flags	= BPF_F_NO_PREALLOC;
 		break;
 	case BPF_MAP_TYPE_CGROUP_STORAGE:
 	case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
@@ -234,17 +297,25 @@ bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
 		btf_value_type_id = 3;
 		value_size = 8;
 		max_entries = 0;
-		map_flags = BPF_F_NO_PREALLOC;
+		opts.map_flags = BPF_F_NO_PREALLOC;
 		btf_fd = load_local_storage_btf();
 		if (btf_fd < 0)
-			return false;
+			return btf_fd;
 		break;
 	case BPF_MAP_TYPE_RINGBUF:
 		key_size = 0;
 		value_size = 0;
 		max_entries = 4096;
 		break;
-	case BPF_MAP_TYPE_UNSPEC:
+	case BPF_MAP_TYPE_STRUCT_OPS:
+		/* we'll get -ENOTSUPP for invalid BTF type ID for struct_ops */
+		opts.btf_vmlinux_value_type_id = 1;
+		exp_err = -524; /* -ENOTSUPP */
+		break;
+	case BPF_MAP_TYPE_BLOOM_FILTER:
+		key_size = 0;
+		max_entries = 1;
+		break;
 	case BPF_MAP_TYPE_HASH:
 	case BPF_MAP_TYPE_ARRAY:
 	case BPF_MAP_TYPE_PROG_ARRAY:
@@ -263,49 +334,114 @@ bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
 	case BPF_MAP_TYPE_XSKMAP:
 	case BPF_MAP_TYPE_SOCKHASH:
 	case BPF_MAP_TYPE_REUSEPORT_SOCKARRAY:
-	case BPF_MAP_TYPE_STRUCT_OPS:
-	default:
 		break;
+	case BPF_MAP_TYPE_UNSPEC:
+	default:
+		return -EOPNOTSUPP;
 	}
 
 	if (map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS ||
 	    map_type == BPF_MAP_TYPE_HASH_OF_MAPS) {
-		LIBBPF_OPTS(bpf_map_create_opts, opts);
-
 		/* TODO: probe for device, once libbpf has a function to create
 		 * map-in-map for offload
 		 */
 		if (ifindex)
-			return false;
+			goto cleanup;
 
 		fd_inner = bpf_map_create(BPF_MAP_TYPE_HASH, NULL,
 					  sizeof(__u32), sizeof(__u32), 1, NULL);
 		if (fd_inner < 0)
-			return false;
+			goto cleanup;
 
 		opts.inner_map_fd = fd_inner;
-		fd = bpf_map_create(map_type, NULL, sizeof(__u32), sizeof(__u32), 1, &opts);
-		close(fd_inner);
-	} else {
-		LIBBPF_OPTS(bpf_map_create_opts, opts);
-
-		/* Note: No other restriction on map type probes for offload */
-		opts.map_flags = map_flags;
-		opts.map_ifindex = ifindex;
-		if (btf_fd >= 0) {
-			opts.btf_fd = btf_fd;
-			opts.btf_key_type_id = btf_key_type_id;
-			opts.btf_value_type_id = btf_value_type_id;
-		}
+	}
 
-		fd = bpf_map_create(map_type, NULL, key_size, value_size, max_entries, &opts);
+	if (btf_fd >= 0) {
+		opts.btf_fd = btf_fd;
+		opts.btf_key_type_id = btf_key_type_id;
+		opts.btf_value_type_id = btf_value_type_id;
 	}
+
+	fd = bpf_map_create(map_type, NULL, key_size, value_size, max_entries, &opts);
+	err = -errno;
+
+cleanup:
 	if (fd >= 0)
 		close(fd);
+	if (fd_inner >= 0)
+		close(fd_inner);
 	if (btf_fd >= 0)
 		close(btf_fd);
 
-	return fd >= 0;
+	if (exp_err)
+		return fd < 0 && err == exp_err ? 1 : 0;
+	else
+		return fd >= 0 ? 1 : 0;
+}
+
+int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts)
+{
+	int ret;
+
+	if (opts)
+		return libbpf_err(-EINVAL);
+
+	ret = probe_map_create(map_type, 0);
+	return libbpf_err(ret);
+}
+
+bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
+{
+	return probe_map_create(map_type, ifindex) == 1;
+}
+
+int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
+			    const void *opts)
+{
+	struct bpf_insn insns[] = {
+		BPF_EMIT_CALL((__u32)helper_id),
+		BPF_EXIT_INSN(),
+	};
+	const size_t insn_cnt = ARRAY_SIZE(insns);
+	char buf[4096];
+	int ret;
+
+	if (opts)
+		return libbpf_err(-EINVAL);
+
+	/* we can't successfully load all prog types to check for BPF helper
+	 * support, so bail out with -EOPNOTSUPP error
+	 */
+	switch (prog_type) {
+	case BPF_PROG_TYPE_TRACING:
+	case BPF_PROG_TYPE_EXT:
+	case BPF_PROG_TYPE_LSM:
+	case BPF_PROG_TYPE_STRUCT_OPS:
+		return -EOPNOTSUPP;
+	default:
+		break;
+	}
+
+	buf[0] = '\0';
+	ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf), 0);
+	if (ret < 0)
+		return libbpf_err(ret);
+
+	/* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
+	 * at all, it will emit something like "invalid func unknown#181".
+	 * If BPF verifier recognizes BPF helper but it's not supported for
+	 * given BPF program type, it will emit "unknown func bpf_sys_bpf#166".
+	 * In both cases, provided combination of BPF program type and BPF
+	 * helper is not supported by the kernel.
+	 * In all other cases, probe_prog_load() above will either succeed (e.g.,
+	 * because BPF helper happens to accept no input arguments or it
+	 * accepts one input argument and initial PTR_TO_CTX is fine for
+	 * that), or we'll get some more specific BPF verifier error about
+	 * some unsatisfied conditions.
+	 */
+	if (ret == 0 && (strstr(buf, "invalid func ") || strstr(buf, "unknown func ")))
+		return 0;
+	return 1; /* assume supported */
 }
 
 bool bpf_probe_helper(enum bpf_func_id id, enum bpf_prog_type prog_type,
@@ -318,8 +454,7 @@ bool bpf_probe_helper(enum bpf_func_id id, enum bpf_prog_type prog_type,
 	char buf[4096] = {};
 	bool res;
 
-	probe_load(prog_type, insns, ARRAY_SIZE(insns), buf, sizeof(buf),
-		   ifindex);
+	probe_prog_load(prog_type, insns, ARRAY_SIZE(insns), buf, sizeof(buf), ifindex);
 	res = !grep(buf, "invalid func ") && !grep(buf, "unknown func ");
 
 	if (ifindex) {
@@ -351,8 +486,8 @@ bool bpf_probe_large_insn_limit(__u32 ifindex)
 	insns[BPF_MAXINSNS] = BPF_EXIT_INSN();
 
 	errno = 0;
-	probe_load(BPF_PROG_TYPE_SCHED_CLS, insns, ARRAY_SIZE(insns), NULL, 0,
-		   ifindex);
+	probe_prog_load(BPF_PROG_TYPE_SCHED_CLS, insns, ARRAY_SIZE(insns), NULL, 0,
+			ifindex);
 
 	return errno != E2BIG && errno != EINVAL;
 }
-- 
2.30.2

[PATCH bpf-next 2/3] selftests/bpf: add libbpf feature-probing API selftests

[Andrii Nakryiko]

Add selftests for prog/map/prog+helper feature probing APIs. Prog and
map selftests are designed in such a way that they will always test all
the possible prog/map types, based on running kernel's vmlinux BTF enum
definition. This way we'll always be sure that when adding new BPF
program types or map types, libbpf will be always updated accordingly to
be able to feature-detect them.

BPF prog_helper selftest will have to be manually extended with
interesting and important prog+helper combinations, it's easy, but can't
be completely automated.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/config            |   2 +
 .../selftests/bpf/prog_tests/libbpf_probes.c  | 124 ++++++++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/libbpf_probes.c

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 5192305159ec..f6287132fa89 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -38,7 +38,9 @@ CONFIG_IPV6_SIT=m
 CONFIG_BPF_JIT=y
 CONFIG_BPF_LSM=y
 CONFIG_SECURITY=y
+CONFIG_RC_CORE=y
 CONFIG_LIRC=y
+CONFIG_BPF_LIRC_MODE2=y
 CONFIG_IMA=y
 CONFIG_SECURITYFS=y
 CONFIG_IMA_WRITE_POLICY=y
diff --git a/tools/testing/selftests/bpf/prog_tests/libbpf_probes.c b/tools/testing/selftests/bpf/prog_tests/libbpf_probes.c
new file mode 100644
index 000000000000..9f766ddd946a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/libbpf_probes.c
@@ -0,0 +1,124 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2021 Facebook */
+
+#include <test_progs.h>
+#include <bpf/btf.h>
+
+void test_libbpf_probe_prog_types(void)
+{
+	struct btf *btf;
+	const struct btf_type *t;
+	const struct btf_enum *e;
+	int i, n, id;
+
+	btf = btf__parse("/sys/kernel/btf/vmlinux", NULL);
+	if (!ASSERT_OK_PTR(btf, "btf_parse"))
+		return;
+
+	/* find enum bpf_prog_type and enumerate each value */
+	id = btf__find_by_name_kind(btf, "bpf_prog_type", BTF_KIND_ENUM);
+	if (!ASSERT_GT(id, 0, "bpf_prog_type_id"))
+		goto cleanup;
+	t = btf__type_by_id(btf, id);
+	if (!ASSERT_OK_PTR(t, "bpf_prog_type_enum"))
+		goto cleanup;
+
+	for (e = btf_enum(t), i = 0, n = btf_vlen(t); i < n; e++, i++) {
+		const char *prog_type_name = btf__str_by_offset(btf, e->name_off);
+		enum bpf_prog_type prog_type = (enum bpf_prog_type)e->val;
+		int res;
+
+		if (prog_type == BPF_PROG_TYPE_UNSPEC)
+			continue;
+
+		if (!test__start_subtest(prog_type_name))
+			continue;
+
+		res = libbpf_probe_bpf_prog_type(prog_type, NULL);
+		ASSERT_EQ(res, 1, prog_type_name);
+	}
+
+cleanup:
+	btf__free(btf);
+}
+
+void test_libbpf_probe_map_types(void)
+{
+	struct btf *btf;
+	const struct btf_type *t;
+	const struct btf_enum *e;
+	int i, n, id;
+
+	btf = btf__parse("/sys/kernel/btf/vmlinux", NULL);
+	if (!ASSERT_OK_PTR(btf, "btf_parse"))
+		return;
+
+	/* find enum bpf_map_type and enumerate each value */
+	id = btf__find_by_name_kind(btf, "bpf_map_type", BTF_KIND_ENUM);
+	if (!ASSERT_GT(id, 0, "bpf_map_type_id"))
+		goto cleanup;
+	t = btf__type_by_id(btf, id);
+	if (!ASSERT_OK_PTR(t, "bpf_map_type_enum"))
+		goto cleanup;
+
+	for (e = btf_enum(t), i = 0, n = btf_vlen(t); i < n; e++, i++) {
+		const char *map_type_name = btf__str_by_offset(btf, e->name_off);
+		enum bpf_map_type map_type = (enum bpf_map_type)e->val;
+		int res;
+
+		if (map_type == BPF_MAP_TYPE_UNSPEC)
+			continue;
+
+		if (!test__start_subtest(map_type_name))
+			continue;
+
+		res = libbpf_probe_bpf_map_type(map_type, NULL);
+		ASSERT_EQ(res, 1, map_type_name);
+	}
+
+cleanup:
+	btf__free(btf);
+}
+
+void test_libbpf_probe_helpers(void)
+{
+#define CASE(prog, helper, supp) {			\
+	.prog_type_name = "BPF_PROG_TYPE_" # prog,	\
+	.helper_name = "bpf_" # helper,			\
+	.prog_type = BPF_PROG_TYPE_ ## prog,		\
+	.helper_id = BPF_FUNC_ ## helper,		\
+	.supported = supp,				\
+}
+	const struct case_def {
+		const char *prog_type_name;
+		const char *helper_name;
+		enum bpf_prog_type prog_type;
+		enum bpf_func_id helper_id;
+		bool supported;
+	} cases[] = {
+		CASE(KPROBE, unspec, false),
+		CASE(KPROBE, map_lookup_elem, true),
+		CASE(KPROBE, loop, true),
+
+		CASE(KPROBE, ktime_get_coarse_ns, false),
+		CASE(SOCKET_FILTER, ktime_get_coarse_ns, true),
+
+		CASE(KPROBE, sys_bpf, false),
+		CASE(SYSCALL, sys_bpf, true),
+	};
+	size_t case_cnt = ARRAY_SIZE(cases), i;
+	char buf[128];
+
+	for (i = 0; i < case_cnt; i++) {
+		const struct case_def *d = &cases[i];
+		int res;
+
+		snprintf(buf, sizeof(buf), "%s+%s", d->prog_type_name, d->helper_name);
+
+		if (!test__start_subtest(buf))
+			continue;
+
+		res = libbpf_probe_bpf_helper(d->prog_type, d->helper_id, NULL);
+		ASSERT_EQ(res, d->supported, buf);
+	}
+}
-- 
2.30.2

[PATCH bpf-next 3/3] bpftool: reimplement large insn size limit feature probing

[Andrii Nakryiko]

Reimplement bpf_probe_large_insn_limit() in bpftool, as that libbpf API
is scheduled for deprecation in v0.8.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/feature.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index 5397077d0d9e..6719b9282eca 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -642,12 +642,32 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
 		printf("\n");
 }
 
-static void
-probe_large_insn_limit(const char *define_prefix, __u32 ifindex)
+/*
+ * Probe for availability of kernel commit (5.3):
+ *
+ * c04c0d2b968a ("bpf: increase complexity limit and maximum program size")
+ */
+static void probe_large_insn_limit(const char *define_prefix, __u32 ifindex)
 {
+	LIBBPF_OPTS(bpf_prog_load_opts, opts,
+		.prog_ifindex = ifindex,
+	);
+	struct bpf_insn insns[BPF_MAXINSNS + 1];
 	bool res;
+	int i, fd;
+
+	for (i = 0; i < BPF_MAXINSNS; i++)
+		insns[i] = BPF_MOV64_IMM(BPF_REG_0, 1);
+	insns[BPF_MAXINSNS] = BPF_EXIT_INSN();
+
+	errno = 0;
+	fd = bpf_prog_load(BPF_PROG_TYPE_SCHED_CLS, NULL, "GPL",
+			   insns, ARRAY_SIZE(insns), &opts);
+	res = fd >= 0 || (errno != E2BIG && errno != EINVAL);
+
+	if (fd >= 0)
+		close(fd);
 
-	res = bpf_probe_large_insn_limit(ifindex);
 	print_bool_feature("have_large_insn_limit",
 			   "Large program size limit",
 			   "LARGE_INSN_LIMIT",
-- 
2.30.2

Re: [PATCH bpf-next 1/3] libbpf: rework feature-probing APIs

[Dave Marchevsky]

On 12/16/21 2:04 AM, Andrii Nakryiko wrote:   
> Create three extensible alternatives to inconsistently named
> feature-probing APIs:
>   - libbpf_probe_bpf_prog_type() instead of bpf_probe_prog_type();
>   - libbpf_probe_bpf_map_type() instead of bpf_probe_map_type();
>   - libbpf_probe_bpf_helper() instead of bpf_probe_helper().
> 
> Set up return values such that libbpf can report errors (e.g., if some
> combination of input arguments isn't possible to validate, etc), in
> addition to whether the feature is supported (return value 1) or not
> supported (return value 0).
> 
> Also schedule deprecation of those three APIs. Also schedule deprecation
> of bpf_probe_large_insn_limit().
> 
> Also fix all the existing detection logic for various program and map
> types that never worked:
>   - BPF_PROG_TYPE_LIRC_MODE2;
>   - BPF_PROG_TYPE_TRACING;
>   - BPF_PROG_TYPE_LSM;
>   - BPF_PROG_TYPE_EXT;
>   - BPF_PROG_TYPE_SYSCALL;
>   - BPF_PROG_TYPE_STRUCT_OPS;
>   - BPF_MAP_TYPE_STRUCT_OPS;
>   - BPF_MAP_TYPE_BLOOM_FILTER.
> 
> Above prog/map types needed special setups and detection logic to work.
> Subsequent patch adds selftests that will make sure that all the
> detection logic keeps working for all current and future program and map
> types, avoiding otherwise inevitable bit rot.
> 
>   [0] Closes: https://github.com/libbpf/libbpf/issues/312
> 
> Cc: Dave Marchevsky <davemarchevsky@fb.com>
> Cc: Julia Kartseva <hex@fb.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

[...]

> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index 4bdec69523a7..65232bcaa84c 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c

[...]

> @@ -84,6 +92,38 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
>  	case BPF_PROG_TYPE_KPROBE:
>  		opts.kern_version = get_kernel_version();
>  		break;
> +	case BPF_PROG_TYPE_LIRC_MODE2:
> +		opts.expected_attach_type = BPF_LIRC_MODE2;
> +		break;
> +	case BPF_PROG_TYPE_TRACING:
> +	case BPF_PROG_TYPE_LSM:
> +		opts.log_buf = buf;
> +		opts.log_size = sizeof(buf);
> +		opts.log_level = 1;
> +		if (prog_type == BPF_PROG_TYPE_TRACING)
> +			opts.expected_attach_type = BPF_TRACE_FENTRY;
> +		else
> +			opts.expected_attach_type = BPF_MODIFY_RETURN;
> +		opts.attach_btf_id = 1;
> +
> +		exp_err = -EINVAL;
> +		exp_msg = "attach_btf_id 1 is not a function";
> +		break;
> +	case BPF_PROG_TYPE_EXT:
> +		opts.log_buf = buf;
> +		opts.log_size = sizeof(buf);
> +		opts.log_level = 1;
> +		opts.attach_btf_id = 1;
> +
> +		exp_err = -EINVAL;
> +		exp_msg = "Cannot replace kernel functions";
> +		break;
> +	case BPF_PROG_TYPE_SYSCALL:
> +		opts.prog_flags = BPF_F_SLEEPABLE;
> +		break;
> +	case BPF_PROG_TYPE_STRUCT_OPS:
> +		exp_err = -524; /* -EOPNOTSUPP */

Why not use the ENOTSUPP macro here, and elsewhere in this patch?
Also, maybe the comment in this particular instance is incorrect?
(EOPNOTSUPP -> ENOTSUPP)

> +		break;
>  	case BPF_PROG_TYPE_UNSPEC:
>  	case BPF_PROG_TYPE_SOCKET_FILTER:
>  	case BPF_PROG_TYPE_SCHED_CLS:

[...]

> +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
> +			    const void *opts)
> +{
> +	struct bpf_insn insns[] = {
> +		BPF_EMIT_CALL((__u32)helper_id),
> +		BPF_EXIT_INSN(),
> +	};
> +	const size_t insn_cnt = ARRAY_SIZE(insns);
> +	char buf[4096];
> +	int ret;
> +
> +	if (opts)
> +		return libbpf_err(-EINVAL);
> +
> +	/* we can't successfully load all prog types to check for BPF helper
> +	 * support, so bail out with -EOPNOTSUPP error
> +	 */
> +	switch (prog_type) {
> +	case BPF_PROG_TYPE_TRACING:
> +	case BPF_PROG_TYPE_EXT:
> +	case BPF_PROG_TYPE_LSM:
> +	case BPF_PROG_TYPE_STRUCT_OPS:
> +		return -EOPNOTSUPP;
> +	default:
> +		break;
> +	}
> +
> +	buf[0] = '\0';
> +	ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf), 0);
> +	if (ret < 0)
> +		return libbpf_err(ret);
> +
> +	/* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
> +	 * at all, it will emit something like "invalid func unknown#181".
> +	 * If BPF verifier recognizes BPF helper but it's not supported for
> +	 * given BPF program type, it will emit "unknown func bpf_sys_bpf#166".
> +	 * In both cases, provided combination of BPF program type and BPF
> +	 * helper is not supported by the kernel.
> +	 * In all other cases, probe_prog_load() above will either succeed (e.g.,
> +	 * because BPF helper happens to accept no input arguments or it
> +	 * accepts one input argument and initial PTR_TO_CTX is fine for
> +	 * that), or we'll get some more specific BPF verifier error about
> +	 * some unsatisfied conditions.
> +	 */
> +	if (ret == 0 && (strstr(buf, "invalid func ") || strstr(buf, "unknown func ")))
> +		return 0;

Maybe worth adding a comment where these are logged in verifier.c, so that if
format is changed or a less brittle way of conveying this info is added, this
fn can be updated.

> +	return 1; /* assume supported */
>  }
>  

[...]

Re: [PATCH bpf-next 2/3] selftests/bpf: add libbpf feature-probing API selftests

[Dave Marchevsky]

On 12/16/21 2:04 AM, Andrii Nakryiko wrote:   
> Add selftests for prog/map/prog+helper feature probing APIs. Prog and
> map selftests are designed in such a way that they will always test all
> the possible prog/map types, based on running kernel's vmlinux BTF enum
> definition. This way we'll always be sure that when adding new BPF
> program types or map types, libbpf will be always updated accordingly to
> be able to feature-detect them.
> 
> BPF prog_helper selftest will have to be manually extended with
> interesting and important prog+helper combinations, it's easy, but can't
> be completely automated.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---

[...]

> +	for (e = btf_enum(t), i = 0, n = btf_vlen(t); i < n; e++, i++) {
> +		const char *prog_type_name = btf__str_by_offset(btf, e->name_off);
> +		enum bpf_prog_type prog_type = (enum bpf_prog_type)e->val;
> +		int res;
> +
> +		if (prog_type == BPF_PROG_TYPE_UNSPEC)
> +			continue;
> +
> +		if (!test__start_subtest(prog_type_name))
> +			continue;
> +
> +		res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> +		ASSERT_EQ(res, 1, prog_type_name);
> +	}

I like how easy BTF makes this.
Maybe worth trying to probe one-past-the-end of enum to confirm it fails as
expected?

Regardless,

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

> +cleanup:
> +	btf__free(btf);
> +}

[...]

Re: [PATCH bpf-next 3/3] bpftool: reimplement large insn size limit feature probing

[Dave Marchevsky]

On 12/16/21 2:04 AM, Andrii Nakryiko wrote:   
> Reimplement bpf_probe_large_insn_limit() in bpftool, as that libbpf API
> is scheduled for deprecation in v0.8.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/bpf/bpftool/feature.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

Re: [PATCH bpf-next 1/3] libbpf: rework feature-probing APIs

[Andrii Nakryiko]

On Thu, Dec 16, 2021 at 4:10 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 12/16/21 2:04 AM, Andrii Nakryiko wrote:
> > Create three extensible alternatives to inconsistently named
> > feature-probing APIs:
> >   - libbpf_probe_bpf_prog_type() instead of bpf_probe_prog_type();
> >   - libbpf_probe_bpf_map_type() instead of bpf_probe_map_type();
> >   - libbpf_probe_bpf_helper() instead of bpf_probe_helper().
> >
> > Set up return values such that libbpf can report errors (e.g., if some
> > combination of input arguments isn't possible to validate, etc), in
> > addition to whether the feature is supported (return value 1) or not
> > supported (return value 0).
> >
> > Also schedule deprecation of those three APIs. Also schedule deprecation
> > of bpf_probe_large_insn_limit().
> >
> > Also fix all the existing detection logic for various program and map
> > types that never worked:
> >   - BPF_PROG_TYPE_LIRC_MODE2;
> >   - BPF_PROG_TYPE_TRACING;
> >   - BPF_PROG_TYPE_LSM;
> >   - BPF_PROG_TYPE_EXT;
> >   - BPF_PROG_TYPE_SYSCALL;
> >   - BPF_PROG_TYPE_STRUCT_OPS;
> >   - BPF_MAP_TYPE_STRUCT_OPS;
> >   - BPF_MAP_TYPE_BLOOM_FILTER.
> >
> > Above prog/map types needed special setups and detection logic to work.
> > Subsequent patch adds selftests that will make sure that all the
> > detection logic keeps working for all current and future program and map
> > types, avoiding otherwise inevitable bit rot.
> >
> >   [0] Closes: https://github.com/libbpf/libbpf/issues/312
> >
> > Cc: Dave Marchevsky <davemarchevsky@fb.com>
> > Cc: Julia Kartseva <hex@fb.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> [...]
>
> > diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> > index 4bdec69523a7..65232bcaa84c 100644
> > --- a/tools/lib/bpf/libbpf_probes.c
> > +++ b/tools/lib/bpf/libbpf_probes.c
>
> [...]
>
> > @@ -84,6 +92,38 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
> >       case BPF_PROG_TYPE_KPROBE:
> >               opts.kern_version = get_kernel_version();
> >               break;
> > +     case BPF_PROG_TYPE_LIRC_MODE2:
> > +             opts.expected_attach_type = BPF_LIRC_MODE2;
> > +             break;
> > +     case BPF_PROG_TYPE_TRACING:
> > +     case BPF_PROG_TYPE_LSM:
> > +             opts.log_buf = buf;
> > +             opts.log_size = sizeof(buf);
> > +             opts.log_level = 1;
> > +             if (prog_type == BPF_PROG_TYPE_TRACING)
> > +                     opts.expected_attach_type = BPF_TRACE_FENTRY;
> > +             else
> > +                     opts.expected_attach_type = BPF_MODIFY_RETURN;
> > +             opts.attach_btf_id = 1;
> > +
> > +             exp_err = -EINVAL;
> > +             exp_msg = "attach_btf_id 1 is not a function";
> > +             break;
> > +     case BPF_PROG_TYPE_EXT:
> > +             opts.log_buf = buf;
> > +             opts.log_size = sizeof(buf);
> > +             opts.log_level = 1;
> > +             opts.attach_btf_id = 1;
> > +
> > +             exp_err = -EINVAL;
> > +             exp_msg = "Cannot replace kernel functions";
> > +             break;
> > +     case BPF_PROG_TYPE_SYSCALL:
> > +             opts.prog_flags = BPF_F_SLEEPABLE;
> > +             break;
> > +     case BPF_PROG_TYPE_STRUCT_OPS:
> > +             exp_err = -524; /* -EOPNOTSUPP */
>
> Why not use the ENOTSUPP macro here, and elsewhere in this patch?

ENOTSUPP is kernel-internal code, so there is no #define ENOTSUPP
available to user-space applications.

> Also, maybe the comment in this particular instance is incorrect?
> (EOPNOTSUPP -> ENOTSUPP)

Yeah, I seem to constantly mess up comments. It should be -ENOTSUPP.

>
> > +             break;
> >       case BPF_PROG_TYPE_UNSPEC:
> >       case BPF_PROG_TYPE_SOCKET_FILTER:
> >       case BPF_PROG_TYPE_SCHED_CLS:
>
> [...]
>
> > +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
> > +                         const void *opts)
> > +{
> > +     struct bpf_insn insns[] = {
> > +             BPF_EMIT_CALL((__u32)helper_id),
> > +             BPF_EXIT_INSN(),
> > +     };
> > +     const size_t insn_cnt = ARRAY_SIZE(insns);
> > +     char buf[4096];
> > +     int ret;
> > +
> > +     if (opts)
> > +             return libbpf_err(-EINVAL);
> > +
> > +     /* we can't successfully load all prog types to check for BPF helper
> > +      * support, so bail out with -EOPNOTSUPP error
> > +      */
> > +     switch (prog_type) {
> > +     case BPF_PROG_TYPE_TRACING:
> > +     case BPF_PROG_TYPE_EXT:
> > +     case BPF_PROG_TYPE_LSM:
> > +     case BPF_PROG_TYPE_STRUCT_OPS:
> > +             return -EOPNOTSUPP;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     buf[0] = '\0';
> > +     ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf), 0);
> > +     if (ret < 0)
> > +             return libbpf_err(ret);
> > +
> > +     /* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
> > +      * at all, it will emit something like "invalid func unknown#181".
> > +      * If BPF verifier recognizes BPF helper but it's not supported for
> > +      * given BPF program type, it will emit "unknown func bpf_sys_bpf#166".
> > +      * In both cases, provided combination of BPF program type and BPF
> > +      * helper is not supported by the kernel.
> > +      * In all other cases, probe_prog_load() above will either succeed (e.g.,
> > +      * because BPF helper happens to accept no input arguments or it
> > +      * accepts one input argument and initial PTR_TO_CTX is fine for
> > +      * that), or we'll get some more specific BPF verifier error about
> > +      * some unsatisfied conditions.
> > +      */
> > +     if (ret == 0 && (strstr(buf, "invalid func ") || strstr(buf, "unknown func ")))
> > +             return 0;
>
> Maybe worth adding a comment where these are logged in verifier.c, so that if
> format is changed or a less brittle way of conveying this info is added, this
> fn can be updated.

Not sure I want to point out that this is done in verifier.c
specifically. What if that code is moved somewhere else? Seems like a
bit too detailed and specific comment to add. I was hoping the above
big comment explains what we do here, thought?

As for the need to change this, with selftests I added we'll get
immediate selftests failure, so this won't bit rot and we'll be always
aware when the detection breaks.

>
> > +     return 1; /* assume supported */
> >  }
> >
>
> [...]

Re: [PATCH bpf-next 2/3] selftests/bpf: add libbpf feature-probing API selftests

[Andrii Nakryiko]

On Thu, Dec 16, 2021 at 4:21 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> On 12/16/21 2:04 AM, Andrii Nakryiko wrote:
> > Add selftests for prog/map/prog+helper feature probing APIs. Prog and
> > map selftests are designed in such a way that they will always test all
> > the possible prog/map types, based on running kernel's vmlinux BTF enum
> > definition. This way we'll always be sure that when adding new BPF
> > program types or map types, libbpf will be always updated accordingly to
> > be able to feature-detect them.
> >
> > BPF prog_helper selftest will have to be manually extended with
> > interesting and important prog+helper combinations, it's easy, but can't
> > be completely automated.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
>
> [...]
>
> > +     for (e = btf_enum(t), i = 0, n = btf_vlen(t); i < n; e++, i++) {
> > +             const char *prog_type_name = btf__str_by_offset(btf, e->name_off);
> > +             enum bpf_prog_type prog_type = (enum bpf_prog_type)e->val;
> > +             int res;
> > +
> > +             if (prog_type == BPF_PROG_TYPE_UNSPEC)
> > +                     continue;
> > +
> > +             if (!test__start_subtest(prog_type_name))
> > +                     continue;
> > +
> > +             res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> > +             ASSERT_EQ(res, 1, prog_type_name);
> > +     }
>
> I like how easy BTF makes this.
> Maybe worth trying to probe one-past-the-end of enum to confirm it fails as
> expected?
>

Yeah, sure, not a bad idea, I'll add in v2.

> Regardless,
>
> Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
>
> > +cleanup:
> > +     btf__free(btf);
> > +}
>
> [...]

Re: [PATCH bpf-next 1/3] libbpf: rework feature-probing APIs

[Dave Marchevsky]

On 12/16/21 7:41 PM, Andrii Nakryiko wrote:   
> On Thu, Dec 16, 2021 at 4:10 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>>
>> On 12/16/21 2:04 AM, Andrii Nakryiko wrote:
>>> Create three extensible alternatives to inconsistently named
>>> feature-probing APIs:
>>>   - libbpf_probe_bpf_prog_type() instead of bpf_probe_prog_type();
>>>   - libbpf_probe_bpf_map_type() instead of bpf_probe_map_type();
>>>   - libbpf_probe_bpf_helper() instead of bpf_probe_helper().
>>>
>>> Set up return values such that libbpf can report errors (e.g., if some
>>> combination of input arguments isn't possible to validate, etc), in
>>> addition to whether the feature is supported (return value 1) or not
>>> supported (return value 0).
>>>
>>> Also schedule deprecation of those three APIs. Also schedule deprecation
>>> of bpf_probe_large_insn_limit().
>>>
>>> Also fix all the existing detection logic for various program and map
>>> types that never worked:
>>>   - BPF_PROG_TYPE_LIRC_MODE2;
>>>   - BPF_PROG_TYPE_TRACING;
>>>   - BPF_PROG_TYPE_LSM;
>>>   - BPF_PROG_TYPE_EXT;
>>>   - BPF_PROG_TYPE_SYSCALL;
>>>   - BPF_PROG_TYPE_STRUCT_OPS;
>>>   - BPF_MAP_TYPE_STRUCT_OPS;
>>>   - BPF_MAP_TYPE_BLOOM_FILTER.
>>>
>>> Above prog/map types needed special setups and detection logic to work.
>>> Subsequent patch adds selftests that will make sure that all the
>>> detection logic keeps working for all current and future program and map
>>> types, avoiding otherwise inevitable bit rot.
>>>
>>>   [0] Closes: https://github.com/libbpf/libbpf/issues/312
>>>
>>> Cc: Dave Marchevsky <davemarchevsky@fb.com>
>>> Cc: Julia Kartseva <hex@fb.com>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>
>> [...]
>>
>>> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
>>> index 4bdec69523a7..65232bcaa84c 100644
>>> --- a/tools/lib/bpf/libbpf_probes.c
>>> +++ b/tools/lib/bpf/libbpf_probes.c
>>
>> [...]
>>
>>> @@ -84,6 +92,38 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
>>>       case BPF_PROG_TYPE_KPROBE:
>>>               opts.kern_version = get_kernel_version();
>>>               break;
>>> +     case BPF_PROG_TYPE_LIRC_MODE2:
>>> +             opts.expected_attach_type = BPF_LIRC_MODE2;
>>> +             break;
>>> +     case BPF_PROG_TYPE_TRACING:
>>> +     case BPF_PROG_TYPE_LSM:
>>> +             opts.log_buf = buf;
>>> +             opts.log_size = sizeof(buf);
>>> +             opts.log_level = 1;
>>> +             if (prog_type == BPF_PROG_TYPE_TRACING)
>>> +                     opts.expected_attach_type = BPF_TRACE_FENTRY;
>>> +             else
>>> +                     opts.expected_attach_type = BPF_MODIFY_RETURN;
>>> +             opts.attach_btf_id = 1;
>>> +
>>> +             exp_err = -EINVAL;
>>> +             exp_msg = "attach_btf_id 1 is not a function";
>>> +             break;
>>> +     case BPF_PROG_TYPE_EXT:
>>> +             opts.log_buf = buf;
>>> +             opts.log_size = sizeof(buf);
>>> +             opts.log_level = 1;
>>> +             opts.attach_btf_id = 1;
>>> +
>>> +             exp_err = -EINVAL;
>>> +             exp_msg = "Cannot replace kernel functions";
>>> +             break;
>>> +     case BPF_PROG_TYPE_SYSCALL:
>>> +             opts.prog_flags = BPF_F_SLEEPABLE;
>>> +             break;
>>> +     case BPF_PROG_TYPE_STRUCT_OPS:
>>> +             exp_err = -524; /* -EOPNOTSUPP */
>>
>> Why not use the ENOTSUPP macro here, and elsewhere in this patch?
> 
> ENOTSUPP is kernel-internal code, so there is no #define ENOTSUPP
> available to user-space applications.
> 
>> Also, maybe the comment in this particular instance is incorrect?
>> (EOPNOTSUPP -> ENOTSUPP)
> 
> Yeah, I seem to constantly mess up comments. It should be -ENOTSUPP.
> 
>>
>>> +             break;
>>>       case BPF_PROG_TYPE_UNSPEC:
>>>       case BPF_PROG_TYPE_SOCKET_FILTER:
>>>       case BPF_PROG_TYPE_SCHED_CLS:
>>
>> [...]
>>
>>> +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
>>> +                         const void *opts)
>>> +{
>>> +     struct bpf_insn insns[] = {
>>> +             BPF_EMIT_CALL((__u32)helper_id),
>>> +             BPF_EXIT_INSN(),
>>> +     };
>>> +     const size_t insn_cnt = ARRAY_SIZE(insns);
>>> +     char buf[4096];
>>> +     int ret;
>>> +
>>> +     if (opts)
>>> +             return libbpf_err(-EINVAL);
>>> +
>>> +     /* we can't successfully load all prog types to check for BPF helper
>>> +      * support, so bail out with -EOPNOTSUPP error
>>> +      */
>>> +     switch (prog_type) {
>>> +     case BPF_PROG_TYPE_TRACING:
>>> +     case BPF_PROG_TYPE_EXT:
>>> +     case BPF_PROG_TYPE_LSM:
>>> +     case BPF_PROG_TYPE_STRUCT_OPS:
>>> +             return -EOPNOTSUPP;
>>> +     default:
>>> +             break;
>>> +     }
>>> +
>>> +     buf[0] = '\0';
>>> +     ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf), 0);
>>> +     if (ret < 0)
>>> +             return libbpf_err(ret);
>>> +
>>> +     /* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
>>> +      * at all, it will emit something like "invalid func unknown#181".
>>> +      * If BPF verifier recognizes BPF helper but it's not supported for
>>> +      * given BPF program type, it will emit "unknown func bpf_sys_bpf#166".
>>> +      * In both cases, provided combination of BPF program type and BPF
>>> +      * helper is not supported by the kernel.
>>> +      * In all other cases, probe_prog_load() above will either succeed (e.g.,
>>> +      * because BPF helper happens to accept no input arguments or it
>>> +      * accepts one input argument and initial PTR_TO_CTX is fine for
>>> +      * that), or we'll get some more specific BPF verifier error about
>>> +      * some unsatisfied conditions.
>>> +      */
>>> +     if (ret == 0 && (strstr(buf, "invalid func ") || strstr(buf, "unknown func ")))
>>> +             return 0;
>>
>> Maybe worth adding a comment where these are logged in verifier.c, so that if
>> format is changed or a less brittle way of conveying this info is added, this
>> fn can be updated.
> 
> Not sure I want to point out that this is done in verifier.c
> specifically. What if that code is moved somewhere else? Seems like a
> bit too detailed and specific comment to add. I was hoping the above
> big comment explains what we do here, thought?
> 
> As for the need to change this, with selftests I added we'll get
> immediate selftests failure, so this won't bit rot and we'll be always
> aware when the detection breaks.

Ah, I think there's a misunderstanding. I meant "In verifier.c,
could you point out that this is done here". But I agree with
your point that this is unnecessary given the selftests added
later in the series.

Acked-by: Dave Marchevsky <davemarchevsky@fb.com>

>>
>>> +     return 1; /* assume supported */
>>>  }
>>>
>>
>> [...]

Re: [PATCH bpf-next 2/3] selftests/bpf: add libbpf feature-probing API selftests

[Andrii Nakryiko]

On Thu, Dec 16, 2021 at 4:42 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Dec 16, 2021 at 4:21 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> >
> > On 12/16/21 2:04 AM, Andrii Nakryiko wrote:
> > > Add selftests for prog/map/prog+helper feature probing APIs. Prog and
> > > map selftests are designed in such a way that they will always test all
> > > the possible prog/map types, based on running kernel's vmlinux BTF enum
> > > definition. This way we'll always be sure that when adding new BPF
> > > program types or map types, libbpf will be always updated accordingly to
> > > be able to feature-detect them.
> > >
> > > BPF prog_helper selftest will have to be manually extended with
> > > interesting and important prog+helper combinations, it's easy, but can't
> > > be completely automated.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> >
> > [...]
> >
> > > +     for (e = btf_enum(t), i = 0, n = btf_vlen(t); i < n; e++, i++) {
> > > +             const char *prog_type_name = btf__str_by_offset(btf, e->name_off);
> > > +             enum bpf_prog_type prog_type = (enum bpf_prog_type)e->val;
> > > +             int res;
> > > +
> > > +             if (prog_type == BPF_PROG_TYPE_UNSPEC)
> > > +                     continue;
> > > +
> > > +             if (!test__start_subtest(prog_type_name))
> > > +                     continue;
> > > +
> > > +             res = libbpf_probe_bpf_prog_type(prog_type, NULL);
> > > +             ASSERT_EQ(res, 1, prog_type_name);
> > > +     }
> >
> > I like how easy BTF makes this.
> > Maybe worth trying to probe one-past-the-end of enum to confirm it fails as
> > expected?
> >
>
> Yeah, sure, not a bad idea, I'll add in v2.

Couldn't do it :( Because selftest is using running kernel's BTF to
find out maximum BPF map type. But that maximum on a slightly outdated
kernel could be something that libbpf actually knows about and
supports (because it was compiled against the latest kernel UAPI
headers). So at run time libbpf returns 0 (correct answer, not
map/prog is not supported), but not the expected -EOPNOTSUPP. And I
don't want to hardcode the maximum enum values (like
BPF_MAP_TYPE_BLOOM_FILTER, as of now), because every time we add new
map we'll need to fix selftests in the same patch, which is a bit
annoying, I think.

If we had equivalents of __BPF_FUNC_MAX_ID for bpf_map_type and
bpf_prog_type, we could have just used those constants, so maybe we
should do that instead?

Regardless, I'm sending selftests as they are right now and we can
follow up with UAPI additions and selftests improvements separately.

>
> > Regardless,
> >
> > Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
> >
> > > +cleanup:
> > > +     btf__free(btf);
> > > +}
> >
> > [...]