Re: [PATCH bpf-next v5 06/25] bpf: Introduce local kptrs

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

On Wed, Nov 09, 2022 at 04:59:41AM IST, Andrii Nakryiko wrote:
> On Mon, Nov 7, 2022 at 3:10 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > Introduce local kptrs, i.e. PTR_TO_BTF_ID that point to a type in
> > program BTF. This is indicated by the presence of MEM_ALLOC type flag in
> > reg->type to avoid having to check btf_is_kernel when trying to match
> > argument types in helpers.
> >
> > Refactor btf_struct_access callback to just take bpf_reg_state instead
> > of btf and btf_type paramters. Note that the call site in
> > check_map_access now simulates access to a PTR_TO_BTF_ID by creating a
> > dummy reg on stack. Since only the type, btf, and btf_id of the register
> > matter for the checks, it can be done so without complicating the usual
> > cases elsewhere in the verifier where reg->btf and reg->btf_id is used
> > verbatim.
> >
> > Whenever walking such types, any pointers being walked will always yield
> > a SCALAR instead of pointer. In the future we might permit kptr inside
> > local kptr (either kernel or local), and it would be permitted only in
> > that case.
> >
> > For now, these local kptrs will always be referenced in verifier
> > context, hence ref_obj_id == 0 for them is a bug. It is allowed to write
> > to such objects, as long fields that are special are not touched
> > (support for which will be added in subsequent patches). Note that once
> > such a local kptr is marked PTR_UNTRUSTED, it is no longer allowed to
> > write to it.
> >
> > No PROBE_MEM handling is therefore done for loads into this type unless
> > PTR_UNTRUSTED is part of the register type, since they can never be in
> > an undefined state, and their lifetime will always be valid.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf.h              | 28 ++++++++++++++++--------
> >  include/linux/filter.h           |  8 +++----
> >  kernel/bpf/btf.c                 | 16 ++++++++++----
> >  kernel/bpf/verifier.c            | 37 ++++++++++++++++++++++++++------
> >  net/bpf/bpf_dummy_struct_ops.c   | 14 ++++++------
> >  net/core/filter.c                | 34 ++++++++++++-----------------
> >  net/ipv4/bpf_tcp_ca.c            | 13 ++++++-----
> >  net/netfilter/nf_conntrack_bpf.c | 17 ++++++---------
> >  8 files changed, 99 insertions(+), 68 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index afc1c51b59ff..75dbd2ecf80a 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -524,6 +524,11 @@ enum bpf_type_flag {
> >         /* Size is known at compile time. */
> >         MEM_FIXED_SIZE          = BIT(10 + BPF_BASE_TYPE_BITS),
> >
> > +       /* MEM is of a type from program BTF, not kernel BTF. This is used to
> > +        * tag PTR_TO_BTF_ID allocated using bpf_obj_new.
> > +        */
> > +       MEM_ALLOC               = BIT(11 + BPF_BASE_TYPE_BITS),
> > +
>
> you fixed one naming confusion with RINGBUF and basically are creating
> a new one, where "ALLOC" means "local kptr"... If we are stuck with
> "local kptr" (which I find very confusing as well, but that's beside
> the point), why not stick to the whole "local" terminology here?
> MEM_LOCAL?
>

See the discussion about this in v4:
https://lore.kernel.org/bpf/20221104075113.5ighwdvero4mugu7@apollo

It was MEM_TYPE_LOCAL before. Also, better naming suggestions are always
welcome, I asked the same in that message as well.

> >         __BPF_TYPE_FLAG_MAX,
> >         __BPF_TYPE_LAST_FLAG    = __BPF_TYPE_FLAG_MAX - 1,
> >  };
> > @@ -771,6 +776,7 @@ struct bpf_prog_ops {
> >                         union bpf_attr __user *uattr);
> >  };
> >
>
> [...]
>
> > -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> > -                     const struct btf_type *t, int off, int size,
> > -                     enum bpf_access_type atype __maybe_unused,
> > +int btf_struct_access(struct bpf_verifier_log *log,
> > +                     const struct bpf_reg_state *reg,
> > +                     int off, int size, enum bpf_access_type atype __maybe_unused,
> >                       u32 *next_btf_id, enum bpf_type_flag *flag)
> >  {
> > +       const struct btf *btf = reg->btf;
> >         enum bpf_type_flag tmp_flag = 0;
> > +       const struct btf_type *t;
> > +       u32 id = reg->btf_id;
> >         int err;
> > -       u32 id;
> >
> > +       t = btf_type_by_id(btf, id);
> >         do {
> >                 err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag);
> >
> >                 switch (err) {
> >                 case WALK_PTR:
> > +                       /* For local types, the destination register cannot
> > +                        * become a pointer again.
> > +                        */
> > +                       if (type_is_local_kptr(reg->type))
> > +                               return SCALAR_VALUE;
>
> passing the entire bpf_reg_state just to differentiate between local
> vs kernel pointer seems like a huge overkill. bpf_reg_state is quite a
> complicated and extensive amount of state, and it seems cleaner to
> just pass it as a flag whether to allow pointer chasing or not. At
> least then we know we only care about that specific aspect, not about
> dozens of other possible fields of bpf_reg_state.
>

I agree that the separation is usually better, especially because this is also a
callback. I don't feel too strong about this though, we certainly do pass the
whole reg to functions which only work on a specific type of pointer. Though the
concern in this case is justified as it's not only an internal function but also
a callback.

It was just a bool in the RFC.
But in https://lore.kernel.org/bpf/20220907233023.x3uclwlnjuhftvtb@macbook-pro-4.dhcp.thefacebook.com
Alexei suggested passing reg instead.
From the link:
> imo it's cleaner to pass 'reg' instead of 'reg->btf',
> so we don't have to pass another boolean.
> And check type_is_local(reg) inside btf_struct_access().