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

["Fāng-ruì Sòng" <maskray@google.com>]

On Mon, Oct 18, 2021 at 12:45 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> 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.

OK. I added:

tests-pie += tst-relr

> > +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.

I know. I have mentioned that the situation is not different from many
previous situations where
a compatibility check may not catch everything.
(When ET_DYN executable was first introduced, did it need to bump
e_version or add something to e_flags?
When DT_INIT_ARRAY was added?
...
)

I did highlight that even in the absence of the check, the harm is minimal:
--pack-dyn-relocs=relr with new glibc will introduce new version
requirement, which will be naturally caught by older ld.so.
There are a ton non-default linker options which can create
non-runnable binaries.

In the end, I consider that the discussion is useful but has derailed
from glibc getting the feature.
Changes are made to new glibc, not old glibc. We cannot time travel to
the past and change glibc to reject new features today.
It is still valuable and we may need it again when GCC/distributions
want to make relr the default for some targets.