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

Commit Message

Amery Hung Feb. 20, 2025, 10:15 p.m. UTC

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 <[e-mail]>
---
 kernel/bpf/verifier.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Eduard Zingerman Feb. 21, 2025, 12:08 a.m. UTC | #1

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 <[e-mail]>
> ---

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 <[e-mail]>

[...]

[e-mail] Feb. 21, 2025, 2:50 a.m. UTC | #2

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

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 <[e-mail]>
> 
> [...]

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!

Patch

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;