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

[Hengqi Chen <hengqi.chen@gmail.com>]

Hi, Jiri:

On 9/4/23 22:10, Jiri Olsa wrote:
> 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 ?

Ahh, right, should be 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?
> 

It looks weird to retrieve version from elf_sym_iter, and we should not
reach here too many times.

>> +
>> +	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
> 

OK, will use asprintf instead.

>> +		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
> 

Let me try.

> 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);

---
Hengqi