From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62f.google.com (mail-pl1-x62f.google.com [IPv6:2607:f8b0:4864:20::62f]) by sourceware.org (Postfix) with ESMTPS id 8FF163858C60 for ; Mon, 18 Oct 2021 19:19:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8FF163858C60 Received: by mail-pl1-x62f.google.com with SMTP id n11so11970850plf.4 for ; Mon, 18 Oct 2021 12:19:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=c0twgcRK5blDfAtspkKEf364MEwbE6DSvTwaRAznPVQ=; b=swqtdBybYcXKZN7CXEJ3Xa4nfYIkP93YDT1T+cuEMXNnD89rgcoAtR6l7QNEUJ3Bo+ 2FnxU/OIYIODuZw8B0SLumQlSgfmY4E4QeBk7Vd8b91LicNB2tGnV6QJtGqY00lHTFiu C4GYmZMWGRW8cjk8SigoLvv05DOwvdoWNKZQm81txSY8k5dswwVUJonvDbV3ATqTysCy sLmMxciH1qPlWXEohAZEiVQegkZGHkAOM3m/a3FUebyIf/nHyTiFpV1vkKaJEVr0l95b /kfHiaWFSQKCyQj7QtsxvS5Zic5cubMWh41VyuWrYZFmrAn2hsKhQj8JNpAVBfE7cuNe LHqg== X-Gm-Message-State: AOAM530QOkBiqAdJdeRv5fypAEXKDylnPF+PqDlu+7aLtX5Q1vT2yfU3 5WqNW8aChCcUvl4wJwxcbhkBlQ== X-Google-Smtp-Source: ABdhPJwwGZeVKIrI7xEHiV0TCwrt/rNqHtR89nDTvy/WjpNHH+vxCdht1PBy0Y8EKXX/XKJjd6zWBA== X-Received: by 2002:a17:90b:1212:: with SMTP id gl18mr906089pjb.166.1634584746310; Mon, 18 Oct 2021 12:19:06 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:8087:bac0:5a04:1c56]) by smtp.gmail.com with ESMTPSA id ls7sm213903pjb.16.2021.10.18.12.19.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Oct 2021 12:19:05 -0700 (PDT) Date: Mon, 18 Oct 2021 12:19:02 -0700 From: =?utf-8?B?RsSBbmctcnXDrCBTw7JuZw==?= To: "H.J. Lu" Cc: GNU C Library , Binutils Subject: Re: [PATCH v2] elf: Support DT_RELR relative relocation format [BZ #27924] Message-ID: <20211018191902.vcf3u5s7h5losxyt@google.com> References: <20211017005020.2645717-1-maskray@google.com> <20211018181530.yz62n64dgn2kd4oe@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-27.4 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL 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: Mon, 18 Oct 2021 19:19:13 -0000 On 2021-10-18, H.J. Lu wrote: >On Mon, Oct 18, 2021 at 11:15 AM Fāng-ruì Sòng wrote: >> >> >> On 2021-10-18, H.J. Lu wrote: >> >On Mon, Oct 18, 2021 at 9:17 AM Fāng-ruì Sòng wrote: >> >> >> >> On Mon, Oct 18, 2021 at 7:42 AM H.J. Lu wrote: >> >> > >> >> > On Sat, Oct 16, 2021 at 5:50 PM Fangrui Song via Binutils >> >> > wrote: >> >> > > >> >> > > PIE and shared objects usually have many relative relocations. In >> >> > > 2017/2018, SHT_RELR/DT_RELR was proposed on >> >> > > https://groups.google.com/g/generic-abi/c/bX460iggiKg/m/GxjM0L-PBAAJ >> >> > > ("Proposal for a new section type SHT_RELR") and is a pre-standard. RELR >> >> > > usually takes 3% or smaller space than R_*_RELATIVE relocations. The >> >> > > virtual memory size of a mostly statically linked PIE is typically 5~10% >> >> > > smaller. >> >> > > >> >> > > --- >> >> > > >> >> > > Notes I will not include in the submitted commit: >> >> > > >> >> > > Available on https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/maskray/relr >> >> > > >> >> > > "pre-standard": even Solaris folks are happy with the refined generic-abi >> >> > > proposal. Cary Coutant will apply the change >> >> > > https://sourceware.org/pipermail/libc-alpha/2021-October/131781.html >> >> > > >> >> > > This patch is simpler than Chrome OS's glibc patch and makes ELF_DYNAMIC_DO_RELR >> >> > > available to all ports. I don't think the current glibc implementation >> >> > > supports ia64 in an ELFCLASS32 container. That said, the style I used is >> >> > > works with an ELFCLASS32 container for 64-bit machine if ElfW(Addr) is >> >> > > 64-bit. >> >> > > >> >> > > * Chrome OS folks have carried a local patch since 2018 (latest version: >> >> > > https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/refs/heads/main/sys-libs/glibc/files/local/glibc-2.32). >> >> > > I.e. this feature has been battle tested. >> >> > > * Android bionic supports 2018 and switched to DT_RELR==36 in 2020. >> >> > > * The Linux kernel has supported CONFIG_RELR since 2019-08 >> >> > > (https://git.kernel.org/linus/5cf896fb6be3effd9aea455b22213e27be8bdb1d). >> >> > > * A musl patch (by me) exists but is not applied: >> >> > > https://www.openwall.com/lists/musl/2019/03/06/3 >> >> > > * rtld-elf from FreeBSD 14 will support DT_RELR. >> >> > > >> >> > > I believe upstream glibc should support DT_RELR to benefit all Linux >> >> > > distributions. I filed some feature requests to get their attention: >> >> > > >> >> > > * Gentoo: https://bugs.gentoo.org/818376 >> >> > > * Arch Linux: https://bugs.archlinux.org/task/72433 >> >> > > * Debian https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=996598 >> >> > > * Fedora https://bugzilla.redhat.com/show_bug.cgi?id=2014699 >> >> > > >> >> > > As of linker support (to the best of my knowledge): >> >> > > >> >> > > * LLD support DT_RELR. >> >> > > * https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/refs/heads/main/sys-devel/binutils/files/ >> >> > > has a gold patch. >> >> > > * GNU ld feature request https://sourceware.org/bugzilla/show_bug.cgi?id=27923 >> >> > > >> >> > > I wish that GNU ld and gold maintainers can implement the feature as well :) >> >> > > >> >> > > Tested on aarch64 and x86_64. >> >> > > >> >> > > Changes from v1 (https://sourceware.org/pipermail/libc-alpha/2021-October/131768.html) >> >> > > * Fix style, simplify code >> >> > > * Improve test >> >> > > --- >> >> > > configure | 31 +++++++++++++++++++++++++++++++ >> >> > > configure.ac | 4 ++++ >> >> > > elf/Makefile | 4 ++++ >> >> > > elf/dynamic-link.h | 28 ++++++++++++++++++++++++++++ >> >> > > elf/elf.h | 13 +++++++++++-- >> >> > > elf/get-dynamic-info.h | 3 +++ >> >> > > elf/tst-relr.c | 27 +++++++++++++++++++++++++++ >> >> > > 7 files changed, 108 insertions(+), 2 deletions(-) >> >> > > create mode 100644 elf/tst-relr.c >> >> > > >> >> > > diff --git a/configure b/configure >> >> > > index 3227e434d3..fdab6a97ef 100755 >> >> > > --- a/configure >> >> > > +++ b/configure >> >> > > @@ -6067,6 +6067,37 @@ $as_echo "$libc_linker_feature" >&6; } >> >> > > config_vars="$config_vars >> >> > > have-depaudit = $libc_cv_depaudit" >> >> > > >> >> > > +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for linker that supports --pack-dyn-relocs=relr" >&5 >> >> > > +$as_echo_n "checking for linker that supports --pack-dyn-relocs=relr... " >&6; } >> >> > > +libc_linker_feature=no >> >> > > +if test x"$gnu_ld" = x"yes"; then >> >> > > + cat > conftest.c <> >> > > +int _start (void) { return 42; } >> >> > > +EOF >> >> > > + if { ac_try='${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp >> >> > > + -Wl,--pack-dyn-relocs=relr -nostdlib -nostartfiles >> >> > > + -fPIC -shared -o conftest.so conftest.c >> >> > > + 1>&5' >> >> > > + { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$ac_try\""; } >&5 >> >> > > + (eval $ac_try) 2>&5 >> >> > > + ac_status=$? >> >> > > + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5 >> >> > > + test $ac_status = 0; }; } >> >> > > + then >> >> > > + libc_linker_feature=yes >> >> > > + fi >> >> > > + rm -f conftest* >> >> > > +fi >> >> > > +if test $libc_linker_feature = yes; then >> >> > > + libc_cv_relr=yes >> >> > > +else >> >> > > + libc_cv_relr=no >> >> > > +fi >> >> > > +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_linker_feature" >&5 >> >> > > +$as_echo "$libc_linker_feature" >&6; } >> >> > > +config_vars="$config_vars >> >> > > +have-relr = $libc_cv_relr" >> >> > > + >> >> > > { $as_echo "$as_me:${as_lineno-$LINENO}: checking for linker that supports --no-dynamic-linker" >&5 >> >> > > $as_echo_n "checking for linker that supports --no-dynamic-linker... " >&6; } >> >> > > libc_linker_feature=no >> >> > > diff --git a/configure.ac b/configure.ac >> >> > > index 00f49f09f7..96110f9d7d 100644 >> >> > > --- a/configure.ac >> >> > > +++ b/configure.ac >> >> > > @@ -1354,6 +1354,10 @@ LIBC_LINKER_FEATURE([--depaudit], [-Wl,--depaudit,x], >> >> > > [libc_cv_depaudit=yes], [libc_cv_depaudit=no]) >> >> > > LIBC_CONFIG_VAR([have-depaudit], [$libc_cv_depaudit]) >> >> > > >> >> > > +LIBC_LINKER_FEATURE([--pack-dyn-relocs=relr], [-Wl,--pack-dyn-relocs=relr], >> >> > > + [libc_cv_relr=yes], [libc_cv_relr=no]) >> >> > > +LIBC_CONFIG_VAR([have-relr], [$libc_cv_relr]) >> >> > > + >> >> > > LIBC_LINKER_FEATURE([--no-dynamic-linker], >> >> > > [-Wl,--no-dynamic-linker], >> >> > > [libc_cv_no_dynamic_linker=yes], >> >> > > diff --git a/elf/Makefile b/elf/Makefile >> >> > > index bf45d8ee24..2c4cdfac68 100644 >> >> > > --- a/elf/Makefile >> >> > > +++ b/elf/Makefile >> >> > > @@ -245,6 +245,10 @@ tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ >> >> > > $(objpfx)tst-audit16-cmp.out >> >> > > endif >> >> > > endif >> >> > > +ifeq ($(have-relr),yes) >> >> > > +tests += tst-relr >> >> > > +LDFLAGS-tst-relr += -Wl,--pack-dyn-relocs=relr >> >> > > +endif >> >> > > endif >> >> > >> >> > Is DT_RELR only generated for PIE? If yes, you need to add it >> >> > to tests-pie and compile it as PIE. >> >> >> >> PIE and shared objects. PDE doesn't need relative relocations. >> >> It is useful to ensure that -Wl,--pack-dyn-relocs=relr doesn't cause >> >> breakage to PDE. >> > >> >Then you need to add a PIE test to verify that >> > >> >1. It has DT_RELR. >> >2. It runs correctly. >> >> --- a/elf/Makefile >> +++ b/elf/Makefile >> @@ -246,4 +246,8 @@ tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ >> endif >> endif >> +ifeq ($(have-relr),yes) >> +tests += tst-relr >> +LDFLAGS-tst-relr += -Wl,--pack-dyn-relocs=relr >> +endif >> endif >> tests += $(tests-execstack-$(have-z-execstack)) >> >> I added this chunk to my test (https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/maskray/relr): >> >> ElfW(Dyn) *d = _DYNAMIC; >> if (d) >> { >> bool has_relr = false; >> for (; d->d_tag != DT_NULL; d++) >> if (d->d_tag == DT_RELR) >> has_relr = true; >> if (!has_relr) >> { >> fprintf (stderr, "no DT_RELR\n"); >> return 1; >> } >> } > >We need 2 tests from the same source: > >1. The DT_RELR linker option doesn't break PDE. >2. The DT_RELR linker option works for PIE. > >You only have one test. Thanks for the feedback. https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/maskray/relr has switched to --- a/elf/Makefile +++ b/elf/Makefile @@ -246,4 +246,10 @@ tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \ endif endif +ifeq ($(have-relr),yes) +tests += tst-relr tst-relr-no-pie +tests-no-pie += tst-relr-no-pie +LDFLAGS-tst-relr += -Wl,--pack-dyn-relocs=relr +LDFLAGS-tst-relr-no-pie += -Wl,--pack-dyn-relocs=relr +endif endif tests += $(tests-execstack-$(have-z-execstack)) >> >> > [hjl@gnu-cfl-2 tmp]$ gcc -pie -fPIE -O2 tst-relr.c >> >> > -Wl,--pack-dyn-relocs=relr -fuse-ld=lld >> >> > [hjl@gnu-cfl-2 tmp]$ ./a.out >> >> > Segmentation fault (core dumped) >> >> > [hjl@gnu-cfl-2 tmp]$ >> >> > >> >> > Given that the current lld implementation generates broken >> >> > binaries for existing glibc without any warning at run-time, >> >> > we should use a different linker command line option to >> >> > implement it properly so that the new binary will fail to >> >> > run on glibc without DT_RELR support at run-time. >> >> >> >> I don't think so. LLD's design is to be machine agnostic and NOT make >> >> decisions which would vary on different machines. >> >> --pack-dyn-relocs=relr is not the default. The user tells LLD to use >> >> DT_RELR and the user is responsible for making sure target ld.so >> >> supports DT_RELR. >> >> LLD is often used as a cross linker. The host ld.so doesn't support >> >> LLD doesn't mean that LLD should disable the format. >> >> For example, it is totally fine to link a FreeBSD executable on a Linux machine. >> > >> >For Linux targets, we need the glibc version dependency for >> >DT_RELR support at run-time. >> >> I know that having an executable crash right away (SIGSEGV, if ld.so doesn't >> resolve relative relocations) without a diagnostic is arguably poor user experience, >> and you might come from this angle and therefore suggest a version check. > >Correct. We need to resolve this before DT_RELR can be accepted. Introducing DT_RELR to provide an alternative way encoding relative relocations is like introducing DT_INIT_ARRAY/DT_PREINIT_ARRAY to replace legacy .ctors/.dtors , but more peaceful: it is a pure linker decision, not mixed compiler codegen and linker decision. In some sense, the problem is not too different from linker creating a WX PT_LOAD segment and the system is configured with rejecting WX. The user provides input and options to make the linker do so. When something is reasonably within the linker's error checking scope, an error checker can be added. But some use cases have valid use of WX PT_LOAD (--omagic), why is the linker in the business of rejecting valid usage? This is not a glibc problem. We could add an error now in the absence of DT_RELR support. But that is moot when the support is coming. Actually, the problem does not need to be solved on linker or glibc' side. The compiler driver can do it. If a Linux distro decides to add -Wl,--pack-dyn-relocs=relr, they do it. The choice is made by them and they are responsible if they want better user experience (usually can be satisfied by upgrating glibc to have a new versioned symbol). When GCC finds such an option universally (or sufficiently) beneficial and decides to add a configure time to make it the default, it can check whether the host supports it. That's the usual way a new thing is added. >> However, this tight libc coupling is not LLD's usual design decision. >> LLD doesn't want to understand what libc it needs to work with. >> >> The user experience problem is related to the ELF spirit that we don't want to >> unnecessarily error for unknown sections/tags/etc. In this case, the evolvement >> of a foundamental feature (relocation) has to be incompatible with older loaders. >> That compatibility check could be implemented in many ways, with their own pros and cons. >> For example, allocating a range of dynamic tags saying that unknown tags in this range >> are hard errors. However, such a foundamental evolution is rare. I tend to think we just >> accept the status quo and do nothing on it. --pack-dyn-relocs=relr users, at least >> from the time being, understand that they need new loaders for the relatively new feature. > >It may be OK for some use cases. I don't think it is acceptable for all use >cases which glibc cares about. See above. >> The problem can be moot when users use --pack-dyn-relocs=relr with new glibc. >> The linked binary will get a high version requirement and running with older glibc >> will be detected right away. I just think it's not linker's responsibility to synthesize >> a symbol targeting a specific glibc version. >> >> -z relr=glibc targeting glibc and -z relr=none targeting non-glibc? That'd just >> be unneeded additional complexity. >> If GNU ld finally decides to proceed with the -z relr=* road, I can consider >> adding a -z relr compatibility option to LLD as well, but I may not synthesize a symbol. >> It's complexity with relatively little benefit IMHO. > > > >-- >H.J.