* [PATCH bpf-next] libbpf: better fix for handling nulled-out struct_ops program
@ 2024-05-01 4:17 Andrii Nakryiko
2024-05-01 17:30 ` patchwork-bot+netdevbpf
0 siblings, 1 reply; 2+ messages in thread
From: Andrii Nakryiko @ 2024-05-01 4:17 UTC (permalink / raw
To: bpf, ast, daniel, martin.lau
Cc: andrii, kernel-team, Kui-Feng Lee, Eduard Zingerman
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 <thinker.li@gmail.com>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.c | 37 +++++++++++++------
.../selftests/bpf/progs/struct_ops_module.c | 2 +-
2 files changed, 27 insertions(+), 12 deletions(-)
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;
--
2.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH bpf-next] libbpf: better fix for handling nulled-out struct_ops program
2024-05-01 4:17 [PATCH bpf-next] libbpf: better fix for handling nulled-out struct_ops program Andrii Nakryiko
@ 2024-05-01 17:30 ` patchwork-bot+netdevbpf
0 siblings, 0 replies; 2+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-05-01 17:30 UTC (permalink / raw
To: Andrii Nakryiko
Cc: bpf, ast, daniel, martin.lau, kernel-team, thinker.li, eddyz87
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-05-01 17:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 4:17 [PATCH bpf-next] libbpf: better fix for handling nulled-out struct_ops program Andrii Nakryiko
2024-05-01 17:30 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).