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

[Yonghong Song <yhs@meta.com>]

On 11/17/22 10:24 AM, Kumar Kartikeya Dwivedi wrote:
> 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 just prototyped a little bit with Alexei's suggested interface. It has 
some differences. I will explain in my next revision.

My prototype already added bpf_rdonly_cast(). As you suggested, it is
not too hard. I have not done with module btf yet. Will add it
as you suggested below.

> 
> 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.

I will focus on implementing bpf_cast_to_kern_ctx() and 
bpf_rdonly_cast(). Others can be delayed for later patches.

> 
> 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.