| Message ID | 20230911015052.72975-3-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Support symbol versioning for uprobe | expand |
On Sun, Sep 10, 2023 at 6:51 PM Hengqi Chen <hengqi.chen@gmail.com> 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@oracle.com> > Acked-by: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- > 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 >
On Wed, Sep 13, 2023 at 7:14 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sun, Sep 10, 2023 at 6:51 PM Hengqi Chen <hengqi.chen@gmail.com> 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@oracle.com> > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > --- > > 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. if we store the pointers in struct Elf, then we have to involve dynamic allocations. > > + 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. > 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
On Thu, Sep 14, 2023 at 5:37 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On Wed, Sep 13, 2023 at 7:14 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Sun, Sep 10, 2023 at 6:51 PM Hengqi Chen <hengqi.chen@gmail.com> 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@oracle.com> > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > --- > > > 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? 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 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
On Fri, Sep 15, 2023 at 1:12 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Sep 14, 2023 at 5:37 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On Wed, Sep 13, 2023 at 7:14 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Sun, Sep 10, 2023 at 6:51 PM Hengqi Chen <hengqi.chen@gmail.com> 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@oracle.com> > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > --- > > > > 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
On Fri, Sep 15, 2023 at 12:30 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > On Fri, Sep 15, 2023 at 1:12 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Sep 14, 2023 at 5:37 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > > > On Wed, Sep 13, 2023 at 7:14 AM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Sun, Sep 10, 2023 at 6:51 PM Hengqi Chen <hengqi.chen@gmail.com> 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@oracle.com> > > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > > > > --- > > > > > 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. I meant to just not do elf_find_next_scn_by_type() search every time, and just store Elf_Scn * pointer for SHT_GNU_verdef in iter state. > > > 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; > > > > > +} [...]
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); + 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 + * 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; + + sname_len = ret; + ret = strncmp(sname, name, sname_len); + 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)) 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:
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@oracle.com> Acked-by: Jiri Olsa <jolsa@kernel.org> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/lib/bpf/elf.c | 146 +++++++++++++++++++++++++++++++++++++---- tools/lib/bpf/libbpf.c | 2 +- 2 files changed, 134 insertions(+), 14 deletions(-)