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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, Feb 13, 2024 at 3:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Feb 8, 2024 at 8:06 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > 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 <ast@kernel.org>
> > ---
> >  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.