Re: [PATCH bpf-next v3 04/13] bpf: Indicate argument that will be released in bpf_func_proto

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Tue, Mar 22, 2022 at 07:17:40AM IST, Joanne Koong wrote:
> On Sun, Mar 20, 2022 at 6:34 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Add a few fields for each arg (argN_release) that when set to true,
> > tells verifier that for a release function, that argument's register
> > will be the one for which meta.ref_obj_id will be set, and which will
> > then be released using release_reference. To capture the regno,
> > introduce a release_regno field in bpf_call_arg_meta.
> >
> > This would be required in the next patch, where we may either pass NULL
> > or a refcounted pointer as an argument to the release function
> > bpf_kptr_xchg. Just releasing only when meta.ref_obj_id is set is not
> > enough, as there is a case where the type of argument needed matches,
> > but the ref_obj_id is set to 0. Hence, we must enforce that whenever
> > meta.ref_obj_id is zero, the register that is to be released can only
> > be NULL for a release function.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf.h   | 10 ++++++++++
> >  kernel/bpf/ringbuf.c  |  2 ++
> >  kernel/bpf/verifier.c | 39 +++++++++++++++++++++++++++++++++------
> >  net/core/filter.c     |  1 +
> >  4 files changed, 46 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index f35920d279dd..48ddde854d67 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -487,6 +487,16 @@ struct bpf_func_proto {
> >                 };
> >                 u32 *arg_btf_id[5];
> >         };
> > +       union {
> > +               struct {
> > +                       bool arg1_release;
> > +                       bool arg2_release;
> > +                       bool arg3_release;
> > +                       bool arg4_release;
> > +                       bool arg5_release;
> > +               };
> > +               bool arg_release[5];
> > +       };
>
> Instead of having the new fields "argx_release" for each arg, what are
> your thoughts on using PTR_RELEASE as an "enum bpf_type_flag" to the
> existing "argx_type" field? For example, instead of
>
>      .arg1_type      = ARG_PTR_TO_ALLOC_MEM,
>      .arg1_release   = true,
>
> could we do something like
>
>      .arg1_type      = ARG_PTR_TO_ALLOC_MEM | PTR_RELEASE
>
> In the verifier, we could determine whether an argument register
> releases a reference by checking whether this PTR_RELEASE flag is set.
>
> Would this be a little cleaner? Curious to hear your thoughts.
>

I don't dislike it, it's just a little more work to make sure we don't have it
set for arg_type in places where it isn't expected, so it would need some
inspection of existing code. It's certainly a bit better than having five bools.

I guess I'll try it out and see.

>
> >         int *ret_btf_id; /* return value btf_id */
> >         bool (*allowed)(const struct bpf_prog *prog);
> >  };
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index 710ba9de12ce..f40ce718630e 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> > @@ -405,6 +405,7 @@ const struct bpf_func_proto bpf_ringbuf_submit_proto = {
> >         .func           = bpf_ringbuf_submit,
> >         .ret_type       = RET_VOID,
> >         .arg1_type      = ARG_PTR_TO_ALLOC_MEM,
> > +       .arg1_release   = true,
> >         .arg2_type      = ARG_ANYTHING,
> >  };
> >
> > @@ -418,6 +419,7 @@ const struct bpf_func_proto bpf_ringbuf_discard_proto = {
> >         .func           = bpf_ringbuf_discard,
> >         .ret_type       = RET_VOID,
> >         .arg1_type      = ARG_PTR_TO_ALLOC_MEM,
> > +       .arg1_release   = true,
> >         .arg2_type      = ARG_ANYTHING,
> >  };
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 744b7362e52e..b8cd34607215 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -245,6 +245,7 @@ struct bpf_call_arg_meta {
> >         struct bpf_map *map_ptr;
> >         bool raw_mode;
> >         bool pkt_access;
> > +       u8 release_regno;
> >         int regno;
> >         int access_size;
> >         int mem_size;
> > @@ -6101,12 +6102,31 @@ static bool check_btf_id_ok(const struct bpf_func_proto *fn)
> >         return true;
> >  }
> >
> > -static int check_func_proto(const struct bpf_func_proto *fn, int func_id)
> > +static bool check_release_regno(const struct bpf_func_proto *fn, int func_id,
> > +                               struct bpf_call_arg_meta *meta)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(fn->arg_release); i++) {
> > +               if (fn->arg_release[i]) {
> > +                       if (!is_release_function(func_id))
> > +                               return false;
> > +                       if (meta->release_regno)
> > +                               return false;
> > +                       meta->release_regno = i + 1;
> > +               }
> > +       }
> > +       return !is_release_function(func_id) || meta->release_regno;
> > +}
> > +
> > +static int check_func_proto(const struct bpf_func_proto *fn, int func_id,
> > +                           struct bpf_call_arg_meta *meta)
> >  {
> >         return check_raw_mode_ok(fn) &&
> >                check_arg_pair_ok(fn) &&
> >                check_btf_id_ok(fn) &&
> > -              check_refcount_ok(fn, func_id) ? 0 : -EINVAL;
> > +              check_refcount_ok(fn, func_id) &&
> > +              check_release_regno(fn, func_id, meta) ? 0 : -EINVAL;
> >  }
> >
> >  /* Packet data might have moved, any old PTR_TO_PACKET[_META,_END]
> > @@ -6785,7 +6805,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >         memset(&meta, 0, sizeof(meta));
> >         meta.pkt_access = fn->pkt_access;
> >
> > -       err = check_func_proto(fn, func_id);
> > +       err = check_func_proto(fn, func_id, &meta);
> >         if (err) {
> >                 verbose(env, "kernel subsystem misconfigured func %s#%d\n",
> >                         func_id_name(func_id), func_id);
> > @@ -6818,8 +6838,17 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >                         return err;
> >         }
> >
> > +       regs = cur_regs(env);
> > +
> >         if (is_release_function(func_id)) {
> > -               err = release_reference(env, meta.ref_obj_id);
> > +               err = -EINVAL;
> > +               if (meta.ref_obj_id)
> > +                       err = release_reference(env, meta.ref_obj_id);
> > +               /* meta.ref_obj_id can only be 0 if register that is meant to be
> > +                * released is NULL, which must be > R0.
> > +                */
> > +               else if (meta.release_regno && register_is_null(&regs[meta.release_regno]))
> > +                       err = 0;
>
> If I'm understanding this correctly, in this patch we will call
> check_release_regno on every function to determine if any / which of
> the argument registers release a reference. Given that in the majority
> of cases the function will not be a release function, what are your
> thoughts on moving that check to be within the scope of this if
> function? So if it is a release function, and meta.ref_obj_id is not
> set, then we do the checking for which argument register is a release
> register and whether that register is null. Curious to hear your
> thoughts.
>

The suggestion looks nice, as it saves a lot of work, but my preference was to
error when the bpf_func_proto fields are incorrect (more than one arg has
argN_release == true). In this case we can still detect such a case, but it is
behind 'if (is_release_function(...))', so it wouldn't catch incorrect
bpf_func_proto of non-release functions.

So whether to do it your way would depend on whether it is considered valuable
(or defensive programming) to detect badly set up bpf_func_proto or not (we
already do it for some other cases, so it's nothing new), particularly for this
case.

>
> >                 if (err) {
> >                         verbose(env, "func %s#%d reference has not been acquired before\n",
> >                                 func_id_name(func_id), func_id);
> > @@ -6827,8 +6856,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >                 }
> >         }
> >
> > -       regs = cur_regs(env);
> > -
> >         switch (func_id) {
> >         case BPF_FUNC_tail_call:
> >                 err = check_reference_leak(env);
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 03655f2074ae..17eff4731b06 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6622,6 +6622,7 @@ static const struct bpf_func_proto bpf_sk_release_proto = {
> >         .gpl_only       = false,
> >         .ret_type       = RET_INTEGER,
> >         .arg1_type      = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> > +       .arg1_release   = true,
> >  };
> >
> >  BPF_CALL_5(bpf_xdp_sk_lookup_udp, struct xdp_buff *, ctx,
> > --
> > 2.35.1
> >

--
Kartikeya