[v2,bpf-next,10/20] bpf: Recognize btf_decl_tag("arg:arena") as PTR_TO_ARENA.

Commit Message

Alexei Starovoitov Feb. 9, 2024, 4:05 a.m. UTC

From: Alexei Starovoitov <[e-mail]>

In global bpf functions recognize btf_decl_tag("arg:arena") as PTR_TO_ARENA.

Note, when the verifier sees:

__weak void foo(struct bar *p)

it recognizes 'p' as PTR_TO_MEM and 'struct bar' has to be a struct with scalars.
Hence the only way to use arena pointers in global functions is to tag them with "arg:arena".

Signed-off-by: Alexei Starovoitov <[e-mail]>
---
 include/linux/bpf.h   |  1 +
 kernel/bpf/btf.c      | 19 +++++++++++++++----
 kernel/bpf/verifier.c | 15 +++++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

Comments

Kumar Kartikeya Dwivedi Feb. 10, 2024, 8:51 a.m. UTC | #1

On Fri, 9 Feb 2024 at 05:06, Alexei Starovoitov
<[e-mail]> wrote:
>
> From: Alexei Starovoitov <[e-mail]>
>
> In global bpf functions recognize btf_decl_tag("arg:arena") as PTR_TO_ARENA.
>
> Note, when the verifier sees:
>
> __weak void foo(struct bar *p)
>
> it recognizes 'p' as PTR_TO_MEM and 'struct bar' has to be a struct with scalars.
> Hence the only way to use arena pointers in global functions is to tag them with "arg:arena".
>
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> ---

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

Andrii Nakryiko Feb. 13, 2024, 11:14 p.m. UTC | #2

On Thu, Feb 8, 2024 at 8:06 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> From: Alexei Starovoitov <[e-mail]>
>
> In global bpf functions recognize btf_decl_tag("arg:arena") as PTR_TO_ARENA.
>
> Note, when the verifier sees:
>
> __weak void foo(struct bar *p)
>
> it recognizes 'p' as PTR_TO_MEM and 'struct bar' has to be a struct with scalars.
> Hence the only way to use arena pointers in global functions is to tag them with "arg:arena".
>
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> ---
>  include/linux/bpf.h   |  1 +
>  kernel/bpf/btf.c      | 19 +++++++++++++++----
>  kernel/bpf/verifier.c | 15 +++++++++++++++
>  3 files changed, 31 insertions(+), 4 deletions(-)
>

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5eeb9bf7e324..fa49602194d5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9348,6 +9348,18 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>                                 bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
>                                 return -EINVAL;
>                         }
> +               } else if (base_type(arg->arg_type) == ARG_PTR_TO_ARENA) {
> +                       /*
> +                        * Can pass any value and the kernel won't crash, but
> +                        * only PTR_TO_ARENA or SCALAR make sense. Everything
> +                        * else is a bug in the bpf program. Point it out to
> +                        * the user at the verification time instead of
> +                        * run-time debug nightmare.
> +                        */
> +                       if (reg->type != PTR_TO_ARENA && reg->type != SCALAR_VALUE) {

the comment above doesn't explain why it's ok to pass SCALAR_VALUE. Is
it because PTR_TO_ARENA will become SCALAR_VALUE after some arithmetic
operations and we don't want to regress user experience? If that's the
case, what's the way for user to convert SCALAR_VALUE back to
PTR_TO_ARENA without going through global subprog? bpf_cast_xxx
instruction through assembly?

> +                               bpf_log(log, "R%d is not a pointer to arena or scalar.\n", regno);
> +                               return -EINVAL;
> +                       }
>                 } else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
>                         ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
>                         if (ret)
> @@ -20329,6 +20341,9 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>                                 reg->btf = bpf_get_btf_vmlinux(); /* can't fail at this point */
>                                 reg->btf_id = arg->btf_id;
>                                 reg->id = ++env->id_gen;
> +                       } else if (base_type(arg->arg_type) == ARG_PTR_TO_ARENA) {
> +                               /* caller can pass either PTR_TO_ARENA or SCALAR */
> +                               mark_reg_unknown(env, regs, i);

shouldn't we set the register type to PTR_TO_ARENA here?


>                         } else {
>                                 WARN_ONCE(1, "BUG: unhandled arg#%d type %d\n",
>                                           i - BPF_REG_1, arg->arg_type);
> --
> 2.34.1
>

Alexei Starovoitov Feb. 14, 2024, 12:26 a.m. UTC | #3

On Tue, Feb 13, 2024 at 3:15 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Thu, Feb 8, 2024 at 8:06 PM Alexei Starovoitov
> <[e-mail]> wrote:
> >
> > From: Alexei Starovoitov <[e-mail]>
> >
> > In global bpf functions recognize btf_decl_tag("arg:arena") as PTR_TO_ARENA.
> >
> > Note, when the verifier sees:
> >
> > __weak void foo(struct bar *p)
> >
> > it recognizes 'p' as PTR_TO_MEM and 'struct bar' has to be a struct with scalars.
> > Hence the only way to use arena pointers in global functions is to tag them with "arg:arena".
> >
> > Signed-off-by: Alexei Starovoitov <[e-mail]>
> > ---
> >  include/linux/bpf.h   |  1 +
> >  kernel/bpf/btf.c      | 19 +++++++++++++++----
> >  kernel/bpf/verifier.c | 15 +++++++++++++++
> >  3 files changed, 31 insertions(+), 4 deletions(-)
> >
>
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 5eeb9bf7e324..fa49602194d5 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9348,6 +9348,18 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> >                                 bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
> >                                 return -EINVAL;
> >                         }
> > +               } else if (base_type(arg->arg_type) == ARG_PTR_TO_ARENA) {
> > +                       /*
> > +                        * Can pass any value and the kernel won't crash, but
> > +                        * only PTR_TO_ARENA or SCALAR make sense. Everything
> > +                        * else is a bug in the bpf program. Point it out to
> > +                        * the user at the verification time instead of
> > +                        * run-time debug nightmare.
> > +                        */
> > +                       if (reg->type != PTR_TO_ARENA && reg->type != SCALAR_VALUE) {
>
> the comment above doesn't explain why it's ok to pass SCALAR_VALUE. Is
> it because PTR_TO_ARENA will become SCALAR_VALUE after some arithmetic
> operations and we don't want to regress user experience? If that's the
> case, what's the way for user to convert SCALAR_VALUE back to
> PTR_TO_ARENA without going through global subprog? bpf_cast_xxx
> instruction through assembly?

bpf_cast_xx inline asm should never be used.
It's for selftests only until llvm 19 is released and in distros.
The scalar_value can come in lots of cases.
Any pointer dereference returns scalar.
Most of the time all arena math is on scalars.
Scalars are passed into global and static funcs and
become ptr_to_arena right before LDX/STX through that pointer.
Sometime llvm still does a bit of math after scalar became ptr_to_arena,
hence needs_zext flag to downgrade alu64 to alu32.
In these selftests that produce non trivial bpf progs
there are 4 such insns that needs_zext in arena_htab.bpf.o.
Also 23 cast_kern, zero cast_user,
and 57 ldx/stx from arena.

>
> > +                               bpf_log(log, "R%d is not a pointer to arena or scalar.\n", regno);
> > +                               return -EINVAL;
> > +                       }
> >                 } else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
> >                         ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
> >                         if (ret)
> > @@ -20329,6 +20341,9 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> >                                 reg->btf = bpf_get_btf_vmlinux(); /* can't fail at this point */
> >                                 reg->btf_id = arg->btf_id;
> >                                 reg->id = ++env->id_gen;
> > +                       } else if (base_type(arg->arg_type) == ARG_PTR_TO_ARENA) {
> > +                               /* caller can pass either PTR_TO_ARENA or SCALAR */
> > +                               mark_reg_unknown(env, regs, i);
>
> shouldn't we set the register type to PTR_TO_ARENA here?

No. It has to be scalar.
It's not ok to deref it with kern_vm_base yet.
It's a full 64-bit value here and upper 32-bit are likely correct user_vm_start.

Hence my struggle with this __arg_arena feature, since it's really for
the verifier not to complain at the call site only.

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 70d5351427e6..46a92e41b9d5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -718,6 +718,7 @@  enum bpf_arg_type {
 	 * on eBPF program stack
 	 */
 	ARG_PTR_TO_MEM,		/* pointer to valid memory (stack, packet, map value) */
+	ARG_PTR_TO_ARENA,
 
 	ARG_CONST_SIZE,		/* number of bytes accessed from memory */
 	ARG_CONST_SIZE_OR_ZERO,	/* number of bytes accessed from memory or 0 */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 8e06d29961f1..857059c8d56c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7053,10 +7053,11 @@  static int btf_get_ptr_to_btf_id(struct bpf_verifier_log *log, int arg_idx,
 }
 
 enum btf_arg_tag {
-	ARG_TAG_CTX = 0x1,
-	ARG_TAG_NONNULL = 0x2,
-	ARG_TAG_TRUSTED = 0x4,
-	ARG_TAG_NULLABLE = 0x8,
+	ARG_TAG_CTX	 = BIT_ULL(0),
+	ARG_TAG_NONNULL  = BIT_ULL(1),
+	ARG_TAG_TRUSTED  = BIT_ULL(2),
+	ARG_TAG_NULLABLE = BIT_ULL(3),
+	ARG_TAG_ARENA	 = BIT_ULL(4),
 };
 
 /* Process BTF of a function to produce high-level expectation of function
@@ -7168,6 +7169,8 @@  int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 				tags |= ARG_TAG_NONNULL;
 			} else if (strcmp(tag, "nullable") == 0) {
 				tags |= ARG_TAG_NULLABLE;
+			} else if (strcmp(tag, "arena") == 0) {
+				tags |= ARG_TAG_ARENA;
 			} else {
 				bpf_log(log, "arg#%d has unsupported set of tags\n", i);
 				return -EOPNOTSUPP;
@@ -7222,6 +7225,14 @@  int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
 			sub->args[i].btf_id = kern_type_id;
 			continue;
 		}
+		if (tags & ARG_TAG_ARENA) {
+			if (tags & ~ARG_TAG_ARENA) {
+				bpf_log(log, "arg#%d arena cannot be combined with any other tags\n", i);
+				return -EINVAL;
+			}
+			sub->args[i].arg_type = ARG_PTR_TO_ARENA;
+			continue;
+		}
 		if (is_global) { /* generic user data pointer */
 			u32 mem_size;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5eeb9bf7e324..fa49602194d5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9348,6 +9348,18 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 				bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
 				return -EINVAL;
 			}
+		} else if (base_type(arg->arg_type) == ARG_PTR_TO_ARENA) {
+			/*
+			 * Can pass any value and the kernel won't crash, but
+			 * only PTR_TO_ARENA or SCALAR make sense. Everything
+			 * else is a bug in the bpf program. Point it out to
+			 * the user at the verification time instead of
+			 * run-time debug nightmare.
+			 */
+			if (reg->type != PTR_TO_ARENA && reg->type != SCALAR_VALUE) {
+				bpf_log(log, "R%d is not a pointer to arena or scalar.\n", regno);
+				return -EINVAL;
+			}
 		} else if (arg->arg_type == (ARG_PTR_TO_DYNPTR | MEM_RDONLY)) {
 			ret = process_dynptr_func(env, regno, -1, arg->arg_type, 0);
 			if (ret)
@@ -20329,6 +20341,9 @@  static int do_check_common(struct bpf_verifier_env *env, int subprog)
 				reg->btf = bpf_get_btf_vmlinux(); /* can't fail at this point */
 				reg->btf_id = arg->btf_id;
 				reg->id = ++env->id_gen;
+			} else if (base_type(arg->arg_type) == ARG_PTR_TO_ARENA) {
+				/* caller can pass either PTR_TO_ARENA or SCALAR */
+				mark_reg_unknown(env, regs, i);
 			} else {
 				WARN_ONCE(1, "BUG: unhandled arg#%d type %d\n",
 					  i - BPF_REG_1, arg->arg_type);