[bpf-next] libbpf: better fix for handling nulled-out struct_ops program

Commit Message

Andrii Nakryiko May 1, 2024, 4:17 a.m. UTC

Previous attempt to fix the handling of nulled-out (from skeleton)
struct_ops program is working well only if struct_ops program is defined
as non-autoloaded by default (i.e., has SEC("?struct_ops") annotation,
with question mark).

Unfortunately, that fix is incomplete due to how
bpf_object_adjust_struct_ops_autoload() is marking referenced or
non-referenced struct_ops program as autoloaded (or not). Because
bpf_object_adjust_struct_ops_autoload() is run after
bpf_map__init_kern_struct_ops() step, which sets program slot to NULL,
such programs won't be considered "referenced", and so its autoload
property won't be changed.

This all sounds convoluted and it is, but the desire is to have as
natural behavior (as far as struct_ops usage is concerned) as possible.

This fix is redoing the original fix but makes it work for
autoloaded-by-default struct_ops programs as well. We achieve this by
forcing prog->autoload to false if prog was declaratively set for some
struct_ops map, but then nulled-out from skeleton (programmatically).
This achieves desired effect of not autoloading it. If such program is
still referenced somewhere else (different struct_ops map or different
callback field), it will get its autoload property adjusted by
bpf_object_adjust_struct_ops_autoload() later.

We also fix selftest, which accidentally used SEC("?struct_ops")
annotation. It was meant to use autoload-by-default program from the
very beginning.

Fixes: f973fccd43d3 ("libbpf: handle nulled-out program in struct_ops correctly")
Cc: Kui-Feng Lee <[e-mail]>
Cc: Eduard Zingerman <[e-mail]>
Cc: Martin KaFai Lau <[e-mail]>
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 tools/lib/bpf/libbpf.c                        | 37 +++++++++++++------
 .../selftests/bpf/progs/struct_ops_module.c   |  2 +-
 2 files changed, 27 insertions(+), 12 deletions(-)

Comments

[e-mail] May 1, 2024, 5:30 p.m. UTC | #1

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <[e-mail]>:

On Tue, 30 Apr 2024 21:17:06 -0700 you wrote:
> Previous attempt to fix the handling of nulled-out (from skeleton)
> struct_ops program is working well only if struct_ops program is defined
> as non-autoloaded by default (i.e., has SEC("?struct_ops") annotation,
> with question mark).
> 
> Unfortunately, that fix is incomplete due to how
> bpf_object_adjust_struct_ops_autoload() is marking referenced or
> non-referenced struct_ops program as autoloaded (or not). Because
> bpf_object_adjust_struct_ops_autoload() is run after
> bpf_map__init_kern_struct_ops() step, which sets program slot to NULL,
> such programs won't be considered "referenced", and so its autoload
> property won't be changed.
> 
> [...]

Here is the summary with links:
  - [bpf-next] libbpf: better fix for handling nulled-out struct_ops program
    https://git.kernel.org/bpf/bpf-next/c/0737df6de946

You are awesome, thank you!

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7667671187e9..57a514eec49b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1128,6 +1128,7 @@  static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 		const struct btf_type *mtype, *kern_mtype;
 		__u32 mtype_id, kern_mtype_id;
 		void *mdata, *kern_mdata;
+		struct bpf_program *prog;
 		__s64 msize, kern_msize;
 		__u32 moff, kern_moff;
 		__u32 kern_member_idx;
@@ -1145,19 +1146,35 @@  static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 
 		kern_member = find_member_by_name(kern_btf, kern_type, mname);
 		if (!kern_member) {
-			/* Skip all zeros or null fields if they are not
-			 * presented in the kernel BTF.
-			 */
-			if (libbpf_is_mem_zeroed(mdata, msize)) {
-				st_ops->progs[i] = NULL;
-				pr_info("struct_ops %s: member %s not found in kernel, skipping it as it's set to zero\n",
+			if (!libbpf_is_mem_zeroed(mdata, msize)) {
+				pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
 					map->name, mname);
-				continue;
+				return -ENOTSUP;
 			}
 
-			pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
+			prog = st_ops->progs[i];
+			if (prog) {
+				/* 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
+				 * 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] = NULL;
+			}
+
+			/* Skip all-zero/NULL fields if they are not present in the kernel BTF */
+			pr_info("struct_ops %s: member %s not found in kernel, skipping it as it's set to zero\n",
 				map->name, mname);
-			return -ENOTSUP;
+			continue;
 		}
 
 		kern_member_idx = kern_member - btf_members(kern_type);
@@ -1183,8 +1200,6 @@  static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 		}
 
 		if (btf_is_ptr(mtype)) {
-			struct bpf_program *prog;
-
 			/* Update the value from the shadow type */
 			prog = *(void **)mdata;
 			st_ops->progs[i] = prog;
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
index 40109be2b3ae..4c56d4a9d9f4 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -63,7 +63,7 @@  struct bpf_testmod_ops___zeroed {
 	int zeroed;
 };
 
-SEC("?struct_ops/test_3")
+SEC("struct_ops/test_3")
 int BPF_PROG(zeroed_op)
 {
 	return 1;