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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
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().
