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

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

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;
        }
     }

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