* [PATCH bpf-next 1/3] bpf: handle trusted PTR_TO_BTF_ID_OR_NULL in argument check logic
2024-02-02 19:05 [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Andrii Nakryiko
@ 2024-02-02 19:05 ` Andrii Nakryiko
2024-02-02 19:05 ` [PATCH bpf-next 2/3] selftests/bpf: add more cases for __arg_trusted __arg_nullable args Andrii Nakryiko
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2024-02-02 19:05 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
Add PTR_TRUSTED | PTR_MAYBE_NULL modifiers for PTR_TO_BTF_ID to
check_reg_type() to support passing trusted nullable PTR_TO_BTF_ID
registers into global functions accepting `__arg_trusted __arg_nullable`
arguments. This hasn't been caught earlier because tests were either
passing known non-NULL PTR_TO_BTF_ID registers or known NULL (SCALAR)
registers.
When utilizing this functionality in complicated real-world BPF
application that passes around PTR_TO_BTF_ID_OR_NULL, it became apparent
that verifier rejects valid case because check_reg_type() doesn't handle
this case explicitly. Existing check_reg_type() logic is already
anticipating this combination, so we just need to explicitly list this
combo in the switch statement.
Fixes: e2b3c4ff5d18 ("bpf: add __arg_trusted global func arg tag")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/verifier.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index cd4d780e5400..c6f566d9dc3a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8234,6 +8234,7 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
switch ((int)reg->type) {
case PTR_TO_BTF_ID:
case PTR_TO_BTF_ID | PTR_TRUSTED:
+ case PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL:
case PTR_TO_BTF_ID | MEM_RCU:
case PTR_TO_BTF_ID | PTR_MAYBE_NULL:
case PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_RCU:
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf-next 2/3] selftests/bpf: add more cases for __arg_trusted __arg_nullable args
2024-02-02 19:05 [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Andrii Nakryiko
2024-02-02 19:05 ` [PATCH bpf-next 1/3] bpf: handle trusted PTR_TO_BTF_ID_OR_NULL in argument check logic Andrii Nakryiko
@ 2024-02-02 19:05 ` Andrii Nakryiko
2024-02-02 19:05 ` [PATCH bpf-next 3/3] bpf: don't emit warnings intended for global subprogs for static subprogs Andrii Nakryiko
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2024-02-02 19:05 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
Add extra layer of global functions to ensure that passing around
(trusted) PTR_TO_BTF_ID_OR_NULL registers works as expected. We also
extend trusted_task_arg_nullable subtest to check three possible valid
argumements: known NULL, known non-NULL, and maybe NULL cases.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../bpf/progs/verifier_global_ptr_args.c | 32 +++++++++++++++++--
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c b/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c
index 484d6262363f..4ab0ef18d7eb 100644
--- a/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c
+++ b/tools/testing/selftests/bpf/progs/verifier_global_ptr_args.c
@@ -19,15 +19,41 @@ __weak int subprog_trusted_task_nullable(struct task_struct *task __arg_trusted
return task->pid + task->tgid;
}
-SEC("?kprobe")
+__weak int subprog_trusted_task_nullable_extra_layer(struct task_struct *task __arg_trusted __arg_nullable)
+{
+ return subprog_trusted_task_nullable(task) + subprog_trusted_task_nullable(NULL);
+}
+
+SEC("?tp_btf/task_newtask")
__success __log_level(2)
__msg("Validating subprog_trusted_task_nullable() func#1...")
__msg(": R1=trusted_ptr_or_null_task_struct(")
int trusted_task_arg_nullable(void *ctx)
{
- struct task_struct *t = bpf_get_current_task_btf();
+ struct task_struct *t1 = bpf_get_current_task_btf();
+ struct task_struct *t2 = bpf_task_acquire(t1);
+ int res = 0;
- return subprog_trusted_task_nullable(t) + subprog_trusted_task_nullable(NULL);
+ /* known NULL */
+ res += subprog_trusted_task_nullable(NULL);
+
+ /* known non-NULL */
+ res += subprog_trusted_task_nullable(t1);
+ res += subprog_trusted_task_nullable_extra_layer(t1);
+
+ /* unknown if NULL or not */
+ res += subprog_trusted_task_nullable(t2);
+ res += subprog_trusted_task_nullable_extra_layer(t2);
+
+ if (t2) {
+ /* known non-NULL after explicit NULL check, just in case */
+ res += subprog_trusted_task_nullable(t2);
+ res += subprog_trusted_task_nullable_extra_layer(t2);
+
+ bpf_task_release(t2);
+ }
+
+ return res;
}
__weak int subprog_trusted_task_nonnull(struct task_struct *task __arg_trusted)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf-next 3/3] bpf: don't emit warnings intended for global subprogs for static subprogs
2024-02-02 19:05 [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Andrii Nakryiko
2024-02-02 19:05 ` [PATCH bpf-next 1/3] bpf: handle trusted PTR_TO_BTF_ID_OR_NULL in argument check logic Andrii Nakryiko
2024-02-02 19:05 ` [PATCH bpf-next 2/3] selftests/bpf: add more cases for __arg_trusted __arg_nullable args Andrii Nakryiko
@ 2024-02-02 19:05 ` Andrii Nakryiko
2024-02-02 21:27 ` [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Eduard Zingerman
2024-02-03 2:20 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2024-02-02 19:05 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
When btf_prepare_func_args() was generalized to handle both static and
global subprogs, a few warnings/errors that are meant only for global
subprog cases started to be emitted for static subprogs, where they are
sort of expected and irrelavant.
Stop polutting verifier logs with irrelevant scary-looking messages.
Fixes: e26080d0da87 ("bpf: prepare btf_prepare_func_args() for handling static subprogs")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/bpf/btf.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ef380e546952..f7725cb6e564 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7122,6 +7122,8 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
args = (const struct btf_param *)(t + 1);
nargs = btf_type_vlen(t);
if (nargs > MAX_BPF_FUNC_REG_ARGS) {
+ if (!is_global)
+ return -EINVAL;
bpf_log(log, "Global function %s() with %d > %d args. Buggy compiler.\n",
tname, nargs, MAX_BPF_FUNC_REG_ARGS);
return -EINVAL;
@@ -7131,6 +7133,8 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
while (btf_type_is_modifier(t))
t = btf_type_by_id(btf, t->type);
if (!btf_type_is_int(t) && !btf_is_any_enum(t)) {
+ if (!is_global)
+ return -EINVAL;
bpf_log(log,
"Global function %s() doesn't return scalar. Only those are supported.\n",
tname);
@@ -7251,6 +7255,8 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
sub->args[i].arg_type = ARG_ANYTHING;
continue;
}
+ if (!is_global)
+ return -EINVAL;
bpf_log(log, "Arg#%d type %s in %s() is not supported yet.\n",
i, btf_type_str(t), tname);
return -EINVAL;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 0/3] Two small fixes for global subprog tagging
2024-02-02 19:05 [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Andrii Nakryiko
` (2 preceding siblings ...)
2024-02-02 19:05 ` [PATCH bpf-next 3/3] bpf: don't emit warnings intended for global subprogs for static subprogs Andrii Nakryiko
@ 2024-02-02 21:27 ` Eduard Zingerman
2024-02-03 2:20 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: Eduard Zingerman @ 2024-02-02 21:27 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, ast, daniel, martin.lau; +Cc: kernel-team
On Fri, 2024-02-02 at 11:05 -0800, Andrii Nakryiko wrote:
> Fix a bug with passing trusted PTR_TO_BTF_ID_OR_NULL register into global
> subprog that expects `__arg_trusted __arg_nullable` arguments, which was
> discovered when adopting production BPF application.
>
> Also fix annoying warnings that are irrelevant for static subprogs, which are
> just an artifact of using btf_prepare_func_args() for both static and global
> subprogs.
Full patch-set looks good to me.
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next 0/3] Two small fixes for global subprog tagging
2024-02-02 19:05 [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Andrii Nakryiko
` (3 preceding siblings ...)
2024-02-02 21:27 ` [PATCH bpf-next 0/3] Two small fixes for global subprog tagging Eduard Zingerman
@ 2024-02-03 2:20 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-02-03 2:20 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Fri, 2 Feb 2024 11:05:26 -0800 you wrote:
> Fix a bug with passing trusted PTR_TO_BTF_ID_OR_NULL register into global
> subprog that expects `__arg_trusted __arg_nullable` arguments, which was
> discovered when adopting production BPF application.
>
> Also fix annoying warnings that are irrelevant for static subprogs, which are
> just an artifact of using btf_prepare_func_args() for both static and global
> subprogs.
>
> [...]
Here is the summary with links:
- [bpf-next,1/3] bpf: handle trusted PTR_TO_BTF_ID_OR_NULL in argument check logic
https://git.kernel.org/bpf/bpf-next/c/8f13c34087d3
- [bpf-next,2/3] selftests/bpf: add more cases for __arg_trusted __arg_nullable args
https://git.kernel.org/bpf/bpf-next/c/e2e70535dd76
- [bpf-next,3/3] bpf: don't emit warnings intended for global subprogs for static subprogs
https://git.kernel.org/bpf/bpf-next/c/1eb986746a67
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] 6+ messages in thread