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

[Yonghong Song <yhs@meta.com>]

On 11/17/22 3:01 PM, Kumar Kartikeya Dwivedi wrote:
> On Fri, Nov 18, 2022 at 04:22:40AM IST, Yonghong Song wrote:
>>
>>
>> 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.
>>
> 
> It's fine to also leave out types in module BTFs for now, atleast as long you
> return a reasonable error message from the verifier. Just relying on btf_vmlinux
> is enough for the FIXMEs in selftests.

Okay, will leave module support at this point then.