From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id B8E93385800E for ; Wed, 30 Mar 2022 14:18:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B8E93385800E Received: by mail-oi1-x236.google.com with SMTP id 12so22174012oix.12 for ; Wed, 30 Mar 2022 07:18:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=HsozuRTNbGM5j0m3U5K9w501IoYYGmWeJP0ReZLxDaQ=; b=bo6D2dZtR9fRolZrJmwQXi1rtDSQ6bRDgY3kx/WmkQREEftnoUVA9vO3RNCuc8tIvN wyHl+aizLBbbgJbOPvV1FZF8CGywB4M/HByatsF6/9ePWY/IK17ieC5AXeVrr/h05ezM Yj6JGowsliM2OH6Qs/kaYCRxFmZemO+q99Evlm5VsZcvGyqBePS+zq62CYthu00dQ/WO Bc1u2kODzehPb2Ic5iKWQH/JRlcM9cSgvDBV74DhB1A67pt7Zbt4vfcqJcw82uamzTG+ K5vRBoTBRNhXs+qsVXMLS5zu7e7WUWADMW2ZWC0GmFzK4rUGzKOdX1qONfK8o6st7hTT 7RDQ== X-Gm-Message-State: AOAM530fBzD65+KrBmdtpjztPeXr6iDJOzIhUfCsAUKK9HTVA70epJ1C xm4NkXzMCB9YXIqLKAOxyCmENIBPU8mAsw== X-Google-Smtp-Source: ABdhPJyKdLNtZnWN/R+pjm8qo3gePk/4aBZLnIYwB4zpYq/feQLcSY5L3vJkJC+nV9g2Lcowdj7EtQ== X-Received: by 2002:a05:6808:13ca:b0:2d0:6e82:6983 with SMTP id d10-20020a05680813ca00b002d06e826983mr1945763oiw.5.1648649914052; Wed, 30 Mar 2022 07:18:34 -0700 (PDT) Received: from ?IPV6:2804:431:c7cb:a6c0:ca7b:5b69:d952:46d0? ([2804:431:c7cb:a6c0:ca7b:5b69:d952:46d0]) by smtp.gmail.com with ESMTPSA id 24-20020a056870109800b000dded4f78f1sm9798781oaq.51.2022.03.30.07.18.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 30 Mar 2022 07:18:33 -0700 (PDT) Message-ID: <066eedff-b1d4-ec3c-3716-5846e2646f9a@linaro.org> Date: Wed, 30 Mar 2022 11:18:31 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v6 3/5] Add GLIBC_ABI_DT_RELR for DT_RELR support Content-Language: en-US To: "H.J. Lu" Cc: GNU C Library , Joseph Myers References: <20220310200329.1935466-1-hjl.tools@gmail.com> <20220310200329.1935466-4-hjl.tools@gmail.com> From: Adhemerval Zanella In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, 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:18:38 -0000 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. > >>> --- >>> 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) { > > >