From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42a.google.com (mail-pf1-x42a.google.com [IPv6:2607:f8b0:4864:20::42a]) by sourceware.org (Postfix) with ESMTPS id 7D8A83858C2D for ; Wed, 30 Mar 2022 14:42:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7D8A83858C2D Received: by mail-pf1-x42a.google.com with SMTP id b15so18906216pfm.5 for ; Wed, 30 Mar 2022 07:42:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=MrB3Hw8p8sQOokJCVLwKkFhHIyBxSqJ1GFB6kzWN/kk=; b=6FChvivvSUFVMny+VD3qYwHx4+4g4zhEVNGS+wi2+6KmXEnAiDi9FuEDqmkWdeyIL9 uEVmY2Kn70sXB23JKt7Vb7G38sgRxAAOi9IQTOlDVHWphMIs0Yfe5G+C1ym+SkiW3QAd TB42gvROSXCd8p1+JIeszYOAkctqhR6kDOEEypjkAh9FyCdrFrU5HdqKc3kCzDOHwdqR dNHWDjIE+k2qzVsnrslGtJpqh1NLQG9bT5SH483PAZ+14mMcNWb1nI6kqFWzQ+2yccP8 dSuV+H/XcBHfEEQD9yjbBUc+aFNSWgt4Z2pobhSxzNtjKlNWD73NLbT+KiYzy7pTw2aP ARsA== X-Gm-Message-State: AOAM532W3N/zkGjwwP6Nox1A+8B+MDYIFBqzVGZpJTXdOHE/NNCE91tA UE6rTkIr8k3AlD1E6Is3dSH7AOYTP5MPoMUPFbg= X-Google-Smtp-Source: ABdhPJyC+fkcG6YiHYwO0pGYwPeLqWNhNiSgBwZjz1iwhicq63tupNFMHTBmhid4VCmMgsc1CJG2TisBDhpkI7ID0sw= X-Received: by 2002:a63:dd47:0:b0:381:2bb3:86ba with SMTP id g7-20020a63dd47000000b003812bb386bamr6577486pgj.381.1648651330331; Wed, 30 Mar 2022 07:42:10 -0700 (PDT) MIME-Version: 1.0 References: <20220310200329.1935466-1-hjl.tools@gmail.com> <20220310200329.1935466-4-hjl.tools@gmail.com> <066eedff-b1d4-ec3c-3716-5846e2646f9a@linaro.org> In-Reply-To: <066eedff-b1d4-ec3c-3716-5846e2646f9a@linaro.org> From: "H.J. Lu" Date: Wed, 30 Mar 2022 07:41:35 -0700 Message-ID: Subject: Re: [PATCH v6 3/5] Add GLIBC_ABI_DT_RELR for DT_RELR support To: Adhemerval Zanella Cc: GNU C Library , Joseph Myers Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3025.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Mar 2022 14:42:15 -0000 On Wed, Mar 30, 2022 at 7:18 AM Adhemerval Zanella wrote: > > > > On 29/03/2022 19:29, H.J. Lu wrote: > > On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella > > wrote: > >> > >> > >> > >> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote: > >>> The EI_ABIVERSION field of the ELF header in executables and shared > >>> libraries can be bumped to indicate the minimum ABI requirement on the > >>> dynamic linker. However, EI_ABIVERSION in executables isn't checked by > >>> the Linux kernel ELF loader nor the existing dynamic linker. Executables > >>> will crash mysteriously if the dynamic linker doesn't support the ABI > >>> features required by the EI_ABIVERSION field. The dynamic linker should > >>> be changed to check EI_ABIVERSION in executables. > >>> > >>> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so > >>> that the existing dynamic linkers will issue an error on executables with > >>> GLIBC_ABI_DT_RELR dependency. Issue an error if there is a DT_RELR entry > >>> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition. > >>> > >>> Support __placeholder_only_for_empty_version_map as the placeholder symbol > >>> used only for empty version map to generate GLIBC_ABI_DT_RELR without any > >>> symbols. > >> > >> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually > >> supports DT_RELR, otherwise if the ABI start to support old glibcs might > >> wrongly assumes DT_RELR and fails with undefined behavior at runtime > >> (which this patch essentially tries to avoid). > > > > The DT_RELR set enables DT_RELR in glibc for all targets. > If I understood correctly, if we add GLIBC_ABI_DT_RELR on 2.36 but only enable > it on some architecture on 2.37 and try to run a binary with DT_RELR enabled > on glibc 2.36, _dl_check_map_versions won't accuse a failure and it will > eventually only fail at runtime (since relocation won't be applied). My > understanding we are trying to avoid such failures. My patch set enables DT_RELR and GLIBC_ABI_DT_RELR on glibc 2.36 for all, regardless of binutils version. There is no per-arch behavior. Only DT_RELR run-time tests are enabled for linkers with -z pack-relative-relocs. > > > >>> --- > >>> elf/Makefile | 12 ++++++++++-- > >>> elf/Versions | 5 +++++ > >>> elf/dl-version.c | 33 +++++++++++++++++++++++++++++++-- > >>> include/link.h | 6 ++++++ > >>> scripts/abilist.awk | 2 ++ > >>> scripts/versions.awk | 7 ++++++- > >>> 6 files changed, 60 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/elf/Makefile b/elf/Makefile > >>> index fd462ba315..d3118b9777 100644 > >>> --- a/elf/Makefile > >>> +++ b/elf/Makefile > >>> @@ -1113,8 +1113,10 @@ $(eval $(call include_dsosort_tests,dso-sort-tests-1.def)) > >>> $(eval $(call include_dsosort_tests,dso-sort-tests-2.def)) > >>> endif > >>> > >>> -check-abi: $(objpfx)check-abi-ld.out > >>> -tests-special += $(objpfx)check-abi-ld.out > >>> +check-abi: $(objpfx)check-abi-ld.out \ > >>> + $(objpfx)check-abi-version-libc.out > >>> +tests-special += $(objpfx)check-abi-ld.out \ > >>> + $(objpfx)check-abi-version-libc.out > >> > >> One line per definition. > > > > Fixed in v7. > > > >>> update-abi: update-abi-ld > >>> update-all-abi: update-all-abi-ld > >>> > >>> @@ -2739,3 +2741,9 @@ $(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie > >>> | sed -ne '/required from libc.so/,$$ p' \ > >>> | grep GLIBC_ABI_DT_RELR > $@; \ > >>> $(evaluate-test) > >>> + > >>> +$(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so > >>> + LC_ALL=C $(READELF) -V -W $< \ > >>> + | sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \ > >>> + | grep GLIBC_ABI_DT_RELR > $@; \ > >>> + $(evaluate-test) > >> > >> This test should be enable iff for $(have-dt-relr) == yes. > > > > This test checks if libc.so has the GLIBC_ABI_DT_RELR > > version definition. Since it doesn't require a new linker, > > there is no need to check $(have-dt-relr). This test > > passes with an old linker. > > > >>> diff --git a/elf/Versions b/elf/Versions > >>> index 8bed855d8c..a9ff278de7 100644 > >>> --- a/elf/Versions > >>> +++ b/elf/Versions > >>> @@ -23,6 +23,11 @@ libc { > >>> GLIBC_2.35 { > >>> _dl_find_object; > >>> } > >>> + GLIBC_ABI_DT_RELR { > >>> + # This symbol is used only for empty version map and will be removed > >>> + # by scripts/versions.awk. > >>> + __placeholder_only_for_empty_version_map; > >>> + } > >> > >> Same as before. Maybe use the same strategy we do for time64: add a config.h > >> define to HAVE_DTRELR (set by configure) and use it to set the symbol if > >> it is defined. > >> > >> This define would be used on dl-version.c to also enable the l_abi_version > >> check. > >> > >>> GLIBC_PRIVATE { > >>> # functions used in other libraries > >>> __libc_early_init; > >>> diff --git a/elf/dl-version.c b/elf/dl-version.c > >>> index b47bd91727..720ec596a5 100644 > >>> --- a/elf/dl-version.c > >>> +++ b/elf/dl-version.c > >>> @@ -214,12 +214,20 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode) > >>> while (1) > >>> { > >>> /* Match the symbol. */ > >>> + const char *string = strtab + aux->vna_name; > >>> result |= match_symbol (DSO_FILENAME (map->l_name), > >>> map->l_ns, aux->vna_hash, > >>> - strtab + aux->vna_name, > >>> - needed->l_real, verbose, > >>> + string, needed->l_real, verbose, > >>> aux->vna_flags & VER_FLG_WEAK); > >>> > >>> + if (map->l_abi_version == lav_none > >>> + /* 0xfd0e42: _dl_elf_hash ("GLIBC_ABI_DT_RELR"). */ > >>> + && aux->vna_hash == 0xfd0e42 > >>> + && __glibc_likely (strcmp (string, > >>> + "GLIBC_ABI_DT_RELR") > >>> + == 0)) > >>> + map->l_abi_version = lav_dt_relr_ref; > >>> + > >>> /* Compare the version index. */ > >>> if ((unsigned int) (aux->vna_other & 0x7fff) > ndx_high) > >>> ndx_high = aux->vna_other & 0x7fff; > >>> @@ -253,6 +261,16 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode) > >>> ent = (ElfW(Verdef) *) (map->l_addr + def->d_un.d_ptr); > >>> while (1) > >>> { > >>> + /* 0x0963cf85: _dl_elf_hash ("GLIBC_PRIVATE"). */ > >>> + if (ent->vd_hash == 0x0963cf85) > >>> + { > >>> + ElfW(Verdaux) *aux = (ElfW(Verdaux) *) ((char *) ent > >>> + + ent->vd_aux); > >>> + if (__glibc_likely (strcmp ("GLIBC_PRIVATE", > >>> + strtab + aux->vda_name) == 0)) > >>> + map->l_abi_version = lav_private_def; > >>> + } > >>> + > >>> if ((unsigned int) (ent->vd_ndx & 0x7fff) > ndx_high) > >>> ndx_high = ent->vd_ndx & 0x7fff; > >>> > >>> @@ -352,6 +370,17 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode) > >>> } > >>> } > >>> > >>> + /* Issue an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR > >>> + dependency nor GLIBC_PRIVATE definition. */ > >>> + if (map->l_info[DT_RELR] != NULL > >>> + && __glibc_unlikely (map->l_abi_version == lav_none)) > >>> + { > >>> + _dl_exception_create > >>> + (&exception, DSO_FILENAME (map->l_name), > >>> + N_("DT_RELR without GLIBC_ABI_DT_RELR dependency")); > >>> + goto call_error; > >>> + } > >>> + > >>> return result; > >>> } > >>> > >>> diff --git a/include/link.h b/include/link.h > >>> index 03db14c7b0..8ec5e35cf2 100644 > >>> --- a/include/link.h > >>> +++ b/include/link.h > >>> @@ -177,6 +177,12 @@ struct link_map > >>> lt_library, /* Library needed by main executable. */ > >>> lt_loaded /* Extra run-time loaded shared object. */ > >>> } l_type:2; > >>> + enum /* ABI dependency of this object. */ > >>> + { > >>> + lav_none, /* No ABI dependency. */ > >>> + lav_dt_relr_ref, /* Need GLIBC_ABI_DT_RELR. */ > >>> + lav_private_def /* Define GLIBC_PRIVATE. */ > >>> + } l_abi_version:2; > >>> unsigned int l_relocated:1; /* Nonzero if object's relocations done. */ > >>> unsigned int l_init_called:1; /* Nonzero if DT_INIT function called. */ > >>> unsigned int l_global:1; /* Nonzero if object in _dl_global_scope. */ > >>> diff --git a/scripts/abilist.awk b/scripts/abilist.awk > >>> index 24a34ccbed..6cc7af6ac8 100644 > >>> --- a/scripts/abilist.awk > >>> +++ b/scripts/abilist.awk > >>> @@ -55,6 +55,8 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) { > >>> # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless. > >>> if (NF > 7 && $7 == ".hidden") next; > >>> > >>> + if (version ~ /^GLIBC_ABI_/ && !include_abi_version) next; > >>> + > >>> if (version == "GLIBC_PRIVATE" && !include_private) next; > >>> > >>> desc = ""; > >>> diff --git a/scripts/versions.awk b/scripts/versions.awk > >>> index 357ad1355e..d70b07bd1a 100644 > >>> --- a/scripts/versions.awk > >>> +++ b/scripts/versions.awk > >>> @@ -185,8 +185,13 @@ END { > >>> closeversion(oldver, veryoldver); > >>> veryoldver = oldver; > >>> } > >>> - printf("%s {\n global:\n", $2) > outfile; > >>> oldver = $2; > >>> + # Skip the placeholder symbol used only for empty version map. > >>> + if ($3 == "__placeholder_only_for_empty_version_map;") { > >>> + printf("%s {\n", $2) > outfile; > >>> + continue; > >>> + } > >>> + printf("%s {\n global:\n", $2) > outfile; > >>> } > >>> printf(" ") > outfile; > >>> for (n = 3; n <= NF; ++n) { > > > > > > -- H.J.