Re: [RFC PATCH bpf-next 2/3] bpf: Implement bpf_get_kern_btf_id() kfunc

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

On Wed, Nov 16, 2022 at 01:56:14AM IST, Yonghong Song wrote:
>
>
> On 11/15/22 12:05 PM, Kumar Kartikeya Dwivedi wrote:
> > On Wed, Nov 16, 2022 at 01:13:08AM IST, Alexei Starovoitov wrote:
> > > On Mon, Nov 14, 2022 at 08:23:39AM -0800, Yonghong Song wrote:
> > > > The signature of bpf_get_kern_btf_id() function looks like
> > > >    void *bpf_get_kern_btf_id(obj, expected_btf_id)
> > > > The obj has a pointer type. The expected_btf_id is 0 or
> > > > a btf id to be returned by the kfunc. The function
> > > > currently supports two kinds of obj:
> > > >    - obj: ptr_to_ctx, expected_btf_id: 0
> > > >      return the expected kernel ctx btf id
> > > >    - obj: ptr to char/unsigned char, expected_btf_id: a struct btf id
> > > >      return expected_btf_id
> > > > The second case looks like a type casting, e.g., in kernel we have
> > > >    #define skb_shinfo(SKB) ((struct skb_shared_info *)(skb_end_pointer(SKB)))
> > > > bpf program can get a skb_shared_info btf id ptr with bpf_get_kern_btf_id()
> > > > kfunc.
> > >
> > > Kumar has proposed
> > > bpf_rdonly_cast(any_64bit_value, btf_id) -> PTR_TO_BTF_ID | PTR_UNTRUSTED.
> > > The idea of bpf_get_kern_btf_id(ctx) looks complementary.
> > > The bpf_get_kern_btf_id name is too specific imo.
> > > How about two kfuncs:
> > >
> > > bpf_cast_to_kern_ctx(ctx) -> ptr_to_btf_id | ptr_trusted
> > > bpf_rdonly_cast(any_scalar, btf_id) -> ptr_to_btf_id | ptr_untrusted
>
> Sounds good. Two helpers can make sense as it is indeed true for
> bpf_cast_to_kern_ctx(ctx), the btf_id is not needed.
>
> > >
> > > ptr_trusted flag will have semantics as discsused with David and Kumar in:
> > > https://lore.kernel.org/bpf/CAADnVQ+KZcFZdC=W_qZ3kam9yAjORtpN-9+Ptg_Whj-gRxCZNQ@mail.gmail.com/
> > >
> > > The verifier knows how to cast safe pointer 'ctx' to kernel 'mirror' structure.
> > > No need for additional btf_id argument.
> > > We can express it as ptr_to_btf_id | ptr_trusted and safely pass to kfuncs.
> > > bpf_rdonly_cast() can accept any 64-bit value.
> > > There is no need to limit it to 'char *' arg. Since it's ptr_to_btf_id | ptr_untrusted
> > > it cannot be passed to kfuncs and only rdonly acccess is allowed.
> > > Both kfuncs need to be cap_perfmon gated, of course.
> > > Thoughts?
>
> Currently, we only have SCALAR_VALUE to represent 'void *', 'char *',
> 'unsigned char *'. yes, some pointer might be long and cast to 'struct foo
> *', so the generalization of bpf_rdonly_cast() to all scalar value
> should be fine. Although it is possible the it might be abused and incuring
> some exception handling, but guarding it with cap_perfmon
> should be okay.
>
> >
> > Here is the PoC I wrote when we discussed this:
> > It still uses bpf_unsafe_cast naming, but that was before Alexei suggested the
> > bpf_rdonly_cast name.
> > https://github.com/kkdwivedi/linux/commits/unsafe-cast (see the 2 latest commits)
> > The selftest showcases how it will be useful.
>
> Sounds good. I can redo may patch for bpf_cast_to_kern_ctx(), which should
> cover some of existing cases. Kumar, since you are working on
> bpf_rdonly_cast(), you could work on that later. If you want me to do it,
> just let me know I can incorporate it in my patch set.

I think the patch itself is pretty trivial. What's needed is a bit of
refactoring, since I would also want to make this work for module BTF types.

In that case, we need to take a type in prog BTF, look it up in the kernel, and
mark the reg using looked up BTF and BTF ID. However this raises module BTF
reference, and it needs to be kept until verifier is done (as it gets set to
reg->btf).

This is why the helper takes local type ID instead of bpf_core_type_id_kernel,
since that doesn't work for module types (IIUC).

Instead of the current used_btfs array logic, Alexei suggested guarding module
BTF free path with a rwsem, which the verifier can hold in bpf_check, so that we
don't have to worry about keeping module BTF references around during verification.
Modules are loaded/unloaded infrequently so it should be fine.

Then it also became clear we currently stash BTFs in some places unecessarily
and we could simply drop those after prog is verified. So it would make sense
to drop those cases too (kfunc_btf_tab, used_btfs btf_mod_pair, etc.). After
verification the prog only needs to pin the module references, not mod BTF
references.

Maybe all of this does not have to be done together.

So let me know if you want to take it, I have no problems with that, otherwise I
can get to it once I am done with the linked list and dynptr stuff.