On Fri, Sep 15, 2023 at 1:12 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Thu, Sep 14, 2023 at 5:37 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote: > > > > On Wed, Sep 13, 2023 at 7:14 AM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > 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? > > > > > > > We reach here only when the symbol part match, and likely we get the > > desired one. > > sure, but you can have multiple versions, so multiple hits of this. > And the ELF itself could be pretty big with lots of sections. So I > think we should try to minimize number of linear searches over ELF > sections, if possible. > > > if we store the pointers in struct Elf, then we have to involve > > dynamic allocations. > > I'm not sure why dynamic allocations are needed? > Your last comment says remember those version name pointer in struct Elf, I thought this would be allocate an array and save those pointers (i.e. store a mapping from version index to version name) > But also I had struct elf_sym_iter in mind, where we already cache > Elf_Data *versyms, so why not do that with verdefs as well? I think verdef is not per symbol. > all these helpers (symbol_match and elf_get_vername) are called only > from elf_sym_iter stuff right now, right? > > > > > > > > + 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. > > > > > > > Sounds good, will do. > > > > > > 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). > > > > > > > Go symbols are complicated, I will try in another patch. > > indeed, thanks! > > > > > > 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 > > > > > > > > Cheers, > > --- > > Hengqi
