On Tue, Oct 17, 2023 at 12:06 PM Fangrui Song <maskray@xxxxxxxxxx> wrote: > > 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. > > We are dealing with symbols defined in an ELF object, not the shared libraries it refers to. So I guess we don't need to handle verneed. > 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
