On Wed, Nov 09, 2022 at 07:02:11AM IST, Alexei Starovoitov wrote:
> On Tue, Nov 8, 2022 at 4:36 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Nov 8, 2022 at 4:00 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> > >
> > > 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.
> >
> > Sorry, I haven't followed <v5. Don't have perfect name, but logically
> > this is BPF program memory. So "prog_kptr" would be something to
> > convert this, but I'm not super happy with such a name. "user_kptr"
> > would be too confusing, drawing incorrect "kernel space vs user space"
> > comparison, while both are kernel memory. It's BPF-side kptr, so
> > "bpf_kptr", but also not great.
>
> yep. I went through the same thinking process.
>
> > So that's why didn't suggest anything specific, but at least as far as
> > MEM_xxx flag goes, MEM_LOCAL_KPTR is better than MEM_ALLOC, IMO. It's
> > at least consistent with the official name of the concept it
> > represents.
>
> "local kptr" doesn't fit here.
> In libbpf, "local" is equally badly named.
> If "local" was a good name we wouldn't have had this discussion.
> So we need to fix it libbpf, but we should start with a proper
> name in the kernel.
> And "local kptr" is not it.
>
> We must avoid exhausting bikeshedding too.
> MEM_ALLOC is something we can use right now and
> as long as "local kptr" doesn't appear in docs, comments and
> commit logs we're good.
> We can rename MEM_ALLOC to something else later.
>
> In commit logs we can just say that this is
> a pointer to an object allocated by the bpf program.
> It's crystal clear definition whereas "local kptr" is nonsensical.
>
Ok, I'll drop the naming everywhere.
> Going back to the kptr definition.
> kptr was supposed to mean a pointer to a kernel object.
> In that light "pointer to an object allocated by the bpf prog"
> is something else.
> Maybe "bptr" ?
> In some ways bpf is a layer different from kernel space and user space.
> Some people joked that there is ring-0 for kernel, ring-3 for user space
> while bpf runs in ring-B.
> Two new btf_tags __bptr and __bptr_ref (or may be just one?)
> might be necessary as well to make it easier to distinguish
> kernel and bpf prog allocated objects.
>
There's also the option of simply using __kptr and __kptr_ref for these (without
__local tag in BPF maps) and doing two stage name lookup for the types. Kernel
BTF takes precedence, if not found there, then it searches program BTF for a
local type. It would probably the simplest for users.
struct map_value {
struct nf_conn __kptr_ref *ct; // kernel
struct foo __kptr_ref *f; // local
struct task_struct __kptr_ref *t; // kernel
struct bar __kptr_ref *b; // local
}
We can revisit this again once the post the follow up to store them in maps.
