Hi Eduard,
On Fri, Apr 26, 2024 at 04:47:53PM GMT, Eduard Zingerman wrote:
> On Thu, 2024-04-25 at 18:28 -0600, 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.
> >
> > Example of encoding:
> >
> > $ bpftool btf dump file .tmp_vmlinux.btf | rg "DECL_TAG 'bpf_kfunc'" | wc -l
> > 121
> >
> > $ bpftool btf dump file .tmp_vmlinux.btf | rg 56337
> > [56337] FUNC 'bpf_ct_change_timeout' type_id=56336 linkage=static
> > [127861] DECL_TAG 'bpf_kfunc' type_id=56337 component_idx=-1
> >
> > 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.
> >
> > This feature is enabled with --btf_features=decl_tag,decl_tag_kfuncs.
> >
> > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Tested-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > Tested-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> > ---
>
> I tested this patch-set on current master with Makefile.btf modified
> to have --btf_features=+decl_tag_kfuncs, the tests are passing.
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> (But please fix get_func_name...)
>
> [...]
>
> > +static char *get_func_name(const char *sym)
> > +{
> > + char *func, *end;
> > +
> > + /* Example input: __BTF_ID__func__vfs_close__1
> > + *
> > + * The goal is to strip the prefix and suffix such that we only
> > + * return vfs_close.
> > + */
> > +
> > + if (!strstarts(sym, BTF_ID_FUNC_PFX))
> > + return NULL;
> > +
> > + /* Strip prefix and handle malformed input such as __BTF_ID__func___ */
> > + func = strdup(sym + sizeof(BTF_ID_FUNC_PFX) - 1);
> > + if (strlen(func) < 2) {
> > + free(func);
> > + return NULL;
> > + }
> > +
> > + /* Strip suffix */
> > + end = strrchr(func, '_');
> > + if (!end || *(end - 1) != '_') {
>
> Sorry, I'm complaining about this silly function again...
> This will do an invalid read for input like "__BTF_ID__func___a":
> - 'func' would be a result of strdup("_a");
> - 'end' would point to the first character of 'func';
> - 'end - 1' would point outside of 'func'.
>
> Here is a repro with valgrind report:
> https://gist.github.com/eddyz87/dfd653ada6584b7b7563fbfe66355eae
Ah, yes. Good catch. I think this diff should do it:
diff --git a/btf_encoder.c b/btf_encoder.c
index 02f0cbb..6cb0c8f 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1435,7 +1435,7 @@ static char *get_func_name(const char *sym)
/* Strip prefix and handle malformed input such as __BTF_ID__func___ */
func = strdup(sym + sizeof(BTF_ID_FUNC_PFX) - 1);
- if (strlen(func) < 2) {
+ if (!strstr(func, "__")) {
free(func);
return NULL;
}
I'll respin.
Thanks,
Daniel
