+ Fangrui 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. > [ 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. > > 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> -- Hengqi