On 2023-10-17, Hengqi Chen wrote:
+ Fangrui
Thanks for CCing me. I have spent countless hours studying symbol
versioning...
https://maskray.me/blog/2020-11-26-all-about-symbol-versioning
On Tue, Oct 17, 2023 at 4:10 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
On Mon, Oct 16, 2023 at 11:28 AM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
>
> Fix too eager assumption that SHT_GNU_verdef ELF section is going to be
> present whenever binary has SHT_GNU_versym section. It seems like either
> SHT_GNU_verdef or SHT_GNU_verneed can be used, so failing on missing
> SHT_GNU_verdef actually breaks use cases in production.
>
> One specific reported issue, which was used to manually test this fix,
> was trying to attach to `readline` function in BASH binary.
>
> Cc: Hengqi Chen <hengqi.chen@xxxxxxxxx>
> Reported-by: Liam Wisehart <liamwisehart@xxxxxxxx>
> Fixes: bb7fa09399b9 ("libbpf: Support symbol versioning for uprobe")
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> tools/lib/bpf/elf.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
Hengqi,
Please take a look when you get a chance. I'm not very familiar with
symbol versioning details, but it seems like we made a too strong
assumption about verdef always being present. In bash's case we have
VERNEED, but not VERDEF, and that seems to be ok:
Yes, both VERNEED and VERDEF are optional.
Yes.
The .gnu.version table assigns a version index to each .dynsym entry. An
entry (version ID) corresponds to a Index: entry in .gnu.version_d or a
Version: entry in .gnu.version_r.
[ 8] .gnu.version VERSYM 000000000001c9ca 01c9ca
00130c 02 A 6 0 2
[ 9] .gnu.version_r VERNEED 000000000001dcd8 01dcd8
0000b0 00 A 7 2 8
So perhaps we need to complete the implementation to take VERNEED into
account. And also let's add a test that can catch an issue like this
going forward. Thanks!
AFAIK, VERNEED contains version requirements for shared libraries.
Yes.
> diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> index 2a158e8a8b7c..2a62bf411bb3 100644
> --- a/tools/lib/bpf/elf.c
> +++ b/tools/lib/bpf/elf.c
> @@ -141,14 +141,15 @@ static int elf_sym_iter_new(struct elf_sym_iter *iter,
> iter->versyms = elf_getdata(scn, 0);
>
> scn = elf_find_next_scn_by_type(elf, SHT_GNU_verdef, NULL);
> - if (!scn) {
> - pr_debug("elf: failed to find verdef ELF sections in '%s'\n", binary_path);
> - return -ENOENT;
> - }
> - if (!gelf_getshdr(scn, &sh))
> + if (!scn)
> + return 0;
> +
> + iter->verdefs = elf_getdata(scn, 0);
> + if (!iter->verdefs || !gelf_getshdr(scn, &sh)) {
> + pr_warn("elf: failed to get verdef ELF section in '%s'\n", binary_path);
> return -EINVAL;
> + }
> iter->verdef_strtabidx = sh.sh_link;
> - iter->verdefs = elf_getdata(scn, 0);
>
> return 0;
> }
> @@ -199,6 +200,9 @@ static const char *elf_get_vername(struct elf_sym_iter *iter, int ver)
> GElf_Verdef verdef;
> int offset;
>
> + if (!iter->verdefs)
> + return NULL;
> +
> offset = 0;
> while (gelf_getverdef(iter->verdefs, offset, &verdef)) {
> if (verdef.vd_ndx != ver) {
> --
> 2.34.1
>
Anyway, this change look good to me, so
Acked-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>
Looks good to me, too.
Review Reviewed-by: Fangrui Song <maskray@xxxxxxxxxx>
---
I have a question about a previous patch
"libbpf: Support symbol versioning for uprobe"
(commit bb7fa09399b937cdc4432ac99f9748f5a7f69389 in next/master).
In the function 'symbol_match',
/* If user specifies symbol version, for dynamic symbols,
* get version name from ELF verdef section for comparison.
*/
if (sh_type == SHT_DYNSYM) {
ver_name = elf_get_vername(iter, sym->ver);
if (!ver_name)
return false;
return strcmp(ver_name, lib_ver) == 0;
}
elf_get_vername only checks verdef, not verneed. Is this an issue?
I am not familiar with tools/lib/bpf or how it is used for uprobe.
Is the function intended to match linker behavior?
Then the rules described at https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#linker-behavior
apply.
I think the current rules are quite good.
--
Hengqi
