Re: [PATCH bpf-next 2/2] libbpf: Support symbol versioning for uprobe

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Sep 04, 2023 at 02:24:44AM +0000, Hengqi Chen wrote:
> Currently, we allow users to specify symbol name for uprobe in a qualified
> form, i.e. func@@LIB or func@@LIB_VERSION. For dynamic symbols, their version
> info is actually stored in .gnu.version and .gnu.version_d sections of the ELF
> objects. So dynamic symbols and the qualified name won't match. Add support for
> symbol versioning ([0]) so that we can handle the above case.
> 
>   [0]: https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-PDA/LSB-PDA.junk/symversion.html
> 
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>  tools/lib/bpf/elf.c | 98 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> index 5c9e588b17da..ed3d9880eaa4 100644
> --- a/tools/lib/bpf/elf.c
> +++ b/tools/lib/bpf/elf.c
> @@ -9,6 +9,7 @@
>  #include "str_error.h"
> 
>  #define STRERR_BUFSIZE  128
> +#define HIDDEN_BIT	16

hum, the docs says it's bit 15 ?

SNIP

> @@ -138,12 +155,57 @@ 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 & ~(1 << HIDDEN_BIT);
> +			ret->hidden = versym & (1 << HIDDEN_BIT);
> +		}
>  		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);

should we read verdefs same as you did for versyms in elf_sym_iter_new,
so you don't need to read that every time?

> +
> +	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;
> +}
> 
>  /* Transform symbol's virtual address (absolute for binaries and relative
>   * for shared libs) into file offset, which is what kernel is expecting
> @@ -191,6 +253,9 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
>  	for (i = 0; i < ARRAY_SIZE(sh_types); i++) {
>  		struct elf_sym_iter iter;
>  		struct elf_sym *sym;
> +		size_t sname_len;
> +		char sname[256];

is this enough? not sure if there's symbol max size,
maybe we could also use asprintf below

> +		const char *ver;
>  		int last_bind = -1;
>  		int cur_bind;
> 
> @@ -201,14 +266,31 @@ 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] != '@')
> -				continue;
> +			if (sh_types[i] == SHT_DYNSYM && is_name_qualified) {
> +				if (sym->hidden)
> +					continue;
> +
> +				sname_len = strlen(sym->name);
> +				if (strncmp(sym->name, name, sname_len) != 0)
> +					continue;
> +
> +				ver = elf_get_vername(elf, sym->ver);
> +				if (!ver)
> +					continue;
> +
> +				snprintf(sname, sizeof(sname), "%s@@%s", sym->name, ver);
> +				if (strncmp(sname, name, name_len) != 0)
> +					continue;
> +			} else {
> +				/* 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] != '@')
> +					continue;

hum, I never checked the versioned symbols, but it looks like we
don't get symbols in 'symbol@version' form, so I wonder how that
worked before

would be great to have a selftest for that

also I had to add change below to test that through prog's section,
I think we need allow '@' in there

thanks,
jirka


---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 96ff1aa4bf6a..a30f3c48f891 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11512,8 +11512,11 @@ 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);