Hi Eduard,
Apologies for long delay - life has been busy.
On Tue, Feb 06, 2024 at 01:31:20AM +0200, Eduard Zingerman wrote:
> On Sun, 2024-02-04 at 11:40 -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.
> >
> > 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.
> >
> > Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> > ---
>
> I've tested this patch-set using kernel built both with clang and gcc,
> on current bpf-next master (2d9a925d0fbf), both times get 124 kfunc definitions.
>
> Tested-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> Two nitpicks below.
>
> [...]
>
> > +static char *get_func_name(const char *sym)
> > +{
> > + char *func, *end;
> > +
> > + if (strncmp(sym, BTF_ID_FUNC_PFX, sizeof(BTF_ID_FUNC_PFX) - 1))
> > + return NULL;
> > +
> > + /* Strip prefix */
> > + func = strdup(sym + sizeof(BTF_ID_FUNC_PFX) - 1);
> > +
> > + /* Strip suffix */
> > + end = strrchr(func, '_');
> > + if (!end || *(end - 1) != '_') {
>
> Nit: this would do out of bounds access on malformed input
> "__BTF_ID__func___"
I think this is actually ok. Reason is we have the strncmp() above
so we know the prefix is there. Then the strdup() in the malformed cased
returns empty string. And strrchr() will then return NULL, so we don't
enter the branch.
I tested it with: https://pastes.dxuuu.xyz/c3j4kk
$ gcc test.c
dxu@kashmir~/scratch $ ./a.out
name=(null)
>
> > + free(func);
> > + return NULL;
> > + }
> > + *(end - 1) = '\0';
> > +
> > + return func;
> > +}
>
> [...]
>
> > +static int btf_encoder__tag_kfuncs(struct btf_encoder *encoder)
> > +{
>
> [...]
>
> > + elf = elf_begin(fd, ELF_C_READ, NULL);
> > + if (elf == NULL) {
> > + elf_error("Cannot update ELF file");
> > + goto out;
> > + }
> > +
> > + /* Location symbol table and .BTF_ids sections */
> > + elf_getshdrstrndx(elf, &strndx);
>
> Nit: in theory elf_getshdrstrndx() could fail and strndx would remain
> uninitialized.
Sure, will fix. Also fixing typo (Location -> Locate).
Thanks,
Daniel
