Re: [PATCH dwarves] pahole: Inject kfunc decl tags into BTF

[Jiri Olsa <olsajiri@gmail.com>]

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@dxuuu.xyz>
> > > > ---
> > > >  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