On Tue, Jun 21, 2022 at 09:45:15PM IST, KP Singh wrote: > On Tue, Jun 21, 2022 at 6:01 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi > > <memxor@xxxxxxxxx> wrote: > > > > > > On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote: > > > > In preparation for the addition of bpf_getxattr kfunc. > > > > > > > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx> > > [...] > > > > > +++ b/kernel/bpf/btf.c > > > > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type) > > > > case BPF_PROG_TYPE_STRUCT_OPS: > > > > return BTF_KFUNC_HOOK_STRUCT_OPS; > > > > case BPF_PROG_TYPE_TRACING: > > > > + case BPF_PROG_TYPE_LSM: > > > > return BTF_KFUNC_HOOK_TRACING; > > > > > > Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register > > > for tracing or lsm progs, you write to the same hook instead, so kfunc enabled > > > for tracing progs also gets enabled for lsm, I guess that is not what user > > > intends when registering kfunc set. > > > > It's probably ok for this case. > > We might combine BTF_KFUNC_HOOK* into fewer hooks and > > I did this intentionally. We do this similarly for helpers too and we > unfortunately, we do realize that LSM hooks are not there in all > places (esp. where one is auditing stuff). > > So in reality one does use a combination of LSM and tracing hooks > with the policy decision coming from the LSM hook but the state > gets accumulated from different hooks. > > > > scope them by attach_btf_id. > > Everything is 'tracing' like at the end. > > Upcoming hid-bpf is 'tracing'. lsm is 'tracing'. > > but we may need to reduce the scope of kfuncs > > based on attach point or based on argument type. > > tc vs xdp don't need to be separate sets. > > Their types (skb vs xdp_md) are different, so only > > right kfuncs can be used, but bpf_ct_release() is needed > > in both tc and xdp. > > So we could combine tc and xdp into 'btf_kfunc_hook_networking' > > without compromising the safety. > > acquire vs release ideally would be indicated with btf_tag, > > but gcc still doesn't have support for btf_tag and we cannot > > require the kernel to be built with clang. > > acquire, release, sleepable, ret_null should be flags on a kfunc > > instead of a set. It would be easier to manage and less boilerplate > > +1 This would be awesome, I gave it a shot to use btf_tag but hit this > feature gap in GCC. > > - KP > > > code. Not clear how to do this cleanly. > > export_symbol approach is a bit heavy and requires name being unique > > across kernel and modules. > > func name mangling or typedef-ing comes to mind. not so clean either. How does this approach look to 'tag' a kfunc? https://godbolt.org/z/jGeK6Y49K Then we find the function whose symbol should be emitted and available in BTF with the prefix (__kfunc_name_) and read off the tag values. Due to the extern inline it should still inline the definition, so there is no overhead at runtime. As you see in godbolt, __foobar_1_2 symbol is visible, with 1 and 2 being tag values. Substituting tag name with underlying value makes parsing easier. We can have fixed number of slots and keep others as 0 (ignore). It can be extended to also tag arguments, which I already did before at [0], but never sent out. Then we don't need the suffixes or special naming of arguments to indicate some tag. e.g. bpf_ct_release will be: KFUNC_CALL_0(void, bpf_ct_release, __kfunc_release, KARG(struct nf_conn *, nfct, __ptr_ref)) { ... } __kfunc_acquire and __kfunc_ret_null will go in the same place as __kfunc_release. We may also allow tagging as ptr_to_btf_id or ptr_to_mem etc. to force passing a certain register type and avoid ambiguity. [0]: https://gist.github.com/kkdwivedi/7839cc9e4f002acc3e15350b1b86c88c -- Kartikeya
