On Fri, Sep 8, 2023 at 11:07 PM Alan Maguire <alan.maguire@xxxxxxxxxx> 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@xxxxxxxxx> > > One question below, but > > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx> > 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
