On Sun, Sep 17, 2023 at 10:50 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 | 134 +++++++++++++++++++++++++++++++++++++---- > tools/lib/bpf/libbpf.c | 2 +- > 2 files changed, 124 insertions(+), 12 deletions(-) > > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c > index 5c9e588b17da..f7ad7a7acc29 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,13 +78,18 @@ 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; > + Elf_Data *verdefs; > size_t nr_syms; > size_t strtabidx; > + size_t verdef_strtabidx; > size_t next_sym_idx; > struct elf_sym sym; > int st_type; > @@ -111,6 +130,29 @@ 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; we don't seem to use sh, why are we calling gelf_getshdr? sanity check or something? I dropped it for now, but let me know if I'm missing something > + iter->versyms = elf_getdata(scn, 0); > + > + scn = elf_find_next_scn_by_type(elf, SHT_GNU_verdef, NULL); > + if (!scn) { > + pr_debug("elf: failed to find verdef ELF sections in '%s'\n", > + binary_path); > + return -ENOENT; > + } > + if (!gelf_getshdr(scn, &sh)) > + return -EINVAL; > + iter->verdef_strtabidx = sh.sh_link; > + iter->verdefs = elf_getdata(scn, 0); > + > return 0; > } > [...] > > /* Transform symbol's virtual address (absolute for binaries and relative > * for shared libs) into file offset, which is what kernel is expecting > @@ -166,7 +277,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; > + const char *at_symbol, *lib_ver; > + bool is_shared_lib; > long ret = -ENOENT; > size_t name_len; > GElf_Ehdr ehdr; > @@ -179,9 +291,15 @@ 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; > + /* Does name specify "@@LIB_VER" or "@LIB_VER" ? */ > + at_symbol = strchr(name, '@'); > + if (at_symbol) { > + name_len = at_symbol - name; > + lib_ver = strrchr(name, '@') + 1; hm... this makes me a bit uneasy. We basically just need to skip extra @ if it's there. Reverse searching from the end seems both unnecessary and potentially error prone. So why not if (at_symbol[1] == '@') at_symbol++; lib_ver = at_symbol + 1; ? This is what I changed it to while applying, but please do let me know if it's problematic. > + } else { > + name_len = strlen(name); > + lib_ver = 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 > @@ -201,13 +319,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(&iter, sh_types[i], sym, name, name_len, lib_ver)) > 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 3a6108e3238b..b4758e54a815 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -11630,7 +11630,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 >
