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

From: Hengqi Chen <hengqi.chen@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	 andrii@kernel.org, olsajiri@gmail.com
Subject: Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
Date: Sun, 10 Sep 2023 20:39:10 +0800	[thread overview]
Message-ID: <CAEyhmHQUdy03=v-1T-uh20_bSZ0EaVY8rqNXKu9awdO4XZdRLg@mail.gmail.com> (raw)
In-Reply-To: <a219fa4f-1b25-e6c2-bd73-59b475118999@oracle.com>

On Fri, Sep 8, 2023 at 11:07 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 05/09/2023 16:12, 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>
>
> One question below, but
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>

Thanks.

> > ---
> >  tools/lib/bpf/elf.c    | 145 +++++++++++++++++++++++++++++++++++++----

> > +
> > +static bool symbol_match(Elf *elf, int sh_type, struct elf_sym *sym, const char *name)
> > +{
> > +     size_t name_len, sname_len;
> > +     bool is_name_qualified;
> > +     const char *ver;
> > +     char *sname;
> > +     int ret;
> > +
> > +     name_len = strlen(name);
> > +     /* Does name specify "@LIB" or "@@LIB" ? */
> > +     is_name_qualified = strstr(name, "@") != NULL;
> > +
> > +     /* If user specify a qualified name, for dynamic symbol,
> > +      * it is in form of func, NOT func@LIB_VER or func@@LIB_VER.
> > +      * So construct a full quailified symbol name using versym info
> > +      * for comparison.
> > +      */
> > +     if (is_name_qualified && sh_type == SHT_DYNSYM) {
> > +             /* Make sure func match func@LIB_VER */
> > +             sname_len = strlen(sym->name);
> > +             if (strncmp(sym->name, name, sname_len) != 0)
> > +                     return false;
> > +
> > +             /* But not func2@LIB_VER */
> > +             if (name[sname_len] != '@')
> > +                     return false;
> > +
> > +             ver = elf_get_vername(elf, sym->ver);
> > +             if (!ver)
> > +                     return false;
> > +
> > +             ret = asprintf(&sname, "%s%s%s", sym->name,
> > +                            sym->hidden ? "@" : "@@", ver);
> > +             if (ret == -1)
> > +                     return false;
> > +
> > +             ret = strncmp(sname, name, name_len);
>
> I _think_ because we're using the length of the name we're searching for
> we'd end up matching pthread_rwlock_wrlock@@G and
> pthread_rwlock_wrlock@@GLIBC_2.34 ; should we use strlen(sname) to do
> an exact match here?
>

Good point, will do.

>
> > +             free(sname);
> > +             return ret == 0;
> > +     }
> > +
> > +     /* Otherwise, for normal symbols or non-qualified names
> > +      * User can specify func, func@@LIB or func@@LIB_VERSION.
> > +      */
> > +     if (strncmp(sym->name, name, name_len) != 0)
> > +             return false;
> > +     /* ...but we don't want a search for "foo" to match 'foo2" also, so any
> > +      * additional characters in sname should be of the form "@LIB" or "@@LIB".
> > +      */
> > +     if (!is_name_qualified && sym->name[name_len] != '\0' && sym->name[name_len] != '@')
> > +             return false;
> > +
> > +     return true;
> > +}
> >
> >  /* Transform symbol's virtual address (absolute for binaries and relative
> >   * for shared libs) into file offset, which is what kernel is expecting
> > @@ -166,9 +296,8 @@ static unsigned long elf_sym_offset(struct elf_sym *sym)
> >  long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
> >  {
> >       int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
> > -     bool is_shared_lib, is_name_qualified;
> > +     bool is_shared_lib;
> >       long ret = -ENOENT;
> > -     size_t name_len;
> >       GElf_Ehdr ehdr;
> >
> >       if (!gelf_getehdr(elf, &ehdr)) {
> > @@ -179,10 +308,6 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
> >       /* for shared lib case, we do not need to calculate relative offset */
> >       is_shared_lib = ehdr.e_type == ET_DYN;
> >
> > -     name_len = strlen(name);
> > -     /* Does name specify "@@LIB"? */
> > -     is_name_qualified = strstr(name, "@@") != NULL;
> > -
> >       /* Search SHT_DYNSYM, SHT_SYMTAB for symbol. This search order is used because if
> >        * a binary is stripped, it may only have SHT_DYNSYM, and a fully-statically
> >        * linked binary may not have SHT_DYMSYM, so absence of a section should not be
> > @@ -201,13 +326,7 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
> >                       goto out;
> >
> >               while ((sym = elf_sym_iter_next(&iter))) {
> > -                     /* User can specify func, func@@LIB or func@@LIB_VERSION. */
> > -                     if (strncmp(sym->name, name, name_len) != 0)
> > -                             continue;
> > -                     /* ...but we don't want a search for "foo" to match 'foo2" also, so any
> > -                      * additional characters in sname should be of the form "@@LIB".
> > -                      */
> > -                     if (!is_name_qualified && sym->name[name_len] != '\0' && sym->name[name_len] != '@')
> > +                     if (!symbol_match(elf, sh_types[i], sym, name))
> >                               continue;
> >
> >                       cur_bind = GELF_ST_BIND(sym->sym.st_info);
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 96ff1aa4bf6a..30b8f96820a7 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -11512,7 +11512,7 @@ static int attach_uprobe(const struct bpf_program *prog, long cookie, struct bpf
> >
> >       *link = NULL;
> >
> > -     n = sscanf(prog->sec_name, "%m[^/]/%m[^:]:%m[a-zA-Z0-9_.]+%li",
> > +     n = sscanf(prog->sec_name, "%m[^/]/%m[^:]:%m[a-zA-Z0-9_.@]+%li",
> >                  &probe_type, &binary_path, &func_name, &offset);
> >       switch (n) {
> >       case 1:
> > --
> > 2.34.1