Re: [PATCH] libbpf: checking the btf_type kind when fixing variable offsets

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 17, 2024 at 4:30 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 16/06/2024 01:29, Donglin Peng wrote:
> > I encountered an issue when building the test_progs using the repository[1]:
> >
> > $ clang --version
> > Ubuntu clang version 17.0.6 (++20231208085846+6009708b4367-1~exp1~20231208085949.74)
> > Target: x86_64-pc-linux-gnu
> > Thread model: posix
> > InstalledDir: /usr/bin
> >
> > $ pwd
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/
> >
> > $ make test_progs V=1
> > ...
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/tools/sbin/bpftool
> > gen object
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked2.o
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked1.o
> > libbpf: failed to find symbol for variable 'bpf_dynptr_slice' in section
> > '.ksyms'
> > Error: failed to link
> > '/work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked1.o':
> > No such file or directory (2)
> > make: *** [Makefile:656:
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.skel.h]
> > Error 254
> >
> > After investigation, I found that the btf_types in the '.ksyms' section have a kind of
> > BTF_KIND_FUNC instead of BTF_KIND_VAR:
> >
> > $ bpftool btf dump file ./ip_check_defrag.bpf.linked1.o
> > ...
> > [2] DATASEC '.ksyms' size=0 vlen=2
> >         type_id=16 offset=0 size=0 (FUNC 'bpf_dynptr_from_skb')
> >         type_id=17 offset=0 size=0 (FUNC 'bpf_dynptr_slice')
> > ...
> > [16] FUNC 'bpf_dynptr_from_skb' type_id=82 linkage=extern
> > [17] FUNC 'bpf_dynptr_slice' type_id=85 linkage=extern
> > ...
> >
> > To fix this, we can a add check for the kind.
> >
> > [1] https://github.com/eddyz87/bpf/tree/binsort-btf-dedup
> > Link: https://lore.kernel.org/all/4f551dc5fc792936ca364ce8324c0adea38162f1.camel@xxxxxxxxx/
> >
>
> The fix makes sense; what I was trying to figure out is why we're only
> seeing this now with the above repo.
>
> So as I understand it, the reason the kfuncs end up in the .ksyms
> section is due to the "__weak __ksym" tagging recently added to
> vmlinux.h construction from BTF via
>
> 770abbb5a25a ("bpftool: Support dumping kfunc prototypes from BTF")
>
> We see as noted
>
> [112] DATASEC '.ksyms' size=0 vlen=2
>         type_id=84 offset=0 size=0 (FUNC 'bpf_dynptr_from_skb')
>         type_id=90 offset=0 size=0 (FUNC 'bpf_dynptr_slice')
>
> So that makes sense, but prior to the above series, we also tagged
> kfuncs in this way before via bpf_kfuncs.h. So there should be no
> difference there.
>
> And with an upstream kernel I don't run into this problem.
>
> The only thing I could come up with is we were usually lucky; when we
> misinterpreted the func as a var and looked its type up, we got
>
>                 int var_linkage = btf_var(vt)->linkage;
>
> ...and were lucky it never equalled 1 (BTF_VAR_GLOBAL_ALLOCATED):
>
>                 /* no need to patch up static or extern vars */
>                 if (var_linkage != BTF_VAR_GLOBAL_ALLOCATED)
>                         continue;
>
> In the case of a function, the above btf_var(vt) would really be
> pointing at the struct btf_type immediately after the relevant
> function's struct btf_type (since unlike variables, functions don't have
> metadata following them). So the only way we'd trip this bug would be if
> the struct btf_type following the func was had a name_off value that
> happened to equal 1 (BTF_VAR_GLOBAL_ALLOCATED).
>
> So maybe the sorting changes to BTF order resulted in us tripping on
> this bug, but regardless the fix seems right to me.
>
> > Fixes: 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
> > Signed-off-by: Donglin Peng <dolinux.peng@xxxxxxxxx>
>
> A few small things below, but
>
> Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> > ---
> >  tools/lib/bpf/linker.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index 0d4be829551b..7f5fc9ac4ad6 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -2213,10 +2213,17 @@ static int linker_fixup_btf(struct src_obj *obj)
> >               vi = btf_var_secinfos(t);
> >               for (j = 0, m = btf_vlen(t); j < m; j++, vi++) {
> >                       const struct btf_type *vt = btf__type_by_id(obj->btf, vi->type);
> > -                     const char *var_name = btf__str_by_offset(obj->btf, vt->name_off);
> > -                     int var_linkage = btf_var(vt)->linkage;
> > +                     const char *var_name;
> > +                     int var_linkage;
> >                       Elf64_Sym *sym;
> >
> > +                     /* should be a variable */
> > +                     if (btf_kind(vt) != BTF_KIND_VAR)
>
> nit: could use if (!btf_is_var(vt)) here instead. It might also be worth
> reworking the comment to acknowledge that we can legitimately have a
> function in this section; i.e. something like
>
>                         /* could be a variable or function */
>
> We handle the func case elsewhere in libbpf (see add_dummy_ksym_var()).

Let's go a half-step further and validate that we see either VAR or
FUNC and for anything unexpected error out loudly, so this is brought
to someone's attention sooner?

Something like:

if (btf_is_func(vt))
    continue; /* FUNC doesn't need adjustment */
if (!btf_is_var(vt)) {
    pr_warn(<helpful and descriptive enough message about unexpected type>);
    return -EINVAL;
}

(use btf_kind_str() for error string, btw)

pw-bot: cr

>
>
> > +                             continue;
> > +
> > +                     var_name = btf__str_by_offset(obj->btf, vt->name_off);
> > +                     var_linkage = btf_var(vt)->linkage;
> > +
> >                       /* no need to patch up static or extern vars */
> >                       if (var_linkage != BTF_VAR_GLOBAL_ALLOCATED)
> >                               continue;





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux