Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe

From: Hengqi Chen <hengqi.chen@gmail.com>
To: Jiri Olsa <olsajiri@gmail.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	 andrii@kernel.org, alan.maguire@oracle.com
Subject: Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
Date: Sun, 10 Sep 2023 20:42:24 +0800	[thread overview]
Message-ID: <CAEyhmHQQ2CB6S0jbrZ86PGsQnc8XSrZ3v8goC3O5=tgAcn_T0A@mail.gmail.com> (raw)
In-Reply-To: <ZP2SEyyS5RFdZzaY@krava>

On Sun, Sep 10, 2023 at 5:53 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Sep 05, 2023 at 03:12:56PM +0000, Hengqi Chen wrote:
> > In current implementation, we assume that symbol found in .dynsym section
> > would have a version suffix and use it to compare with symbol user supplied.
> > According to the spec ([0]), this assumption is incorrect, the version info
> > of dynamic symbols are stored in .gnu.version and .gnu.version_d sections
> > of ELF objects. For example:
> >
> >     $ nm -D /lib/x86_64-linux-gnu/libc.so.6 | grep rwlock_wrlock
> >     000000000009b1a0 T __pthread_rwlock_wrlock@GLIBC_2.2.5
> >     000000000009b1a0 T pthread_rwlock_wrlock@@GLIBC_2.34
> >     000000000009b1a0 T pthread_rwlock_wrlock@GLIBC_2.2.5
> >
> >     $ readelf -W --dyn-syms /lib/x86_64-linux-gnu/libc.so.6 | grep rwlock_wrlock
> >       706: 000000000009b1a0   878 FUNC    GLOBAL DEFAULT   15 __pthread_rwlock_wrlock@GLIBC_2.2.5
> >       2568: 000000000009b1a0   878 FUNC    GLOBAL DEFAULT   15 pthread_rwlock_wrlock@@GLIBC_2.34
> >       2571: 000000000009b1a0   878 FUNC    GLOBAL DEFAULT   15 pthread_rwlock_wrlock@GLIBC_2.2.5
> >
> > In this case, specify pthread_rwlock_wrlock@@GLIBC_2.34 or
> > pthread_rwlock_wrlock@GLIBC_2.2.5 in bpf_uprobe_opts::func_name won't work.
> > Because the qualified name does NOT match `pthread_rwlock_wrlock` (without
> > version suffix) in .dynsym sections.
> >
> > This commit implements the symbol versioning for dynsym and allows user to
> > specify symbol in the following forms:
> >   - func
> >   - func@LIB_VERSION
> >   - func@@LIB_VERSION
> >
> > In case of symbol conflicts, error out and users should resolve it by
> > specifying a qualified name.
> >
> >   [0]: https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/symversion.html
> >
> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>
> I have a question below, but other than that
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
>
> SNIP
>
> > @@ -119,6 +148,7 @@ static struct elf_sym *elf_sym_iter_next(struct elf_sym_iter *iter)
> >       struct elf_sym *ret = &iter->sym;
> >       GElf_Sym *sym = &ret->sym;
> >       const char *name = NULL;
> > +     GElf_Versym versym;
> >       Elf_Scn *sym_scn;
> >       size_t idx;
> >
> > @@ -138,12 +168,112 @@ static struct elf_sym *elf_sym_iter_next(struct elf_sym_iter *iter)
> >
> >               iter->next_sym_idx = idx + 1;
> >               ret->name = name;
> > +             ret->ver = 0;
> > +             ret->hidden = false;
> > +
> > +             if (iter->versyms) {
> > +                     if (!gelf_getversym(iter->versyms, idx, &versym))
> > +                             continue;
> > +                     ret->ver = versym & VERSYM_VERSION;
> > +                     ret->hidden = versym & VERSYM_HIDDEN;
>
> the doc mentions value 1 being special, also I can see readelf
> code checking on that.. is that taken into account?
>

Yes. For value 1, there is a corresponding version def, so no difference.

> > +             }
> >               return ret;
> >       }
> >
> >       return NULL;
> >  }
> >
>
> SNIP