On 22/01/2022 04:04, Kumar Kartikeya Dwivedi wrote:
On Sat, Jan 22, 2022 at 04:18:13AM IST, Alexei Starovoitov wrote:
On Fri, Jan 21, 2022 at 07:39:53PM +0000, Usama Arif wrote:
This patchset is a working prototype that adds support for calling helper
functions in eBPF applications that have been defined in a kernel module.
It would require further work including code refractoring (not included in
the patchset) to rename functions, data structures and variables that are
used for kfunc as well to be appropriately renamed for module helper usage.
If the idea of module helper functions and the approach used in this patchset
is acceptable to the bpf community, I can send a follow up patchset with the
code refractoring included to make it ready for review.
Module helpers are useful as:
- They support more argument and return types when compared to module
kfunc.
What exactly is missing?
I looked at the set. I think the only difference between existing support and
this series is that you are using bpf_func_proto for argument checks, right? Is
there anything else it is adding?
We discussed whether to use bpf_func_proto for kfunc in [0], the conclusion was
that BTF has enough info that we don't have to use it. The only thing missing
is making the verifier assume type of argument from kernel BTF rather than
passed in argument register.
e.g. same argument can currently work with PTR_TO_BTF_ID and PTR_TO_MEM. On
Alexei's suggestion, we disabled the bad cases by limiting PTR_TO_MEM support
to struct with scalars. For current usecase that works fine.
I think once BTF tags are supported, we will be able to tag the function
argument as __arg_mem or __arg_btf_id and make the verifier more strict.
Same can be done for the return value (instead of ret_null_set as it is now).
[0]: https://lore.kernel.org/bpf/20211105204908.4cqxk2nbkas6bduw@xxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Hi,
Thanks for the replies and the reviews!
Yes, as Kumar said, using bpf_func_proto for argument checking and
return register setting. In check_helper_call, the argument registers
are checked (for e.g. at
https://elixir.bootlin.com/linux/latest/source/kernel/bpf/verifier.c#L5324)
and return registers are setup (for e.g. at
https://elixir.bootlin.com/linux/latest/source/kernel/bpf/verifier.c#L6551)
according to the arg_type and ret_type in bpf_func_proto. But i don't
see the checks and setups done in check_kfunc_call for *all*
bpf_return_type and bpf_reg_type.
I do see now that PTR_TO_BTF_ID ret_type was added to kfunc last week,
but in addition to Kumars answer I think there are also more types that
might not be handled.
For e.g. the return register types for RET_PTR_TO_SOCKET vs
RET_PTR_TO_TCP_SOCK are set to different values depending on ret_type in
bpf_func_proto. Similarly in check_func_arg called by check_helper_call
only, the checks are different whether its ARG_PTR_TO_MAP_VALUE or
ARG_PTR_TO_UNINIT_MAP_VALUE or other argument type. These aren't done in
check_kfunc_call as there is no bpf_func_proto to distinguish between
them. But maybe i am missing something over here?
I guess the other way to support the additional types above is as Kumar
said by using BTF tags (I guess they they are being worked on as I didnt
see any mention of it in code?) with kfunc. But wouldn't it be better to
have consistency between the kfunc and helper functions in how different
arguments and return types are handled? Dealing with many btf tags which
will give same information about argument and return types as
bpf_func_proto will likely just result in repeated code in
check_helper_call and check_kfunc_call and further code to handle the tags?
- This adds a way to have helper functions that would be too specialized
for a specific usecase to merge upstream, but are functions that can have
a constant API and can be maintained in-kernel modules.
Could you give an example of something that would be "too specialized" that
it's worth maintaining in a module, but not worth maintaining in the kernel?
Also see:
https://www.kernel.org/doc/html/latest/bpf/bpf_design_QA.html#q-new-functionality-via-kernel-modules
Why do you think we made that rule years ago?
I guess the rule was made (I might be missing something over here!) as
by moving functionality to kernel modules it will determine whether the
eBPF application will work or not depending on the module, which makes
the application less compatible across different systems, which is
definitely not good. But i believe the above point does not remain since
module kfuncs were introduced. I think that helpers on a very high level
are similar to kfuncs with function protos for argument and return
checking (ignoring implementation details of using helper id in
bpf_helper_defs.h vs using ksysm section with BTF id in kfunc). So i
think module helpers have the same affect from a compatibility
perspective when compared to module kfunc, with the advantages of having
bpf_func_proto.
Thanks again for the replies! I guess only worth addressing the comments
and sending further patchsets with refractor for kfunc, if the idea is
acceptable to the community and maintainers.
Thanks,
Usama
