[bpf] bpf: Fix error message on kfunc arg type mismatch

Commit Message

Maxim Mikityanskiy Sept. 9, 2024, 1:39 p.m. UTC

When "arg#%d expected pointer to ctx, but got %s" error is printed, both
template parts actually point to the type of the argument, therefore, it
will also say "but got PTR", regardless of what was the actual register
type.

Fix the message to print the register type in the second part of the
template, change the existing test to adapt to the new format, and add a
new test to test the case when arg is a pointer to context, but reg is a
scalar.

Fixes: 00b85860feb8 ("bpf: Rewrite kfunc argument handling")
Signed-off-by: Maxim Mikityanskiy <[e-mail]>
---
 kernel/bpf/verifier.c                               | 3 ++-
 tools/testing/selftests/bpf/prog_tests/kfunc_call.c | 1 +
 tools/testing/selftests/bpf/progs/kfunc_call_fail.c | 7 +++++++
 tools/testing/selftests/bpf/verifier/calls.c        | 2 +-
 4 files changed, 11 insertions(+), 2 deletions(-)

Comments

Alexei Starovoitov Sept. 9, 2024, 5:35 p.m. UTC | #1

On Mon, Sep 9, 2024 at 6:39 AM Maxim Mikityanskiy <[e-mail]> wrote:
>
> When "arg#%d expected pointer to ctx, but got %s" error is printed, both
> template parts actually point to the type of the argument, therefore, it
> will also say "but got PTR", regardless of what was the actual register
> type.
>
> Fix the message to print the register type in the second part of the
> template, change the existing test to adapt to the new format, and add a
> new test to test the case when arg is a pointer to context, but reg is a
> scalar.
>
> Fixes: 00b85860feb8 ("bpf: Rewrite kfunc argument handling")

Kumar,

please review.

This is bpf-next material. fwiw.

Kumar Kartikeya Dwivedi Sept. 9, 2024, 6:49 p.m. UTC | #2

On Mon, 9 Sept 2024 at 15:39, Maxim Mikityanskiy <[e-mail]> wrote:
>
> When "arg#%d expected pointer to ctx, but got %s" error is printed, both
> template parts actually point to the type of the argument, therefore, it
> will also say "but got PTR", regardless of what was the actual register
> type.
>
> Fix the message to print the register type in the second part of the
> template, change the existing test to adapt to the new format, and add a
> new test to test the case when arg is a pointer to context, but reg is a
> scalar.
>
> Fixes: 00b85860feb8 ("bpf: Rewrite kfunc argument handling")
> Signed-off-by: Maxim Mikityanskiy <[e-mail]>
> ---

Acked-by: Kumar Kartikeya Dwivedi <[e-mail]>

[e-mail] Sept. 9, 2024, 11 p.m. UTC | #3

Hello:

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

On Mon,  9 Sep 2024 16:39:09 +0300 you wrote:
> When "arg#%d expected pointer to ctx, but got %s" error is printed, both
> template parts actually point to the type of the argument, therefore, it
> will also say "but got PTR", regardless of what was the actual register
> type.
> 
> Fix the message to print the register type in the second part of the
> template, change the existing test to adapt to the new format, and add a
> new test to test the case when arg is a pointer to context, but reg is a
> scalar.
> 
> [...]

Here is the summary with links:
  - [bpf] bpf: Fix error message on kfunc arg type mismatch
    https://git.kernel.org/bpf/bpf-next/c/bee109b7b3e5

You are awesome, thank you!

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d8520095ca03..8b9f0a2981d4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11948,7 +11948,8 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 		switch (kf_arg_type) {
 		case KF_ARG_PTR_TO_CTX:
 			if (reg->type != PTR_TO_CTX) {
-				verbose(env, "arg#%d expected pointer to ctx, but got %s\n", i, btf_type_str(t));
+				verbose(env, "arg#%d expected pointer to ctx, but got %s\n",
+					i, reg_type_str(env, reg->type));
 				return -EINVAL;
 			}
 
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index 5b743212292f..f79c8e53cb3e 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -68,6 +68,7 @@  static struct kfunc_test_params kfunc_tests[] = {
 	TC_FAIL(kfunc_call_test_get_mem_fail_oob, 0, "min value is outside of the allowed memory range"),
 	TC_FAIL(kfunc_call_test_get_mem_fail_not_const, 0, "is not a const"),
 	TC_FAIL(kfunc_call_test_mem_acquire_fail, 0, "acquire kernel function does not return PTR_TO_BTF_ID"),
+	TC_FAIL(kfunc_call_test_pointer_arg_type_mismatch, 0, "arg#0 expected pointer to ctx, but got scalar"),
 
 	/* success cases */
 	TC_TEST(kfunc_call_test1, 12),
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
index 4b0b7b79cdfb..08fae306539c 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
@@ -150,4 +150,11 @@  int kfunc_call_test_mem_acquire_fail(struct __sk_buff *skb)
 	return ret;
 }
 
+SEC("?tc")
+int kfunc_call_test_pointer_arg_type_mismatch(struct __sk_buff *skb)
+{
+	bpf_kfunc_call_test_pass_ctx((void *)10);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index d0cdd156cd55..7afc2619ab14 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -76,7 +76,7 @@ 
 	},
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.result = REJECT,
-	.errstr = "arg#0 expected pointer to ctx, but got PTR",
+	.errstr = "arg#0 expected pointer to ctx, but got fp",
 	.fixup_kfunc_btf_id = {
 		{ "bpf_kfunc_call_test_pass_ctx", 2 },
 	},