[PATCH bpf-next v1 1/2] bpf: Do not allow tail call in strcut_ops program with __ref argument

[PATCH bpf-next v1 1/2] bpf: Do not allow tail call in strcut_ops program with __ref argument

[Amery Hung]

Reject struct_ops programs with refcounted kptr arguments (arguments
tagged with __ref suffix) that tail call. Once a refcounted kptr is
passed to a struct_ops program from the kernel, it can be freed or
xchged into maps. As there is no guarantee a callee can get the same
valid refcounted kptr in the ctx, we cannot allow such usage.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 kernel/bpf/verifier.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 85b2d4e65834..8f1df279e432 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22450,10 +22450,11 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	const struct bpf_struct_ops *st_ops;
 	const struct btf_member *member;
 	struct bpf_prog *prog = env->prog;
+	bool has_refcounted_arg = false;
 	u32 btf_id, member_idx;
 	struct btf *btf;
 	const char *mname;
-	int err;
+	int i, err;
 
 	if (!prog->gpl_compatible) {
 		verbose(env, "struct ops programs must have a GPL compatible license\n");
@@ -22523,6 +22524,23 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 		return -EACCES;
 	}
 
+	for (i = 0; i < st_ops_desc->arg_info[member_idx].cnt; i++) {
+		if (st_ops_desc->arg_info[member_idx].info->refcounted) {
+			has_refcounted_arg = true;
+			break;
+		}
+	}
+
+	/* Tail call is not allowed for programs with refcounted arguments since we
+	 * cannot guarantee that valid refcounted kptrs will be passed to the callee.
+	 */
+	for (i = 0; i < env->subprog_cnt; i++) {
+		if (has_refcounted_arg && env->subprog_info[i].has_tail_call) {
+			verbose(env, "program with __ref argument cannot tail call\n");
+			return -EINVAL;
+		}
+	}
+
 	prog->aux->attach_func_proto = func_proto;
 	prog->aux->attach_func_name = mname;
 	env->ops = st_ops->verifier_ops;
-- 
2.47.1

[PATCH bpf-next v1 2/2] selftests/bpf: Test struct_ops program with __ref arg calling bpf_tail_call

[Amery Hung]

Test if the verifier rejects struct_ops program with __ref argument
calling bpf_tail_call().

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 .../prog_tests/test_struct_ops_refcounted.c   |  2 ++
 .../struct_ops_refcounted_fail__tail_call.c   | 36 +++++++++++++++++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    |  1 +
 3 files changed, 39 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_refcounted_fail__tail_call.c

diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_refcounted.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_refcounted.c
index e290a2f6db95..da60c715fc59 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_refcounted.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_refcounted.c
@@ -3,10 +3,12 @@
 #include "struct_ops_refcounted.skel.h"
 #include "struct_ops_refcounted_fail__ref_leak.skel.h"
 #include "struct_ops_refcounted_fail__global_subprog.skel.h"
+#include "struct_ops_refcounted_fail__tail_call.skel.h"
 
 void test_struct_ops_refcounted(void)
 {
 	RUN_TESTS(struct_ops_refcounted);
 	RUN_TESTS(struct_ops_refcounted_fail__ref_leak);
 	RUN_TESTS(struct_ops_refcounted_fail__global_subprog);
+	RUN_TESTS(struct_ops_refcounted_fail__tail_call);
 }
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_refcounted_fail__tail_call.c b/tools/testing/selftests/bpf/progs/struct_ops_refcounted_fail__tail_call.c
new file mode 100644
index 000000000000..3b125025a1f2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_refcounted_fail__tail_call.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "../test_kmods/bpf_testmod.h"
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+} prog_array SEC(".maps");
+
+/* Test that the verifier rejects a program with referenced kptr arguments
+ * that tail call
+ */
+SEC("struct_ops/test_refcounted")
+__failure __msg("program with __ref argument cannot tail call")
+int refcounted_fail__tail_call(unsigned long long *ctx)
+{
+	struct task_struct *task = (struct task_struct *)ctx[1];
+
+	bpf_task_release(task);
+	bpf_tail_call(ctx, &prog_array, 0);
+
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_ref_acquire = {
+	.test_refcounted = (void *)refcounted_fail__tail_call,
+};
+
diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
index 89dc502de9d4..578bfc40dd05 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -1130,6 +1130,7 @@ static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
 };
 
 static const struct bpf_verifier_ops bpf_testmod_verifier_ops = {
+	.get_func_proto	 = bpf_base_func_proto,
 	.is_valid_access = bpf_testmod_ops_is_valid_access,
 };
 
-- 
2.47.1

Re: [PATCH bpf-next v1 1/2] bpf: Do not allow tail call in strcut_ops program with __ref argument

[Eduard Zingerman]

On Thu, 2025-02-20 at 14:15 -0800, Amery Hung wrote:
> Reject struct_ops programs with refcounted kptr arguments (arguments
> tagged with __ref suffix) that tail call. Once a refcounted kptr is
> passed to a struct_ops program from the kernel, it can be freed or
> xchged into maps. As there is no guarantee a callee can get the same
> valid refcounted kptr in the ctx, we cannot allow such usage.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---

An alternative location for this check would be in the
check_helper_call(). If done there, this would allow dead code
elimination for tail calls within functions with refcounted arguments.
Which would be useful only if in the future tail calls from such
functions would be allowed (e.g. a program having a branch w/o tail
call for old kernel and with tail call for new kernel).
Probably unlikely to happen, so I think current position of the check is ok.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Test struct_ops program with __ref arg calling bpf_tail_call

[Eduard Zingerman]

On Thu, 2025-02-20 at 14:15 -0800, Amery Hung wrote:
> Test if the verifier rejects struct_ops program with __ref argument
> calling bpf_tail_call().
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

Re: [PATCH bpf-next v1 1/2] bpf: Do not allow tail call in strcut_ops program with __ref argument

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Thu, 20 Feb 2025 14:15:31 -0800 you wrote:
> Reject struct_ops programs with refcounted kptr arguments (arguments
> tagged with __ref suffix) that tail call. Once a refcounted kptr is
> passed to a struct_ops program from the kernel, it can be freed or
> xchged into maps. As there is no guarantee a callee can get the same
> valid refcounted kptr in the ctx, we cannot allow such usage.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next,v1,1/2] bpf: Do not allow tail call in strcut_ops program with __ref argument
    https://git.kernel.org/bpf/bpf-next/c/38f1e66abd18
  - [bpf-next,v1,2/2] selftests/bpf: Test struct_ops program with __ref arg calling bpf_tail_call
    https://git.kernel.org/bpf/bpf-next/c/63817c771194

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