On Sun, Sep 10, 2023 at 6:51 PM Hengqi Chen <hengqi.chen@xxxxxxxxx> 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 > > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx> > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx> > Signed-off-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> > --- > tools/lib/bpf/elf.c | 146 +++++++++++++++++++++++++++++++++++++---- > tools/lib/bpf/libbpf.c | 2 +- > 2 files changed, 134 insertions(+), 14 deletions(-) > > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c > index 5c9e588b17da..825da903a34c 100644 > --- a/tools/lib/bpf/elf.c > +++ b/tools/lib/bpf/elf.c > @@ -1,5 +1,8 @@ > // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) > > +#ifndef _GNU_SOURCE > +#define _GNU_SOURCE > +#endif > #include <libelf.h> > #include <gelf.h> > #include <fcntl.h> > @@ -10,6 +13,17 @@ > > #define STRERR_BUFSIZE 128 > > +/* A SHT_GNU_versym section holds 16-bit words. This bit is set if > + * the symbol is hidden and can only be seen when referenced using an > + * explicit version number. This is a GNU extension. > + */ > +#define VERSYM_HIDDEN 0x8000 > + > +/* This is the mask for the rest of the data in a word read from a > + * SHT_GNU_versym section. > + */ > +#define VERSYM_VERSION 0x7fff > + > int elf_open(const char *binary_path, struct elf_fd *elf_fd) > { > char errmsg[STRERR_BUFSIZE]; > @@ -64,11 +78,14 @@ struct elf_sym { > const char *name; > GElf_Sym sym; > GElf_Shdr sh; > + int ver; > + bool hidden; > }; > > struct elf_sym_iter { > Elf *elf; > Elf_Data *syms; > + Elf_Data *versyms; > size_t nr_syms; > size_t strtabidx; > size_t next_sym_idx; > @@ -111,6 +128,18 @@ static int elf_sym_iter_new(struct elf_sym_iter *iter, > iter->nr_syms = iter->syms->d_size / sh.sh_entsize; > iter->elf = elf; > iter->st_type = st_type; > + > + /* Version symbol table is meaningful to dynsym only */ > + if (sh_type != SHT_DYNSYM) > + return 0; > + > + scn = elf_find_next_scn_by_type(elf, SHT_GNU_versym, NULL); > + if (!scn) > + return 0; > + if (!gelf_getshdr(scn, &sh)) > + return -EINVAL; > + iter->versyms = elf_getdata(scn, 0); > + > return 0; > } > > @@ -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,113 @@ 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; > + } > return ret; > } > > return NULL; > } > > +static const char *elf_get_vername(Elf *elf, int ver) > +{ > + GElf_Verdaux verdaux; > + GElf_Verdef verdef; > + Elf_Data *verdefs; > + size_t strtabidx; > + GElf_Shdr sh; > + Elf_Scn *scn; > + int offset; > + > + scn = elf_find_next_scn_by_type(elf, SHT_GNU_verdef, NULL); so this is a linear search, right? And we'll be doing it for every .dynsym symbol. Let's do this once at the creation time and remember a pointer inside struct Elf? > + if (!scn) > + return NULL; > + if (!gelf_getshdr(scn, &sh)) > + return NULL; > + strtabidx = sh.sh_link; > + verdefs = elf_getdata(scn, 0); > + > + offset = 0; > + while (gelf_getverdef(verdefs, offset, &verdef)) { > + if (verdef.vd_ndx != ver) { > + if (!verdef.vd_next) > + break; > + > + offset += verdef.vd_next; > + continue; > + } > + > + if (!gelf_getverdaux(verdefs, offset + verdef.vd_aux, &verdaux)) > + break; > + > + return elf_strptr(elf, strtabidx, verdaux.vda_name); > + > + } > + return NULL; > +} > + > +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 gmail points out typo: qualified > + * 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) nit: ret < 0, I've spent enough time switching all users of libbpf to not rely on exact -1 return, let's not show a bad example ;) > + return false; > + > + sname_len = ret; > + ret = strncmp(sname, name, sname_len); why is this strncmp? shouldn't the match be exact? both name is version-qualified, and current ELF symbol is version-qualified. They have to exactly match, no? > + 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 +297,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 +309,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 +327,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)) ok, so let's consider what we are doing here. While previously we did a relatively expensive strstr() operation once, now we are doing it for every symbol in ELF. This might add up. Plus, we then do dynamic allocations with asprintf, which also is kind of unfortunate. But let's take a step back. Why we don't determine if the name is qualified once. Remember what is the length of unqualified name, where does the version part starts, and pass all that to symbol_match in a prepared form. Then we don't need to construct "fully qualified" form of an ELF symbol. We can compare unqual name and version name separately. No allocation, no wasted work. Not sure if we need to care whether we had "@" or "@@" in the requested symbol, but that's a detail. > 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", BTW, while you are at it. Arnaldo was trying to use this SEC("uprobe") stuff for tracing Go functions. Go doesn't seem to do any mangling, so function names can have lots of "interesting" symbols ([], @, etc). If you get a chance, would you mind updating this partsing logic to be able to accommodate such crazy function names as well? Thanks! > &probe_type, &binary_path, &func_name, &offset); > switch (n) { > case 1: > -- > 2.34.1 >
