On Sat, Sep 23, 2023 at 5:48 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > 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 > OK, It's just a sanity check. > > + 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. > > That's better. Thanks. > > + } 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 > >
