Re: [PATCH v2] elf: Support DT_RELR relative relocation format [BZ #27924]

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Oct 18, 2021 at 12:19 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> On 2021-10-18, H.J. Lu wrote:
> >On Mon, Oct 18, 2021 at 11:15 AM Fāng-ruì Sòng <maskray@google.com> wrote:
> >>
> >>
> >> On 2021-10-18, H.J. Lu wrote:
> >> >On Mon, Oct 18, 2021 at 9:17 AM Fāng-ruì Sòng <maskray@google.com> wrote:
> >> >>
> >> >> On Mon, Oct 18, 2021 at 7:42 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >> >> >
> >> >> > On Sat, Oct 16, 2021 at 5:50 PM Fangrui Song via Binutils
> >> >> > <binutils@sourceware.org> 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 <<EOF
> >> >> > > +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

You need one test with explicit PIE and one with explicit non-PIE
since not all compilers default to 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.
>

The problem is running the DT_RELR binary on the old ld.so, which
can't be changed.

H.J.