* [PATCH bpf-next v2 0/3] libbpf: Support symbol versioning for uprobe
@ 2023-09-05 15:12 Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset " Hengqi Chen
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Hengqi Chen @ 2023-09-05 15:12 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, alan.maguire, olsajiri, hengqi.chen
Dynamic symbols in shared library may have the same name, 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
There are two pthread_rwlock_wrlock symbols in libc.so .dynsym section.
The one with @@ is the default version, the other is hidden.
Note that the version info is stored in .gnu.version and .gnu.version_d
sections of libc and the two symbols are at the _same_ offset.
Currently, specify `pthread_rwlock_wrlock`, `pthread_rwlock_wrlock@@GLIBC_2.34`
or `pthread_rwlock_wrlock@GLIBC_2.2.5` in bpf_uprobe_opts::func_name won't work.
Because there are two `pthread_rwlock_wrlock` in .dynsym sections without the
version suffix and both are global bind.
We could solve this by introducing symbol versioning ([0]). So that users can
specify func, func@LIB_VERSION or func@@LIB_VERSION to attach a uprobe.
This patchset resolves symbol conflicts and add symbol versioning for uprobe.
- Patch 1 resolves symbol conflicts at the same offset
- Patch 2 adds symbol versioning for dynsym
- Patch 3 adds selftests for the above changes
Changes from v1:
- Address comments from Alan and Jiri
- Add selftests (Someone reminds me that there is an attempt at [1]
and part of the selftest code from Andrii is taken from there)
[0]: https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/symversion.html
[1]: https://lore.kernel.org/lkml/CAEf4BzZTrjjyyOm3ak9JsssPSh6T_ZmGd677a2rt5e5rBLUrpQ@mail.gmail.com/
Hengqi Chen (3):
libbpf: Resolve symbol conflicts at the same offset for uprobe
libbpf: Support symbol versioning for uprobe
selftests/bpf: Add tests for symbol versioning for uprobe
tools/lib/bpf/elf.c | 150 ++++++++++++++++--
tools/lib/bpf/libbpf.c | 2 +-
tools/testing/selftests/bpf/Makefile | 5 +-
.../testing/selftests/bpf/liburandom_read.map | 15 ++
.../testing/selftests/bpf/prog_tests/uprobe.c | 95 +++++++++++
.../testing/selftests/bpf/progs/test_uprobe.c | 61 +++++++
tools/testing/selftests/bpf/urandom_read.c | 9 ++
.../testing/selftests/bpf/urandom_read_lib1.c | 41 +++++
8 files changed, 361 insertions(+), 17 deletions(-)
create mode 100644 tools/testing/selftests/bpf/liburandom_read.map
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe.c
create mode 100644 tools/testing/selftests/bpf/progs/test_uprobe.c
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset for uprobe
2023-09-05 15:12 [PATCH bpf-next v2 0/3] libbpf: Support symbol versioning for uprobe Hengqi Chen
@ 2023-09-05 15:12 ` Hengqi Chen
2023-09-08 14:30 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
2023-09-05 15:12 ` [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning " Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for " Hengqi Chen
2 siblings, 2 replies; 12+ messages in thread
From: Hengqi Chen @ 2023-09-05 15:12 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, alan.maguire, olsajiri, hengqi.chen
Dynamic symbols in shared library may have the same name, 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
Currently, users can't attach a uprobe to pthread_rwlock_wrlock because
there are two symbols named pthread_rwlock_wrlock and both are global
bind. And libbpf considers it as a conflict.
Since both of them are at the same offset we could accept one of them
harmlessly. Note that we already does this in elf_resolve_syms_offsets.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
tools/lib/bpf/elf.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
index 9d0296c1726a..5c9e588b17da 100644
--- a/tools/lib/bpf/elf.c
+++ b/tools/lib/bpf/elf.c
@@ -214,7 +214,10 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
if (ret > 0) {
/* handle multiple matches */
- if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
+ if (elf_sym_offset(sym) == ret) {
+ /* same offset, no problem */
+ continue;
+ } else if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
/* Only accept one non-weak bind. */
pr_warn("elf: ambiguous match for '%s', '%s' in '%s'\n",
sym->name, name, binary_path);
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
2023-09-05 15:12 [PATCH bpf-next v2 0/3] libbpf: Support symbol versioning for uprobe Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset " Hengqi Chen
@ 2023-09-05 15:12 ` Hengqi Chen
2023-09-08 15:07 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
2023-09-05 15:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for " Hengqi Chen
2 siblings, 2 replies; 12+ messages in thread
From: Hengqi Chen @ 2023-09-05 15:12 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, alan.maguire, olsajiri, hengqi.chen
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@gmail.com>
---
tools/lib/bpf/elf.c | 145 +++++++++++++++++++++++++++++++++++++----
tools/lib/bpf/libbpf.c | 2 +-
2 files changed, 133 insertions(+), 14 deletions(-)
diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
index 5c9e588b17da..2d2819b7e018 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,112 @@ 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;
+
+ ret = strncmp(sname, name, name_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 +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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for symbol versioning for uprobe
2023-09-05 15:12 [PATCH bpf-next v2 0/3] libbpf: Support symbol versioning for uprobe Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset " Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning " Hengqi Chen
@ 2023-09-05 15:12 ` Hengqi Chen
2023-09-08 14:40 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
2 siblings, 2 replies; 12+ messages in thread
From: Hengqi Chen @ 2023-09-05 15:12 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, alan.maguire, olsajiri, hengqi.chen
This exercises the newly added dynsym symbol versioning logics.
Now we accept symbols in form of func, func@LIB_VERSION or
func@@LIB_VERSION.
The test rely on liburandom_read.so. For liburandom_read.so, we have:
$ nm -D liburandom_read.so
w __cxa_finalize@GLIBC_2.17
w __gmon_start__
w _ITM_deregisterTMCloneTable
w _ITM_registerTMCloneTable
0000000000000000 A LIBURANDOM_READ_1.0.0
0000000000000000 A LIBURANDOM_READ_2.0.0
000000000000081c T urandlib_api@@LIBURANDOM_READ_2.0.0
0000000000000814 T urandlib_api@LIBURANDOM_READ_1.0.0
0000000000000824 T urandlib_api_sameoffset@LIBURANDOM_READ_1.0.0
0000000000000824 T urandlib_api_sameoffset@@LIBURANDOM_READ_2.0.0
000000000000082c T urandlib_read_without_sema@@LIBURANDOM_READ_1.0.0
00000000000007c4 T urandlib_read_with_sema@@LIBURANDOM_READ_1.0.0
0000000000011018 D urandlib_read_with_sema_semaphore@@LIBURANDOM_READ_1.0.0
For `urandlib_api`, specifying `urandlib_api` will cause a conflict because
there are two symbols named urandlib_api and both are global bind.
For `urandlib_api_sameoffset`, there are also two symbols in the .so, but
both are at the same offset and essentially they refer to the same function
so no conflict.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
tools/testing/selftests/bpf/Makefile | 5 +-
.../testing/selftests/bpf/liburandom_read.map | 15 +++
.../testing/selftests/bpf/prog_tests/uprobe.c | 95 +++++++++++++++++++
.../testing/selftests/bpf/progs/test_uprobe.c | 61 ++++++++++++
tools/testing/selftests/bpf/urandom_read.c | 9 ++
.../testing/selftests/bpf/urandom_read_lib1.c | 41 ++++++++
6 files changed, 224 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/liburandom_read.map
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe.c
create mode 100644 tools/testing/selftests/bpf/progs/test_uprobe.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index edef49fcd23e..49ff9c716b79 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -193,11 +193,12 @@ endif
# Filter out -static for liburandom_read.so and its dependent targets so that static builds
# do not fail. Static builds leave urandom_read relying on system-wide shared libraries.
-$(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
+$(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c liburandom_read.map
$(call msg,LIB,,$@)
$(Q)$(CLANG) $(filter-out -static,$(CFLAGS) $(LDFLAGS)) \
- $^ $(filter-out -static,$(LDLIBS)) \
+ $(filter %.c,$^) $(filter-out -static,$(LDLIBS)) \
-fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
+ -Wl,--version-script=liburandom_read.map \
-fPIC -shared -o $@
$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
diff --git a/tools/testing/selftests/bpf/liburandom_read.map b/tools/testing/selftests/bpf/liburandom_read.map
new file mode 100644
index 000000000000..38a97a419a04
--- /dev/null
+++ b/tools/testing/selftests/bpf/liburandom_read.map
@@ -0,0 +1,15 @@
+LIBURANDOM_READ_1.0.0 {
+ global:
+ urandlib_api;
+ urandlib_api_sameoffset;
+ urandlib_read_without_sema;
+ urandlib_read_with_sema;
+ urandlib_read_with_sema_semaphore;
+ local:
+ *;
+};
+
+LIBURANDOM_READ_2.0.0 {
+ global:
+ urandlib_api;
+} LIBURANDOM_READ_1.0.0;
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe.c b/tools/testing/selftests/bpf/prog_tests/uprobe.c
new file mode 100644
index 000000000000..6c619bcca41c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include <test_progs.h>
+#include "test_uprobe.skel.h"
+
+static FILE *urand_spawn(int *pid)
+{
+ FILE *f;
+
+ /* urandom_read's stdout is wired into f */
+ f = popen("./urandom_read 1 report-pid", "r");
+ if (!f)
+ return NULL;
+
+ if (fscanf(f, "%d", pid) != 1) {
+ pclose(f);
+ errno = EINVAL;
+ return NULL;
+ }
+
+ return f;
+}
+
+static int urand_trigger(FILE **urand_pipe)
+{
+ int exit_code;
+
+ /* pclose() waits for child process to exit and returns their exit code */
+ exit_code = pclose(*urand_pipe);
+ *urand_pipe = NULL;
+
+ return exit_code;
+}
+
+void test_uprobe(void)
+{
+ LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
+ struct test_uprobe *skel;
+ FILE *urand_pipe = NULL;
+ int urand_pid = 0, err;
+
+ skel = test_uprobe__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel_open"))
+ return;
+
+ urand_pipe = urand_spawn(&urand_pid);
+ if (!ASSERT_OK_PTR(urand_pipe, "urand_spawn"))
+ goto cleanup;
+
+ skel->bss->my_pid = urand_pid;
+
+ /* Manual attach uprobe to urandlib_api
+ * There are two `urandlib_api` symbols in .dynsym section:
+ * - urandlib_api@LIBURANDOM_READ_1.0.0
+ * - urandlib_api@LIBURANDOM_READ_1.0.0
+ * Both are global bind and would cause a conflict if user
+ * specify the symbol name without a version suffix
+ */
+ uprobe_opts.func_name = "urandlib_api";
+ skel->links.test4 = bpf_program__attach_uprobe_opts(skel->progs.test4,
+ urand_pid,
+ "./liburandom_read.so",
+ 0 /* offset */,
+ &uprobe_opts);
+ if (!ASSERT_ERR_PTR(skel->links.test4, "urandlib_api_attach_conflict"))
+ goto cleanup;
+
+ uprobe_opts.func_name = "urandlib_api@LIBURANDOM_READ_1.0.0";
+ skel->links.test4 = bpf_program__attach_uprobe_opts(skel->progs.test4,
+ urand_pid,
+ "./liburandom_read.so",
+ 0 /* offset */,
+ &uprobe_opts);
+ if (!ASSERT_OK_PTR(skel->links.test4, "urandlib_api_attach_ok"))
+ goto cleanup;
+
+ /* Auto attach 3 uprobes to urandlib_api_sameoffset */
+ err = test_uprobe__attach(skel);
+ if (!ASSERT_OK(err, "skel_attach"))
+ goto cleanup;
+
+ /* trigger urandom_read */
+ ASSERT_OK(urand_trigger(&urand_pipe), "urand_exit_code");
+
+ ASSERT_EQ(skel->bss->test1_result, 1, "urandlib_api_sameoffset");
+ ASSERT_EQ(skel->bss->test2_result, 1, "urandlib_api_sameoffset@v1");
+ ASSERT_EQ(skel->bss->test3_result, 1, "urandlib_api_sameoffset@@v2");
+ ASSERT_EQ(skel->bss->test4_result, 1, "urandlib_api");
+
+cleanup:
+ if (urand_pipe)
+ pclose(urand_pipe);
+ test_uprobe__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_uprobe.c b/tools/testing/selftests/bpf/progs/test_uprobe.c
new file mode 100644
index 000000000000..8680280558da
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_uprobe.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Hengqi Chen */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+pid_t my_pid = 0;
+
+int test1_result = 0;
+int test2_result = 0;
+int test3_result = 0;
+int test4_result = 0;
+
+SEC("uprobe/./liburandom_read.so:urandlib_api_sameoffset")
+int BPF_UPROBE(test1)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != my_pid)
+ return 0;
+
+ test1_result = 1;
+ return 0;
+}
+
+SEC("uprobe/./liburandom_read.so:urandlib_api_sameoffset@LIBURANDOM_READ_1.0.0")
+int BPF_UPROBE(test2)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != my_pid)
+ return 0;
+
+ test2_result = 1;
+ return 0;
+}
+
+SEC("uprobe/./liburandom_read.so:urandlib_api_sameoffset@@LIBURANDOM_READ_2.0.0")
+int BPF_UPROBE(test3)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != my_pid)
+ return 0;
+
+ test3_result = 1;
+ return 0;
+}
+
+SEC("uprobe")
+int BPF_UPROBE(test4)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != my_pid)
+ return 0;
+
+ test4_result = 1;
+ return 0;
+}
diff --git a/tools/testing/selftests/bpf/urandom_read.c b/tools/testing/selftests/bpf/urandom_read.c
index e92644d0fa75..b28e910a8fbb 100644
--- a/tools/testing/selftests/bpf/urandom_read.c
+++ b/tools/testing/selftests/bpf/urandom_read.c
@@ -21,6 +21,11 @@ void urand_read_without_sema(int iter_num, int iter_cnt, int read_sz);
void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz);
void urandlib_read_without_sema(int iter_num, int iter_cnt, int read_sz);
+int urandlib_api(void);
+__asm__(".symver urandlib_api_old,urandlib_api@LIBURANDOM_READ_1.0.0");
+int urandlib_api_old(void);
+int urandlib_api_sameoffset(void);
+
unsigned short urand_read_with_sema_semaphore SEC(".probes");
static __attribute__((noinline))
@@ -83,6 +88,10 @@ int main(int argc, char *argv[])
urandom_read(fd, count);
+ urandlib_api();
+ urandlib_api_old();
+ urandlib_api_sameoffset();
+
close(fd);
return 0;
}
diff --git a/tools/testing/selftests/bpf/urandom_read_lib1.c b/tools/testing/selftests/bpf/urandom_read_lib1.c
index 86186e24b740..403b0735e223 100644
--- a/tools/testing/selftests/bpf/urandom_read_lib1.c
+++ b/tools/testing/selftests/bpf/urandom_read_lib1.c
@@ -11,3 +11,44 @@ void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz)
{
STAP_PROBE3(urandlib, read_with_sema, iter_num, iter_cnt, read_sz);
}
+
+/* Symbol versioning is different between static and shared library.
+ * Properly versioned symbols are needed for shared library, but
+ * only the symbol of the new version is needed for static library.
+ * Starting with GNU C 10, use symver attribute instead of .symver assembler
+ * directive, which works better with GCC LTO builds.
+ */
+#if defined(__GNUC__) && __GNUC__ >= 10
+
+#define DEFAULT_VERSION(internal_name, api_name, version) \
+ __attribute__((symver(#api_name "@@" #version)))
+#define COMPAT_VERSION(internal_name, api_name, version) \
+ __attribute__((symver(#api_name "@" #version)))
+
+#else
+
+#define COMPAT_VERSION(internal_name, api_name, version) \
+ asm(".symver " #internal_name "," #api_name "@" #version);
+#define DEFAULT_VERSION(internal_name, api_name, version) \
+ asm(".symver " #internal_name "," #api_name "@@" #version);
+
+#endif
+
+COMPAT_VERSION(urandlib_api_v1, urandlib_api, LIBURANDOM_READ_1.0.0)
+int urandlib_api_v1(void)
+{
+ return 1;
+}
+
+DEFAULT_VERSION(urandlib_api_v2, urandlib_api, LIBURANDOM_READ_2.0.0)
+int urandlib_api_v2(void)
+{
+ return 2;
+}
+
+COMPAT_VERSION(urandlib_api_sameoffset, urandlib_api_sameoffset, LIBURANDOM_READ_1.0.0)
+DEFAULT_VERSION(urandlib_api_sameoffset, urandlib_api_sameoffset, LIBURANDOM_READ_2.0.0)
+int urandlib_api_sameoffset(void)
+{
+ return 3;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset for uprobe
2023-09-05 15:12 ` [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset " Hengqi Chen
@ 2023-09-08 14:30 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
1 sibling, 0 replies; 12+ messages in thread
From: Alan Maguire @ 2023-09-08 14:30 UTC (permalink / raw)
To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, olsajiri
On 05/09/2023 16:12, Hengqi Chen wrote:
> Dynamic symbols in shared library may have the same name, 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
>
> Currently, users can't attach a uprobe to pthread_rwlock_wrlock because
> there are two symbols named pthread_rwlock_wrlock and both are global
> bind. And libbpf considers it as a conflict.
>
> Since both of them are at the same offset we could accept one of them
> harmlessly. Note that we already does this in elf_resolve_syms_offsets.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/lib/bpf/elf.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> index 9d0296c1726a..5c9e588b17da 100644
> --- a/tools/lib/bpf/elf.c
> +++ b/tools/lib/bpf/elf.c
> @@ -214,7 +214,10 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
>
> if (ret > 0) {
> /* handle multiple matches */
> - if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
> + if (elf_sym_offset(sym) == ret) {
> + /* same offset, no problem */
> + continue;
> + } else if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
> /* Only accept one non-weak bind. */
> pr_warn("elf: ambiguous match for '%s', '%s' in '%s'\n",
> sym->name, name, binary_path);
> --
> 2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for symbol versioning for uprobe
2023-09-05 15:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for " Hengqi Chen
@ 2023-09-08 14:40 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
1 sibling, 0 replies; 12+ messages in thread
From: Alan Maguire @ 2023-09-08 14:40 UTC (permalink / raw)
To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, olsajiri
On 05/09/2023 16:12, Hengqi Chen wrote:
> This exercises the newly added dynsym symbol versioning logics.
> Now we accept symbols in form of func, func@LIB_VERSION or
> func@@LIB_VERSION.
>
> The test rely on liburandom_read.so. For liburandom_read.so, we have:
>
> $ nm -D liburandom_read.so
> w __cxa_finalize@GLIBC_2.17
> w __gmon_start__
> w _ITM_deregisterTMCloneTable
> w _ITM_registerTMCloneTable
> 0000000000000000 A LIBURANDOM_READ_1.0.0
> 0000000000000000 A LIBURANDOM_READ_2.0.0
> 000000000000081c T urandlib_api@@LIBURANDOM_READ_2.0.0
> 0000000000000814 T urandlib_api@LIBURANDOM_READ_1.0.0
> 0000000000000824 T urandlib_api_sameoffset@LIBURANDOM_READ_1.0.0
> 0000000000000824 T urandlib_api_sameoffset@@LIBURANDOM_READ_2.0.0
> 000000000000082c T urandlib_read_without_sema@@LIBURANDOM_READ_1.0.0
> 00000000000007c4 T urandlib_read_with_sema@@LIBURANDOM_READ_1.0.0
> 0000000000011018 D urandlib_read_with_sema_semaphore@@LIBURANDOM_READ_1.0.0
>
> For `urandlib_api`, specifying `urandlib_api` will cause a conflict because
> there are two symbols named urandlib_api and both are global bind.
> For `urandlib_api_sameoffset`, there are also two symbols in the .so, but
> both are at the same offset and essentially they refer to the same function
> so no conflict.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Great work putting tests together for this! One small suggestion if
there's a v3; would it be worth having a uretprobe test that uses
library version specification too?
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/testing/selftests/bpf/Makefile | 5 +-
> .../testing/selftests/bpf/liburandom_read.map | 15 +++
> .../testing/selftests/bpf/prog_tests/uprobe.c | 95 +++++++++++++++++++
> .../testing/selftests/bpf/progs/test_uprobe.c | 61 ++++++++++++
> tools/testing/selftests/bpf/urandom_read.c | 9 ++
> .../testing/selftests/bpf/urandom_read_lib1.c | 41 ++++++++
> 6 files changed, 224 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/liburandom_read.map
> create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_uprobe.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index edef49fcd23e..49ff9c716b79 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -193,11 +193,12 @@ endif
>
> # Filter out -static for liburandom_read.so and its dependent targets so that static builds
> # do not fail. Static builds leave urandom_read relying on system-wide shared libraries.
> -$(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
> +$(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c liburandom_read.map
> $(call msg,LIB,,$@)
> $(Q)$(CLANG) $(filter-out -static,$(CFLAGS) $(LDFLAGS)) \
> - $^ $(filter-out -static,$(LDLIBS)) \
> + $(filter %.c,$^) $(filter-out -static,$(LDLIBS)) \
> -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
> + -Wl,--version-script=liburandom_read.map \
> -fPIC -shared -o $@
>
> $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
> diff --git a/tools/testing/selftests/bpf/liburandom_read.map b/tools/testing/selftests/bpf/liburandom_read.map
> new file mode 100644
> index 000000000000..38a97a419a04
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/liburandom_read.map
> @@ -0,0 +1,15 @@
> +LIBURANDOM_READ_1.0.0 {
> + global:
> + urandlib_api;
> + urandlib_api_sameoffset;
> + urandlib_read_without_sema;
> + urandlib_read_with_sema;
> + urandlib_read_with_sema_semaphore;
> + local:
> + *;
> +};
> +
> +LIBURANDOM_READ_2.0.0 {
> + global:
> + urandlib_api;
> +} LIBURANDOM_READ_1.0.0;
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe.c b/tools/testing/selftests/bpf/prog_tests/uprobe.c
> new file mode 100644
> index 000000000000..6c619bcca41c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include <test_progs.h>
> +#include "test_uprobe.skel.h"
> +
> +static FILE *urand_spawn(int *pid)
> +{
> + FILE *f;
> +
> + /* urandom_read's stdout is wired into f */
> + f = popen("./urandom_read 1 report-pid", "r");
> + if (!f)
> + return NULL;
> +
> + if (fscanf(f, "%d", pid) != 1) {
> + pclose(f);
> + errno = EINVAL;
> + return NULL;
> + }
> +
> + return f;
> +}
> +
> +static int urand_trigger(FILE **urand_pipe)
> +{
> + int exit_code;
> +
> + /* pclose() waits for child process to exit and returns their exit code */
> + exit_code = pclose(*urand_pipe);
> + *urand_pipe = NULL;
> +
> + return exit_code;
> +}
> +
> +void test_uprobe(void)
> +{
> + LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
> + struct test_uprobe *skel;
> + FILE *urand_pipe = NULL;
> + int urand_pid = 0, err;
> +
> + skel = test_uprobe__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel_open"))
> + return;
> +
> + urand_pipe = urand_spawn(&urand_pid);
> + if (!ASSERT_OK_PTR(urand_pipe, "urand_spawn"))
> + goto cleanup;
> +
> + skel->bss->my_pid = urand_pid;
> +
> + /* Manual attach uprobe to urandlib_api
> + * There are two `urandlib_api` symbols in .dynsym section:
> + * - urandlib_api@LIBURANDOM_READ_1.0.0
> + * - urandlib_api@LIBURANDOM_READ_1.0.0
> + * Both are global bind and would cause a conflict if user
> + * specify the symbol name without a version suffix
> + */
> + uprobe_opts.func_name = "urandlib_api";
> + skel->links.test4 = bpf_program__attach_uprobe_opts(skel->progs.test4,
> + urand_pid,
> + "./liburandom_read.so",
> + 0 /* offset */,
> + &uprobe_opts);
> + if (!ASSERT_ERR_PTR(skel->links.test4, "urandlib_api_attach_conflict"))
> + goto cleanup;
> +
> + uprobe_opts.func_name = "urandlib_api@LIBURANDOM_READ_1.0.0";
> + skel->links.test4 = bpf_program__attach_uprobe_opts(skel->progs.test4,
> + urand_pid,
> + "./liburandom_read.so",
> + 0 /* offset */,
> + &uprobe_opts);
> + if (!ASSERT_OK_PTR(skel->links.test4, "urandlib_api_attach_ok"))
> + goto cleanup;
> +
> + /* Auto attach 3 uprobes to urandlib_api_sameoffset */
> + err = test_uprobe__attach(skel);
> + if (!ASSERT_OK(err, "skel_attach"))
> + goto cleanup;
> +
> + /* trigger urandom_read */
> + ASSERT_OK(urand_trigger(&urand_pipe), "urand_exit_code");
> +
> + ASSERT_EQ(skel->bss->test1_result, 1, "urandlib_api_sameoffset");
> + ASSERT_EQ(skel->bss->test2_result, 1, "urandlib_api_sameoffset@v1");
> + ASSERT_EQ(skel->bss->test3_result, 1, "urandlib_api_sameoffset@@v2");
> + ASSERT_EQ(skel->bss->test4_result, 1, "urandlib_api");
> +
> +cleanup:
> + if (urand_pipe)
> + pclose(urand_pipe);
> + test_uprobe__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_uprobe.c b/tools/testing/selftests/bpf/progs/test_uprobe.c
> new file mode 100644
> index 000000000000..8680280558da
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_uprobe.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Hengqi Chen */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +pid_t my_pid = 0;
> +
> +int test1_result = 0;
> +int test2_result = 0;
> +int test3_result = 0;
> +int test4_result = 0;
> +
> +SEC("uprobe/./liburandom_read.so:urandlib_api_sameoffset")
> +int BPF_UPROBE(test1)
> +{
> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
> +
> + if (pid != my_pid)
> + return 0;
> +
> + test1_result = 1;
> + return 0;
> +}
> +
> +SEC("uprobe/./liburandom_read.so:urandlib_api_sameoffset@LIBURANDOM_READ_1.0.0")
> +int BPF_UPROBE(test2)
> +{
> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
> +
> + if (pid != my_pid)
> + return 0;
> +
> + test2_result = 1;
> + return 0;
> +}
> +
> +SEC("uprobe/./liburandom_read.so:urandlib_api_sameoffset@@LIBURANDOM_READ_2.0.0")
> +int BPF_UPROBE(test3)
> +{
> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
> +
> + if (pid != my_pid)
> + return 0;
> +
> + test3_result = 1;
> + return 0;
> +}
> +
> +SEC("uprobe")
> +int BPF_UPROBE(test4)
> +{
> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
> +
> + if (pid != my_pid)
> + return 0;
> +
> + test4_result = 1;
> + return 0;
> +}
> diff --git a/tools/testing/selftests/bpf/urandom_read.c b/tools/testing/selftests/bpf/urandom_read.c
> index e92644d0fa75..b28e910a8fbb 100644
> --- a/tools/testing/selftests/bpf/urandom_read.c
> +++ b/tools/testing/selftests/bpf/urandom_read.c
> @@ -21,6 +21,11 @@ void urand_read_without_sema(int iter_num, int iter_cnt, int read_sz);
> void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz);
> void urandlib_read_without_sema(int iter_num, int iter_cnt, int read_sz);
>
> +int urandlib_api(void);
> +__asm__(".symver urandlib_api_old,urandlib_api@LIBURANDOM_READ_1.0.0");
> +int urandlib_api_old(void);
> +int urandlib_api_sameoffset(void);
> +
> unsigned short urand_read_with_sema_semaphore SEC(".probes");
>
> static __attribute__((noinline))
> @@ -83,6 +88,10 @@ int main(int argc, char *argv[])
>
> urandom_read(fd, count);
>
> + urandlib_api();
> + urandlib_api_old();
> + urandlib_api_sameoffset();
> +
> close(fd);
> return 0;
> }
> diff --git a/tools/testing/selftests/bpf/urandom_read_lib1.c b/tools/testing/selftests/bpf/urandom_read_lib1.c
> index 86186e24b740..403b0735e223 100644
> --- a/tools/testing/selftests/bpf/urandom_read_lib1.c
> +++ b/tools/testing/selftests/bpf/urandom_read_lib1.c
> @@ -11,3 +11,44 @@ void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz)
> {
> STAP_PROBE3(urandlib, read_with_sema, iter_num, iter_cnt, read_sz);
> }
> +
> +/* Symbol versioning is different between static and shared library.
> + * Properly versioned symbols are needed for shared library, but
> + * only the symbol of the new version is needed for static library.
> + * Starting with GNU C 10, use symver attribute instead of .symver assembler
> + * directive, which works better with GCC LTO builds.
> + */
> +#if defined(__GNUC__) && __GNUC__ >= 10
> +
> +#define DEFAULT_VERSION(internal_name, api_name, version) \
> + __attribute__((symver(#api_name "@@" #version)))
> +#define COMPAT_VERSION(internal_name, api_name, version) \
> + __attribute__((symver(#api_name "@" #version)))
> +
> +#else
> +
> +#define COMPAT_VERSION(internal_name, api_name, version) \
> + asm(".symver " #internal_name "," #api_name "@" #version);
> +#define DEFAULT_VERSION(internal_name, api_name, version) \
> + asm(".symver " #internal_name "," #api_name "@@" #version);
> +
> +#endif
> +
> +COMPAT_VERSION(urandlib_api_v1, urandlib_api, LIBURANDOM_READ_1.0.0)
> +int urandlib_api_v1(void)
> +{
> + return 1;
> +}
> +
> +DEFAULT_VERSION(urandlib_api_v2, urandlib_api, LIBURANDOM_READ_2.0.0)
> +int urandlib_api_v2(void)
> +{
> + return 2;
> +}
> +
> +COMPAT_VERSION(urandlib_api_sameoffset, urandlib_api_sameoffset, LIBURANDOM_READ_1.0.0)
> +DEFAULT_VERSION(urandlib_api_sameoffset, urandlib_api_sameoffset, LIBURANDOM_READ_2.0.0)
> +int urandlib_api_sameoffset(void)
> +{
> + return 3;
> +}
> --
> 2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
2023-09-05 15:12 ` [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning " Hengqi Chen
@ 2023-09-08 15:07 ` Alan Maguire
2023-09-10 12:39 ` Hengqi Chen
2023-09-10 9:53 ` Jiri Olsa
1 sibling, 1 reply; 12+ messages in thread
From: Alan Maguire @ 2023-09-08 15:07 UTC (permalink / raw)
To: Hengqi Chen, bpf; +Cc: ast, daniel, andrii, olsajiri
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@gmail.com>
One question below, but
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/lib/bpf/elf.c | 145 +++++++++++++++++++++++++++++++++++++----
> tools/lib/bpf/libbpf.c | 2 +-
> 2 files changed, 133 insertions(+), 14 deletions(-)
>
> diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> index 5c9e588b17da..2d2819b7e018 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,112 @@ 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;
> +
> + 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?
> + 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for symbol versioning for uprobe
2023-09-05 15:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for " Hengqi Chen
2023-09-08 14:40 ` Alan Maguire
@ 2023-09-10 9:53 ` Jiri Olsa
1 sibling, 0 replies; 12+ messages in thread
From: Jiri Olsa @ 2023-09-10 9:53 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, ast, daniel, andrii, alan.maguire, olsajiri
On Tue, Sep 05, 2023 at 03:12:57PM +0000, Hengqi Chen wrote:
> This exercises the newly added dynsym symbol versioning logics.
> Now we accept symbols in form of func, func@LIB_VERSION or
> func@@LIB_VERSION.
>
> The test rely on liburandom_read.so. For liburandom_read.so, we have:
>
> $ nm -D liburandom_read.so
> w __cxa_finalize@GLIBC_2.17
> w __gmon_start__
> w _ITM_deregisterTMCloneTable
> w _ITM_registerTMCloneTable
> 0000000000000000 A LIBURANDOM_READ_1.0.0
> 0000000000000000 A LIBURANDOM_READ_2.0.0
> 000000000000081c T urandlib_api@@LIBURANDOM_READ_2.0.0
> 0000000000000814 T urandlib_api@LIBURANDOM_READ_1.0.0
> 0000000000000824 T urandlib_api_sameoffset@LIBURANDOM_READ_1.0.0
> 0000000000000824 T urandlib_api_sameoffset@@LIBURANDOM_READ_2.0.0
> 000000000000082c T urandlib_read_without_sema@@LIBURANDOM_READ_1.0.0
> 00000000000007c4 T urandlib_read_with_sema@@LIBURANDOM_READ_1.0.0
> 0000000000011018 D urandlib_read_with_sema_semaphore@@LIBURANDOM_READ_1.0.0
>
> For `urandlib_api`, specifying `urandlib_api` will cause a conflict because
> there are two symbols named urandlib_api and both are global bind.
> For `urandlib_api_sameoffset`, there are also two symbols in the .so, but
> both are at the same offset and essentially they refer to the same function
> so no conflict.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
one nit below, but looks good
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
> ---
> tools/testing/selftests/bpf/Makefile | 5 +-
> .../testing/selftests/bpf/liburandom_read.map | 15 +++
> .../testing/selftests/bpf/prog_tests/uprobe.c | 95 +++++++++++++++++++
> .../testing/selftests/bpf/progs/test_uprobe.c | 61 ++++++++++++
> tools/testing/selftests/bpf/urandom_read.c | 9 ++
> .../testing/selftests/bpf/urandom_read_lib1.c | 41 ++++++++
> 6 files changed, 224 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/liburandom_read.map
> create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_uprobe.c
>
SNIP
> +void test_uprobe(void)
> +{
> + LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
> + struct test_uprobe *skel;
> + FILE *urand_pipe = NULL;
> + int urand_pid = 0, err;
> +
> + skel = test_uprobe__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel_open"))
> + return;
> +
> + urand_pipe = urand_spawn(&urand_pid);
> + if (!ASSERT_OK_PTR(urand_pipe, "urand_spawn"))
> + goto cleanup;
> +
> + skel->bss->my_pid = urand_pid;
> +
> + /* Manual attach uprobe to urandlib_api
> + * There are two `urandlib_api` symbols in .dynsym section:
> + * - urandlib_api@LIBURANDOM_READ_1.0.0
> + * - urandlib_api@LIBURANDOM_READ_1.0.0
nit, should that be version 2.0.0? ^
> + * Both are global bind and would cause a conflict if user
> + * specify the symbol name without a version suffix
> + */
> + uprobe_opts.func_name = "urandlib_api";
> + skel->links.test4 = bpf_program__attach_uprobe_opts(skel->progs.test4,
> + urand_pid,
> + "./liburandom_read.so",
> + 0 /* offset */,
> + &uprobe_opts);
> + if (!ASSERT_ERR_PTR(skel->links.test4, "urandlib_api_attach_conflict"))
> + goto cleanup;
> +
> + uprobe_opts.func_name = "urandlib_api@LIBURANDOM_READ_1.0.0";
> + skel->links.test4 = bpf_program__attach_uprobe_opts(skel->progs.test4,
> + urand_pid,
> + "./liburandom_read.so",
> + 0 /* offset */,
> + &uprobe_opts);
> + if (!ASSERT_OK_PTR(skel->links.test4, "urandlib_api_attach_ok"))
> + goto cleanup;
> +
> + /* Auto attach 3 uprobes to urandlib_api_sameoffset */
> + err = test_uprobe__attach(skel);
> + if (!ASSERT_OK(err, "skel_attach"))
> + goto cleanup;
> +
> + /* trigger urandom_read */
> + ASSERT_OK(urand_trigger(&urand_pipe), "urand_exit_code");
> +
> + ASSERT_EQ(skel->bss->test1_result, 1, "urandlib_api_sameoffset");
> + ASSERT_EQ(skel->bss->test2_result, 1, "urandlib_api_sameoffset@v1");
> + ASSERT_EQ(skel->bss->test3_result, 1, "urandlib_api_sameoffset@@v2");
> + ASSERT_EQ(skel->bss->test4_result, 1, "urandlib_api");
> +
> +cleanup:
> + if (urand_pipe)
> + pclose(urand_pipe);
> + test_uprobe__destroy(skel);
> +}
SNIP
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset for uprobe
2023-09-05 15:12 ` [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset " Hengqi Chen
2023-09-08 14:30 ` Alan Maguire
@ 2023-09-10 9:53 ` Jiri Olsa
1 sibling, 0 replies; 12+ messages in thread
From: Jiri Olsa @ 2023-09-10 9:53 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, ast, daniel, andrii, alan.maguire, olsajiri
On Tue, Sep 05, 2023 at 03:12:55PM +0000, Hengqi Chen wrote:
> Dynamic symbols in shared library may have the same name, 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
>
> Currently, users can't attach a uprobe to pthread_rwlock_wrlock because
> there are two symbols named pthread_rwlock_wrlock and both are global
> bind. And libbpf considers it as a conflict.
>
> Since both of them are at the same offset we could accept one of them
> harmlessly. Note that we already does this in elf_resolve_syms_offsets.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
> ---
> tools/lib/bpf/elf.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> index 9d0296c1726a..5c9e588b17da 100644
> --- a/tools/lib/bpf/elf.c
> +++ b/tools/lib/bpf/elf.c
> @@ -214,7 +214,10 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
>
> if (ret > 0) {
> /* handle multiple matches */
> - if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
> + if (elf_sym_offset(sym) == ret) {
> + /* same offset, no problem */
> + continue;
> + } else if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
> /* Only accept one non-weak bind. */
> pr_warn("elf: ambiguous match for '%s', '%s' in '%s'\n",
> sym->name, name, binary_path);
> --
> 2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
2023-09-05 15:12 ` [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning " Hengqi Chen
2023-09-08 15:07 ` Alan Maguire
@ 2023-09-10 9:53 ` Jiri Olsa
2023-09-10 12:42 ` Hengqi Chen
1 sibling, 1 reply; 12+ messages in thread
From: Jiri Olsa @ 2023-09-10 9:53 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, ast, daniel, andrii, alan.maguire, olsajiri
On Tue, Sep 05, 2023 at 03:12:56PM +0000, 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@gmail.com>
I have a question below, but other than that
Acked-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
SNIP
> @@ -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,112 @@ 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;
the doc mentions value 1 being special, also I can see readelf
code checking on that.. is that taken into account?
> + }
> return ret;
> }
>
> return NULL;
> }
>
SNIP
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
2023-09-08 15:07 ` Alan Maguire
@ 2023-09-10 12:39 ` Hengqi Chen
0 siblings, 0 replies; 12+ messages in thread
From: Hengqi Chen @ 2023-09-10 12:39 UTC (permalink / raw)
To: Alan Maguire; +Cc: bpf, ast, daniel, andrii, olsajiri
On Fri, Sep 8, 2023 at 11:07 PM Alan Maguire <alan.maguire@oracle.com> 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@gmail.com>
>
> One question below, but
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning for uprobe
2023-09-10 9:53 ` Jiri Olsa
@ 2023-09-10 12:42 ` Hengqi Chen
0 siblings, 0 replies; 12+ messages in thread
From: Hengqi Chen @ 2023-09-10 12:42 UTC (permalink / raw)
To: Jiri Olsa; +Cc: bpf, ast, daniel, andrii, alan.maguire
On Sun, Sep 10, 2023 at 5:53 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Sep 05, 2023 at 03:12:56PM +0000, 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@gmail.com>
>
> I have a question below, but other than that
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
>
> SNIP
>
> > @@ -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,112 @@ 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;
>
> the doc mentions value 1 being special, also I can see readelf
> code checking on that.. is that taken into account?
>
Yes. For value 1, there is a corresponding version def, so no difference.
> > + }
> > return ret;
> > }
> >
> > return NULL;
> > }
> >
>
> SNIP
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-09-10 12:42 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-05 15:12 [PATCH bpf-next v2 0/3] libbpf: Support symbol versioning for uprobe Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 1/3] libbpf: Resolve symbol conflicts at the same offset " Hengqi Chen
2023-09-08 14:30 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
2023-09-05 15:12 ` [PATCH bpf-next v2 2/3] libbpf: Support symbol versioning " Hengqi Chen
2023-09-08 15:07 ` Alan Maguire
2023-09-10 12:39 ` Hengqi Chen
2023-09-10 9:53 ` Jiri Olsa
2023-09-10 12:42 ` Hengqi Chen
2023-09-05 15:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add tests for " Hengqi Chen
2023-09-08 14:40 ` Alan Maguire
2023-09-10 9:53 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox