From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x535.google.com (mail-pg1-x535.google.com [IPv6:2607:f8b0:4864:20::535]) by sourceware.org (Postfix) with ESMTPS id A5A4C3858400 for ; Mon, 18 Oct 2021 18:15:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A5A4C3858400 Received: by mail-pg1-x535.google.com with SMTP id f5so16997141pgc.12 for ; Mon, 18 Oct 2021 11:15: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:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=9wF3OtBi5RVutRJ+AmCD5tg9nvOF6zPmyWh7sQFlauk=; b=mc3f4wY01WL95GP/tanv+vAjoR58UPLtbuKBjHxhEb0UxoDaxMWpnCwtgHB7cIsgeM EsYLm4zxoCxQ8DROSR7RW0WJY3ch4vIL+jc97lPTuz6xb+p9GCXOFKCtQq7NzXNH3Nr5 uYAPxZoMGejpFdGdrL39U41k/Gm+FaEKCqh4lrDsdKelH+lLKuJJaMcVHJ2ZIWIl7SEK poWfoSDACGyR7mObIEvJtldwtSircesdafrBX3BFP+bXGx3lGgBbbOTMGeCah9sAd1mw 03h9nICVmHYZwl9mMKLSkqoe5WqXurr266881PinXrOTV7V00LrXb9ENxQvf8zOmnFwa nyfQ== X-Gm-Message-State: AOAM533vPvpsV6pU+OEF7vd5Ln1yhzK9hxSaSEDgQw7nEvhCZyJTX4TK eq4TLdYxy50BFjf+v0JdJOEeBQ== X-Google-Smtp-Source: ABdhPJwtXfzxmF4UYoQtdBw7yroAY+LcIg1ySlFnFJROCxl3DmxDAWfJ90CknOvRAT4osildr3jWoQ== X-Received: by 2002:a05:6a00:140e:b0:444:b077:51ef with SMTP id l14-20020a056a00140e00b00444b07751efmr29435178pfu.61.1634580934364; Mon, 18 Oct 2021 11:15:34 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:8087:bac0:5a04:1c56]) by smtp.gmail.com with ESMTPSA id a21sm103324pju.57.2021.10.18.11.15.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Oct 2021 11:15:33 -0700 (PDT) Date: Mon, 18 Oct 2021 11:15:30 -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: <20211018181530.yz62n64dgn2kd4oe@google.com> References: <20211017005020.2645717-1-maskray@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.2 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 18:15:38 -0000 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; } } >> > [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. 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. 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.