Merge branch 'libbpf: further struct_ops fixes and improvements'

Andrii Nakryiko says:

====================
Fix yet another case of mishandling SEC("struct_ops") programs that were
nulled out programmatically through BPF skeleton by the user.

While at it, add some improvements around detecting and reporting errors,
specifically a common case of declaring SEC("struct_ops") program, but
forgetting to actually make use of it by setting it as a callback
implementation in SEC(".struct_ops") variable (i.e., map) declaration.

A bunch of new selftests are added as well.
====================

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

5 files changed, 156 insertions, 17 deletions

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a3566a456bc8..5401f2df463d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1152,22 +1152,15 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 				return -ENOTSUP;
 			}
 
-			prog = st_ops->progs[i];
-			if (prog) {
+			if (st_ops->progs[i]) {
 				/* If we had declaratively set struct_ops callback, we need to
-				 * first validate that it's actually a struct_ops program.
-				 * And then force its autoload to false, because it doesn't have
+				 * force its autoload to false, because it doesn't have
 				 * a chance of succeeding from POV of the current struct_ops map.
 				 * If this program is still referenced somewhere else, though,
 				 * then bpf_object_adjust_struct_ops_autoload() will update its
 				 * autoload accordingly.
 				 */
-				if (!is_valid_st_ops_program(obj, prog)) {
-					pr_warn("struct_ops init_kern %s: member %s is declaratively assigned a non-struct_ops program\n",
-						map->name, mname);
-					return -EINVAL;
-				}
-				prog->autoload = false;
+				st_ops->progs[i]->autoload = false;
 				st_ops->progs[i] = NULL;
 			}
 
@@ -1200,11 +1193,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 		}
 
 		if (btf_is_ptr(mtype)) {
-			/* Update the value from the shadow type */
 			prog = *(void **)mdata;
+			/* just like for !kern_member case above, reset declaratively
+			 * set (at compile time) program's autload to false,
+			 * if user replaced it with another program or NULL
+			 */
+			if (st_ops->progs[i] && st_ops->progs[i] != prog)
+				st_ops->progs[i]->autoload = false;
+
+			/* Update the value from the shadow type */
 			st_ops->progs[i] = prog;
 			if (!prog)
 				continue;
+
 			if (!is_valid_st_ops_program(obj, prog)) {
 				pr_warn("struct_ops init_kern %s: member %s is not a struct_ops program\n",
 					map->name, mname);
@@ -7371,7 +7372,11 @@ static int bpf_object_load_prog(struct bpf_object *obj, struct bpf_program *prog
 	__u32 log_level = prog->log_level;
 	int ret, err;
 
-	if (prog->type == BPF_PROG_TYPE_UNSPEC) {
+	/* Be more helpful by rejecting programs that can't be validated early
+	 * with more meaningful and actionable error message.
+	 */
+	switch (prog->type) {
+	case BPF_PROG_TYPE_UNSPEC:
 		/*
 		 * The program type must be set.  Most likely we couldn't find a proper
 		 * section definition at load time, and thus we didn't infer the type.
@@ -7379,6 +7384,15 @@ static int bpf_object_load_prog(struct bpf_object *obj, struct bpf_program *prog
 		pr_warn("prog '%s': missing BPF prog type, check ELF section name '%s'\n",
 			prog->name, prog->sec_name);
 		return -EINVAL;
+	case BPF_PROG_TYPE_STRUCT_OPS:
+		if (prog->attach_btf_id == 0) {
+			pr_warn("prog '%s': SEC(\"struct_ops\") program isn't referenced anywhere, did you forget to use it?\n",
+				prog->name);
+			return -EINVAL;
+		}
+		break;
+	default:
+		break;
 	}
 
 	if (!insns || !insns_cnt)
diff --git a/tools/lib/bpf/str_error.c b/tools/lib/bpf/str_error.c
index 146da01979c7..5e6a1e27ddf9 100644
--- a/tools/lib/bpf/str_error.c
+++ b/tools/lib/bpf/str_error.c
@@ -2,6 +2,7 @@
 #undef _GNU_SOURCE
 #include <string.h>
 #include <stdio.h>
+#include <errno.h>
 #include "str_error.h"
 
 /* make sure libbpf doesn't use kernel-only integer typedefs */
@@ -15,7 +16,18 @@
 char *libbpf_strerror_r(int err, char *dst, int len)
 {
 	int ret = strerror_r(err < 0 ? -err : err, dst, len);
-	if (ret)
-		snprintf(dst, len, "ERROR: strerror_r(%d)=%d", err, ret);
+	/* on glibc <2.13, ret == -1 and errno is set, if strerror_r() can't
+	 * handle the error, on glibc >=2.13 *positive* (errno-like) error
+	 * code is returned directly
+	 */
+	if (ret == -1)
+		ret = errno;
+	if (ret) {
+		if (ret == EINVAL)
+			/* strerror_r() doesn't recognize this specific error */
+			snprintf(dst, len, "unknown error (%d)", err < 0 ? err : -err);
+		else
+			snprintf(dst, len, "ERROR: strerror_r(%d)=%d", err, ret);
+	}
 	return dst;
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index bd39586abd5a..29e183a80f49 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -4,6 +4,8 @@
 #include <time.h>
 
 #include "struct_ops_module.skel.h"
+#include "struct_ops_nulled_out_cb.skel.h"
+#include "struct_ops_forgotten_cb.skel.h"
 
 static void check_map_info(struct bpf_map_info *info)
 {
@@ -174,13 +176,83 @@ cleanup:
 	struct_ops_module__destroy(skel);
 }
 
+/* validate that it's ok to "turn off" callback that kernel supports */
+static void test_struct_ops_nulled_out_cb(void)
+{
+	struct struct_ops_nulled_out_cb *skel;
+	int err;
+
+	skel = struct_ops_nulled_out_cb__open();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	/* kernel knows about test_1, but we still null it out */
+	skel->struct_ops.ops->test_1 = NULL;
+
+	err = struct_ops_nulled_out_cb__load(skel);
+	if (!ASSERT_OK(err, "skel_load"))
+		goto cleanup;
+
+	ASSERT_FALSE(bpf_program__autoload(skel->progs.test_1_turn_off), "prog_autoload");
+	ASSERT_LT(bpf_program__fd(skel->progs.test_1_turn_off), 0, "prog_fd");
+
+cleanup:
+	struct_ops_nulled_out_cb__destroy(skel);
+}
+
+/* validate that libbpf generates reasonable error message if struct_ops is
+ * not referenced in any struct_ops map
+ */
+static void test_struct_ops_forgotten_cb(void)
+{
+	struct struct_ops_forgotten_cb *skel;
+	char *log;
+	int err;
+
+	skel = struct_ops_forgotten_cb__open();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	start_libbpf_log_capture();
+
+	err = struct_ops_forgotten_cb__load(skel);
+	if (!ASSERT_ERR(err, "skel_load"))
+		goto cleanup;
+
+	log = stop_libbpf_log_capture();
+	ASSERT_HAS_SUBSTR(log,
+			  "prog 'test_1_forgotten': SEC(\"struct_ops\") program isn't referenced anywhere, did you forget to use it?",
+			  "libbpf_log");
+	free(log);
+
+	struct_ops_forgotten_cb__destroy(skel);
+
+	/* now let's programmatically use it, we should be fine now */
+	skel = struct_ops_forgotten_cb__open();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	skel->struct_ops.ops->test_1 = skel->progs.test_1_forgotten; /* not anymore */
+
+	err = struct_ops_forgotten_cb__load(skel);
+	if (!ASSERT_OK(err, "skel_load"))
+		goto cleanup;
+
+cleanup:
+	struct_ops_forgotten_cb__destroy(skel);
+}
+
 void serial_test_struct_ops_module(void)
 {
-	if (test__start_subtest("test_struct_ops_load"))
+	if (test__start_subtest("struct_ops_load"))
 		test_struct_ops_load();
-	if (test__start_subtest("test_struct_ops_not_zeroed"))
+	if (test__start_subtest("struct_ops_not_zeroed"))
 		test_struct_ops_not_zeroed();
-	if (test__start_subtest("test_struct_ops_incompatible"))
+	if (test__start_subtest("struct_ops_incompatible"))
 		test_struct_ops_incompatible();
+	if (test__start_subtest("struct_ops_null_out_cb"))
+		test_struct_ops_nulled_out_cb();
+	if (test__start_subtest("struct_ops_forgotten_cb"))
+		test_struct_ops_forgotten_cb();
 }
 
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c b/tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c
new file mode 100644
index 000000000000..3c822103bd40
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1_forgotten)
+{
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops ops = {
+	/* we forgot to reference test_1_forgotten above, oops */
+};
+
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c b/tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c
new file mode 100644
index 000000000000..fa2021388485
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+int rand;
+int arr[1];
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1_turn_off)
+{
+	return arr[rand]; /* potentially way out of range access */
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops ops = {
+	.test_1 = (void *)test_1_turn_off,
+};
+