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
jirka
[2] ab21d6063c01 bpf: Introduce 8-byte BTF set
