On Wed, Jan 03, 2024 at 09:48:09AM +0100, Jiri Olsa wrote:
> On Tue, Jan 02, 2024 at 05:56:02PM -0700, Daniel Xu wrote:
> > On Thu, Dec 21, 2023 at 09:35:28AM +0100, Jiri Olsa wrote:
> > > On Wed, Dec 20, 2023 at 11:37:01PM -0700, Daniel Xu wrote:
> > > > On Wed, Dec 20, 2023 at 03:19:52PM -0700, Daniel Xu wrote:
> > > > > This commit teaches pahole to parse symbols in .BTF_ids section in
> > > > > vmlinux and discover exported kfuncs. Pahole then takes the list of
> > > > > kfuncs and injects a BTF_KIND_DECL_TAG for each kfunc.
> > > > >
> > > > > This enables downstream users and tools to dynamically discover which
> > > > > kfuncs are available on a system by parsing vmlinux or module BTF, both
> > > > > available in /sys/kernel/btf.
> > > > >
> > > > > Example of encoding:
> > > > >
> > > > > $ bpftool btf dump file .tmp_vmlinux.btf | rg DECL_TAG | wc -l
> > > > > 388
> > > > >
> > > > > $ bpftool btf dump file .tmp_vmlinux.btf | rg 68940
> > > > > [68940] FUNC 'bpf_xdp_get_xfrm_state' type_id=68939 linkage=static
> > > > > [128124] DECL_TAG 'kfunc' type_id=68940 component_idx=-1
> > > > >
> > > > > Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> > > > > ---
> > > > > btf_encoder.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 202 insertions(+)
> > > > >
> > > >
> > > > Hmm, looking more, seems like this will pick up non-kfunc functions as
> > > > well. For example, kernel/trace/bpf_trace.c:
> > > >
> > > >
> > > > BTF_SET_START(btf_allowlist_d_path)
> > > > #ifdef CONFIG_SECURITY
> > > > BTF_ID(func, security_file_permission)
> > > > BTF_ID(func, security_inode_getattr)
> > > > BTF_ID(func, security_file_open)
> > > > #endif
> > > > #ifdef CONFIG_SECURITY_PATH
> > > > BTF_ID(func, security_path_truncate)
> > > > #endif
> > > > BTF_ID(func, vfs_truncate)
> > > > BTF_ID(func, vfs_fallocate)
> > > > BTF_ID(func, dentry_open)
> > > > BTF_ID(func, vfs_getattr)
> > > > BTF_ID(func, filp_close)
> > > > BTF_SET_END(btf_allowlist_d_path)
> > >
> > > you need to pick up only 'BTF_ID(func, ...)' IDs that belongs to SET8 lists,
> > > which are bounded by __BTF_ID__set8__<name> symbols, which also provide size
> > >
> > > __BTF_ID__func_* symbol that has address inside the SET8 list is kfunc
> >
> > I managed to add that logic. But I did some spot checks and it looks
> > like SET8 lists are not quite limited to kfuncs. For example, in
> > net/mptcp/bpf.c:
> >
> > BTF_SET8_START(bpf_mptcp_fmodret_ids)
> > BTF_ID_FLAGS(func, update_socket_protocol)
> > BTF_SET8_END(bpf_mptcp_fmodret_ids)
> >
> > static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
> > .owner = THIS_MODULE,
> > .set = &bpf_mptcp_fmodret_ids,
> > };
> >
> > And in net/socket.c:
> >
> > __bpf_hook_start();
> > __weak noinline int update_socket_protocol(int family, int type, int protocol)
> > {
> > return protocol;
> > }
> > __bpf_hook_end();
> >
> > IOW, update_socket_protocol() is a hook, not a kfunc.
>
> hum, right.. we use kfuncs set8 registration now also to mark attachable
> hooks for fmodret programs, see [1]
>
> there are similar hooks registered in HID code as well
>
> [1] 5b481acab4ce bpf: do not rely on ALLOW_ERROR_INJECTION for fmod_ret)
>
> >
> > >
> > > >
> > > > Maybe we need a codemod from:
> > > >
> > > > BTF_ID(func, ...
> > > >
> > > > to:
> > > >
> > > > BTF_ID(kfunc, ...
> > >
> > > I think it's better to keep just 'func' and not to do anything special for
> > > kfuncs in resolve_btfids logic to keep it simple
> > >
> > > also it's going to be already in pahole so if we want to make a fix in future
> > > you need to change pahole, resolve_btfids and possibly also kernel
> >
> > So maybe special annotation is still needed. WDYT?
>
> anyway, it looks like we actually do have flags field in set8 (thanks Kumar! ;-) )
>
> struct btf_id_set8 {
> u32 cnt;
> u32 flags;
> struct {
> u32 id;
> u32 flags;
> } pairs[];
> };
>
> it's not mentioned in the commit changelog [2], but it looks like it was
> added just to keep data aligned, and AFAICS it's not used anywhere
>
> how about we add a flag saying this set8 has kfuncs in it
Nice, yeah. This makes sense. We can tag it at the BTF_SET8_START level
to reduce churn. At same time, we can WARN_ON() if the flag is missing
in register_btf_kfunc_id_set().
Thanks,
Daniel
