[PATCH bpf-next 0/7] libbpf: further struct_ops fixes and improvements

[PATCH bpf-next 0/7] libbpf: further struct_ops fixes and improvements

[Andrii Nakryiko]

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.

Andrii Nakryiko (7):
  libbpf: remove unnecessary struct_ops prog validity check
  libbpf: handle yet another corner case of nulling out struct_ops
    program
  selftests/bpf: add another struct_ops callback use case test
  libbpf: fix libbpf_strerror_r() handling unknown errors
  libbpf: improve early detection of doomed-to-fail BPF program loading
  selftests/bpf: validate struct_ops early failure detection logic
  selftests/bpf: shorten subtest names for struct_ops_module test

 tools/lib/bpf/libbpf.c                        | 38 ++++++---
 tools/lib/bpf/str_error.c                     | 16 +++-
 .../bpf/prog_tests/test_struct_ops_module.c   | 78 ++++++++++++++++++-
 .../bpf/progs/struct_ops_forgotten_cb.c       | 19 +++++
 .../bpf/progs/struct_ops_nulled_out_cb.c      | 22 ++++++
 5 files changed, 156 insertions(+), 17 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c

-- 
2.43.0

[PATCH bpf-next 1/7] libbpf: remove unnecessary struct_ops prog validity check

[Andrii Nakryiko]

libbpf ensures that BPF program references set in map->st_ops->progs[i]
during open phase are always valid STRUCT_OPS programs. This is done in
bpf_object__collect_st_ops_relos(). So there is no need to double-check
that in bpf_map__init_kern_struct_ops().

Simplify the code by removing unnecessary check. Also, we avoid using
local prog variable to keep code similar to the upcoming fix, which adds
similar logic in another part of bpf_map__init_kern_struct_ops().

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a3566a456bc8..7d77a1b158ce 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;
 			}
 
-- 
2.43.0

[PATCH bpf-next 2/7] libbpf: handle yet another corner case of nulling out struct_ops program

[Andrii Nakryiko]

There is yet another corner case where user can set STRUCT_OPS program
reference in STRUCT_OPS map to NULL, but libbpf will fail to disable
autoload for such BPF program. This time it's the case of "new" kernel
which has type information about callback field, but user explicitly
nulled-out program reference from user-space after opening BPF object.

Fix, hopefully, the last remaining unhandled case.

Fixes: 0737df6de946 ("libbpf: better fix for handling nulled-out struct_ops program")
Fixes: f973fccd43d3 ("libbpf: handle nulled-out program in struct_ops correctly")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7d77a1b158ce..04de4fb81785 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1193,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);
-- 
2.43.0

[PATCH bpf-next 3/7] selftests/bpf: add another struct_ops callback use case test

[Andrii Nakryiko]

Add a test which tests the case that was just fixed. Kernel has full
type information about callback, but user explicitly nulls out the
reference to declaratively set BPF program reference.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/prog_tests/test_struct_ops_module.c   | 27 +++++++++++++++++++
 .../bpf/progs/struct_ops_nulled_out_cb.c      | 22 +++++++++++++++
 2 files changed, 49 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c

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..f3c61ebad323 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,7 @@
 #include <time.h>
 
 #include "struct_ops_module.skel.h"
+#include "struct_ops_nulled_out_cb.skel.h"
 
 static void check_map_info(struct bpf_map_info *info)
 {
@@ -174,6 +175,30 @@ static void test_struct_ops_incompatible(void)
 	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);
+}
+
 void serial_test_struct_ops_module(void)
 {
 	if (test__start_subtest("test_struct_ops_load"))
@@ -182,5 +207,7 @@ void serial_test_struct_ops_module(void)
 		test_struct_ops_not_zeroed();
 	if (test__start_subtest("test_struct_ops_incompatible"))
 		test_struct_ops_incompatible();
+	if (test__start_subtest("test_struct_ops_null_out_cb"))
+		test_struct_ops_nulled_out_cb();
 }
 
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,
+};
+
-- 
2.43.0

[PATCH bpf-next 4/7] libbpf: fix libbpf_strerror_r() handling unknown errors

[Andrii Nakryiko]

strerror_r(), used from libbpf-specific libbpf_strerror_r() wrapper is
documented to return error in two different ways, depending on glibc
version. Take that into account when handling strerror_r()'s own errors,
which happens when we pass some non-standard (internal) kernel error to
it. Before this patch we'd have "ERROR: strerror_r(524)=22", which is
quite confusing. Now for the same situation we'll see a bit less
visually scary "unknown error (-524)".

At least we won't confuse user with irrelevant EINVAL (22).

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/str_error.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

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;
 }
-- 
2.43.0

[PATCH bpf-next 5/7] libbpf: improve early detection of doomed-to-fail BPF program loading

[Andrii Nakryiko]

Extend libbpf's pre-load checks for BPF programs, detecting more typical
conditions that are destinated to cause BPF program failure. This is an
opportunity to provide more helpful and actionable error message to
users, instead of potentially very confusing BPF verifier log and/or
error.

In this case, we detect struct_ops BPF program that was not referenced
anywhere, but still attempted to be loaded (according to libbpf logic).
Suggest that the program might need to be used in some struct_ops
variable. User will get a message of the following kind:

  libbpf: prog 'test_1_forgotten': SEC("struct_ops") program isn't referenced anywhere, did you forget to use it?

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 04de4fb81785..5401f2df463d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7372,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.
@@ -7380,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)
-- 
2.43.0

[PATCH bpf-next 6/7] selftests/bpf: validate struct_ops early failure detection logic

[Andrii Nakryiko]

Add a simple test that validates that libbpf will reject isolated
struct_ops program early with helpful warning message.

Also validate that explicit use of such BPF program through BPF skeleton
after BPF object is open won't trigger any warnings.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/prog_tests/test_struct_ops_module.c   | 45 +++++++++++++++++++
 .../bpf/progs/struct_ops_forgotten_cb.c       | 19 ++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c

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 f3c61ebad323..3785b648c8ad 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
@@ -5,6 +5,7 @@
 
 #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)
 {
@@ -199,6 +200,48 @@ static void test_struct_ops_nulled_out_cb(void)
 	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"))
@@ -209,5 +252,7 @@ void serial_test_struct_ops_module(void)
 		test_struct_ops_incompatible();
 	if (test__start_subtest("test_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 */
+};
+
-- 
2.43.0

[PATCH bpf-next 7/7] selftests/bpf: shorten subtest names for struct_ops_module test

[Andrii Nakryiko]

Drive-by clean up, we shouldn't use meaningless "test_" prefix for
subtest names.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../selftests/bpf/prog_tests/test_struct_ops_module.c     | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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 3785b648c8ad..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
@@ -244,13 +244,13 @@ static void test_struct_ops_forgotten_cb(void)
 
 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("test_struct_ops_null_out_cb"))
+	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();
-- 
2.43.0

Re: [PATCH bpf-next 0/7] libbpf: further struct_ops fixes and improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Mon,  6 May 2024 17:13:28 -0700 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/7] libbpf: remove unnecessary struct_ops prog validity check
    https://git.kernel.org/bpf/bpf-next/c/8374b56b1df5
  - [bpf-next,2/7] libbpf: handle yet another corner case of nulling out struct_ops program
    https://git.kernel.org/bpf/bpf-next/c/e18e2e70dbd1
  - [bpf-next,3/7] selftests/bpf: add another struct_ops callback use case test
    https://git.kernel.org/bpf/bpf-next/c/9d66d60e968d
  - [bpf-next,4/7] libbpf: fix libbpf_strerror_r() handling unknown errors
    https://git.kernel.org/bpf/bpf-next/c/548c2ede0dc8
  - [bpf-next,5/7] libbpf: improve early detection of doomed-to-fail BPF program loading
    https://git.kernel.org/bpf/bpf-next/c/c78420bafe7c
  - [bpf-next,6/7] selftests/bpf: validate struct_ops early failure detection logic
    https://git.kernel.org/bpf/bpf-next/c/41df0733ea41
  - [bpf-next,7/7] selftests/bpf: shorten subtest names for struct_ops_module test
    https://git.kernel.org/bpf/bpf-next/c/7b9959b8cdbc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html