From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x233.google.com (mail-oi1-x233.google.com [IPv6:2607:f8b0:4864:20::233]) by sourceware.org (Postfix) with ESMTPS id 7B04A3857C49 for ; Tue, 29 Mar 2022 16:34:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7B04A3857C49 Received: by mail-oi1-x233.google.com with SMTP id w127so19560302oig.10 for ; Tue, 29 Mar 2022 09:34:36 -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=yuc73vbjLZcCH8pqtXERK0mOzIaKbuNKblEwVjD2PzE=; b=IdAN5gfUVww/DyxuHGsovmE8nnYmoOVcu3RbHmY1XM7CNdWyL2LWlmPl9tGIgp45Uf UZaWtElKwAtLJ5x07+YL1N66LwVS06tHJvfjF9PzwT1/Hg/hSMnFyTglzYkDciZhU21G OIeJYru+QECvdcCuEWFteqHHhm9p67O/m3CgL/n67n/aKEsmeFLGIwI7yr9sLuFUzkRA fi7dF7pF3ZjAG/WN4qpQREXLXktX255+s4OmJGG3mccg8faSLSf7QUDnz8PJEFvvviCY nAHNIA3qgQv3UdTQewii3CB2SRUCYfcIIyN9qnKRjsrHxeHROwnoRXb0VXPq8JWygj95 ESww== X-Gm-Message-State: AOAM531HQiNtXO0WyN3I6qpCtq9QQV2x6KMGC2qLNzqndlgvO3gJON0i HWS9kgK2s5eHUaoPqGW1R+Y+xw== X-Google-Smtp-Source: ABdhPJy9rzKh9UYcJW/woI2Lieh0vvgOZJL+4i7CpZ6obBog09xQnJ/At/vuSPrh883Oalfyi9jkiA== X-Received: by 2002:aca:2b14:0:b0:2ef:89b7:a885 with SMTP id i20-20020aca2b14000000b002ef89b7a885mr12962oik.16.1648571675472; Tue, 29 Mar 2022 09:34:35 -0700 (PDT) Received: from ?IPV6:2804:431:c7cb:a6c0:f1e1:dcf6:8c18:df3b? ([2804:431:c7cb:a6c0:f1e1:dcf6:8c18:df3b]) by smtp.gmail.com with ESMTPSA id u16-20020a056808151000b002f734da0881sm6968967oiw.57.2022.03.29.09.34.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 29 Mar 2022 09:34:34 -0700 (PDT) Message-ID: <4bd6936c-0c57-34e6-0338-9fc2d87c8637@linaro.org> Date: Tue, 29 Mar 2022 13:34:32 -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 1/5] elf: Support DT_RELR relative relocation format [BZ #27924] Content-Language: en-US To: "H.J. Lu" , libc-alpha@sourceware.org Cc: Joseph Myers References: <20220310200329.1935466-1-hjl.tools@gmail.com> <20220310200329.1935466-2-hjl.tools@gmail.com> From: Adhemerval Zanella In-Reply-To: <20220310200329.1935466-2-hjl.tools@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.2 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: Tue, 29 Mar 2022 16:34:43 -0000 On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote: > From: Fangrui Song > > 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. I really don't think ia64 ELFCLASS32 should be blocker for this change. > > * 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: Agreed. > > * 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 > > Changes from the original patch: > > 1. Check the linker option, -z pack-relative-relocs, which add a > GLIBC_ABI_DT_RELR symbol version dependency on the shared C library if > it provides a GLIBC_2.XX symbol version. > 2. Change make variale to have-dt-relr. > 3. Rename tst-relr-no-pie to tst-relr-pie for --disable-default-pie. > 4. Use TEST_VERIFY in tst-relr.c. > 5. Add the check-tst-relr-pie.out test to check for linker generated > libc.so version dependency on GLIBC_ABI_DT_RELR. > 6. Move ELF_DYNAMIC_DO_RELR before ELF_DYNAMIC_DO_REL. Patch looks ok, some comments below. > --- > configure | 42 +++++++++++++++++++++++++++ > configure.ac | 10 +++++++ > elf/Makefile | 14 +++++++++ > elf/dynamic-link.h | 34 ++++++++++++++++++++++ > elf/elf.h | 13 +++++++-- > elf/get-dynamic-info.h | 3 ++ > elf/tst-relr-pie.c | 1 + > elf/tst-relr.c | 64 ++++++++++++++++++++++++++++++++++++++++++ > 8 files changed, 179 insertions(+), 2 deletions(-) > create mode 100644 elf/tst-relr-pie.c > create mode 100644 elf/tst-relr.c > > diff --git a/configure b/configure > index 8e5bee775a..9156e29fe9 100755 > --- a/configure > +++ b/configure > @@ -6115,6 +6115,48 @@ $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 -z pack-relative-relocs" >&5 > +$as_echo_n "checking for linker that supports -z pack-relative-relocs... " >&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,-z,pack-relative-relocs -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 > + if ${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp -Wl,-z,pack-relative-relocs -nostdlib \ > + -nostartfiles -fPIC -shared -o conftest.so conftest.c 2>&1 \ > + | grep "warning: -z pack-relative-relocs ignored" > /dev/null 2>&1; then > + true > + else > + libc_linker_feature=yes > + fi > + fi > + rm -f conftest* > +fi > +if test $libc_linker_feature = yes; then > + libc_cv_dt_relr=yes > +else > + libc_cv_dt_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-dt-relr = $libc_cv_dt_relr" > +if test "$libc_cv_dt_relr" = yes; then > + have_dt_relr=1 > +else > + have_dt_relr=0 > +fi > + > { $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 87f67d25ec..5c09871fee 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1367,6 +1367,16 @@ 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([-z pack-relative-relocs], > + [-Wl,-z,pack-relative-relocs], > + [libc_cv_dt_relr=yes], [libc_cv_dt_relr=no]) > +LIBC_CONFIG_VAR([have-dt-relr], [$libc_cv_dt_relr]) > +if test "$libc_cv_dt_relr" = yes; then > + have_dt_relr=1 > +else > + have_dt_relr=0 > +fi > + I think you don't need to setup have_dt_relr, it does not accomplish anything here (LIBC_CONFIG_VAR already sets the required have-dt-relr on config.make). > 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 c96924e9c2..fd462ba315 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -541,6 +541,14 @@ tests-special += \ > # tests-special > endif > endif > +ifeq ($(have-dt-relr),yes) > +tests += tst-relr tst-relr-pie > +tests-pie += tst-relr-pie > +tests-special += $(objpfx)check-tst-relr-pie.out Please use one line per test: tests += \ tst-relr \ tst-relr-pie \ # tests tests-pie += tst-relr-pie \ # tests-pie tests-special += \ $(objpfx)check-tst-relr-pie.out \ # tests-special Also, the tst-rel-pie should only be added if $(have-fpie): ifeq ($(have-fpie),yes) tests-pie += \ tst-relr-pie \ #tests-pie endf > +CFLAGS-tst-relr-pie.c += $(pie-ccflag) > +LDFLAGS-tst-relr += -Wl,-z,pack-relative-relocs > +LDFLAGS-tst-relr-pie += -Wl,-z,pack-relative-relocs > +endif > endif > > ifeq ($(run-built-tests),yes) > @@ -2725,3 +2733,9 @@ $(objpfx)tst-p_align3: $(objpfx)tst-p_alignmod3.so > $(objpfx)tst-p_align3.out: tst-p_align3.sh $(objpfx)tst-p_align3 > $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ > $(evaluate-test) > + > +$(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie > + LC_ALL=C $(OBJDUMP) -p $< \ > + | sed -ne '/required from libc.so/,$$ p' \ > + | grep GLIBC_ABI_DT_RELR > $@; \ > + $(evaluate-test) Ok. > diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h > index 25dd7ca4f2..d04c457e55 100644 > --- a/elf/dynamic-link.h > +++ b/elf/dynamic-link.h > @@ -146,12 +146,46 @@ elf_machine_lazy_rel (struct link_map *map, struct r_scope_elem *scope[], > # define ELF_DYNAMIC_DO_RELA(map, scope, lazy, skip_ifunc) /* Nothing to do. */ > # endif > > +# define ELF_DYNAMIC_DO_RELR(map) \ > + do { \ > + ElfW(Addr) l_addr = (map)->l_addr, *where = 0; \ > + const ElfW(Relr) *r, *end; \ > + if (!(map)->l_info[DT_RELR]) \ > + break; \ > + r = (const ElfW(Relr) *)D_PTR((map), l_info[DT_RELR]); \ > + end = (const ElfW(Relr) *)((const char *)r + \ > + (map)->l_info[DT_RELRSZ]->d_un.d_val); \ > + for (; r < end; r++) \ > + { \ > + ElfW(Relr) entry = *r; \ > + if ((entry & 1) == 0) \ > + { \ > + where = (ElfW(Addr) *)(l_addr + entry); \ > + *where++ += l_addr; \ > + } \ > + else \ > + { \ > + for (long i = 0; (entry >>= 1) != 0; i++) \ > + if ((entry & 1) != 0) \ > + where[i] += l_addr; \ > + where += CHAR_BIT * sizeof(ElfW(Relr)) - 1; \ > + } \ > + } \ > + } while (0); > + > /* This can't just be an inline function because GCC is too dumb > to inline functions containing inlines themselves. */ > +# ifdef RTLD_BOOTSTRAP > +# define DO_RTLD_BOOTSTRAP 1 > +# else > +# define DO_RTLD_BOOTSTRAP 0 > +# endif > # define ELF_DYNAMIC_RELOCATE(map, scope, lazy, consider_profile, skip_ifunc) \ > do { \ > int edr_lazy = elf_machine_runtime_setup ((map), (scope), (lazy), \ > (consider_profile)); \ > + if (((map) != &GL(dl_rtld_map) || DO_RTLD_BOOTSTRAP)) \ > + ELF_DYNAMIC_DO_RELR (map); \ > ELF_DYNAMIC_DO_REL ((map), (scope), edr_lazy, skip_ifunc); \ > ELF_DYNAMIC_DO_RELA ((map), (scope), edr_lazy, skip_ifunc); \ > } while (0) Ok. > diff --git a/elf/elf.h b/elf/elf.h > index 0735f6b579..0195029188 100644 > --- a/elf/elf.h > +++ b/elf/elf.h > @@ -443,7 +443,8 @@ typedef struct > #define SHT_PREINIT_ARRAY 16 /* Array of pre-constructors */ > #define SHT_GROUP 17 /* Section group */ > #define SHT_SYMTAB_SHNDX 18 /* Extended section indices */ > -#define SHT_NUM 19 /* Number of defined types. */ > +#define SHT_RELR 19 /* RELR relative relocations */ > +#define SHT_NUM 20 /* Number of defined types. */ > #define SHT_LOOS 0x60000000 /* Start OS-specific. */ > #define SHT_GNU_ATTRIBUTES 0x6ffffff5 /* Object attributes. */ > #define SHT_GNU_HASH 0x6ffffff6 /* GNU-style hash table. */ > @@ -662,6 +663,11 @@ typedef struct > Elf64_Sxword r_addend; /* Addend */ > } Elf64_Rela; > Ok. > +/* RELR relocation table entry */ > + > +typedef Elf32_Word Elf32_Relr; > +typedef Elf64_Xword Elf64_Relr; > + > /* How to extract and insert information held in the r_info field. */ > > #define ELF32_R_SYM(val) ((val) >> 8) > @@ -887,7 +893,10 @@ typedef struct > #define DT_PREINIT_ARRAY 32 /* Array with addresses of preinit fct*/ > #define DT_PREINIT_ARRAYSZ 33 /* size in bytes of DT_PREINIT_ARRAY */ > #define DT_SYMTAB_SHNDX 34 /* Address of SYMTAB_SHNDX section */ > -#define DT_NUM 35 /* Number used */ > +#define DT_RELRSZ 35 /* Total size of RELR relative relocations */ > +#define DT_RELR 36 /* Address of RELR relative relocations */ > +#define DT_RELRENT 37 /* Size of one RELR relative relocaction */ > +#define DT_NUM 38 /* Number used */ > #define DT_LOOS 0x6000000d /* Start of OS-specific */ > #define DT_HIOS 0x6ffff000 /* End of OS-specific */ > #define DT_LOPROC 0x70000000 /* Start of processor-specific */ Ok. > diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h > index 6c2ccd6db4..6c2a3a12b1 100644 > --- a/elf/get-dynamic-info.h > +++ b/elf/get-dynamic-info.h > @@ -89,6 +89,7 @@ elf_get_dynamic_info (struct link_map *l, bool bootstrap, > # if ! ELF_MACHINE_NO_REL > ADJUST_DYN_INFO (DT_REL); > # endif > + ADJUST_DYN_INFO (DT_RELR); > ADJUST_DYN_INFO (DT_JMPREL); > ADJUST_DYN_INFO (VERSYMIDX (DT_VERSYM)); > ADJUST_DYN_INFO (ADDRIDX (DT_GNU_HASH)); > @@ -113,6 +114,8 @@ elf_get_dynamic_info (struct link_map *l, bool bootstrap, > if (info[DT_REL] != NULL) > assert (info[DT_RELENT]->d_un.d_val == sizeof (ElfW(Rel))); > #endif > + if (info[DT_RELR] != NULL) > + assert (info[DT_RELRENT]->d_un.d_val == sizeof (ElfW(Relr))); > if (bootstrap || static_pie_bootstrap) > { > assert (info[DT_RUNPATH] == NULL); Ok. > diff --git a/elf/tst-relr-pie.c b/elf/tst-relr-pie.c > new file mode 100644 > index 0000000000..7df0cdbfd6 > --- /dev/null > +++ b/elf/tst-relr-pie.c > @@ -0,0 +1 @@ > +#include "tst-relr.c" > diff --git a/elf/tst-relr.c b/elf/tst-relr.c Ok. > new file mode 100644 > index 0000000000..56addd2ad4 > --- /dev/null > +++ b/elf/tst-relr.c > @@ -0,0 +1,64 @@ > +/* Basic tests for DT_RELR. > + Copyright (C) 2022 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > +#include > + > +static int o, x; > + > +#define ELEMS O O O O O O O O X X X X X X X O O X O O X X X E X E E O X O E > +#define E 0, > + > +#define O &o, > +#define X &x, > +void *arr[] = { ELEMS }; > +#undef O > +#undef X > + > +#define O 1, > +#define X 2, > +static char val[] = { ELEMS }; > + > +static int > +do_test (void) > +{ > + 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 defined __PIE__ || defined __pie__ || defined PIE || defined pie > + TEST_VERIFY (has_relr); > +#else > + TEST_VERIFY (!has_relr); > +#endif > + } > + > + for (int i = 0; i < sizeof (arr) / sizeof (arr[0]); i++) Use array_length here. > + TEST_VERIFY ((arr[i] == 0 && val[i] == 0) > + || (arr[i] == &o && val[i] == 1) > + || (arr[i] == &x && val[i] == 2)); > + > + return 0; > +} > + > +#include This fails without the rest of the patchset applied: $ ./testrun.sh elf/tst-relr elf/tst-relr: ./libc.so.6: version `GLIBC_ABI_DT_RELR' not found (required by elf/tst-relr) I suggest you move it to third patch when GLIBC_ABI_DT_RELR is added.