Re: [PATCH v7 3/6] elf: Support DT_RELR relative relocation format [BZ #27924]

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

On Mon, Apr 4, 2022 at 10:12 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 31/03/2022 13:38, H.J. Lu wrote:
> > From: Fangrui Song <maskray@google.com>
> >
> > 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
> >
> > 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.
>
> I think it is worth to add a note on commit that DT_RELR relocation are
> always applies a bind-now (do -Wl,-z,lazy or LD_BIND_NOW has not effect).

DT_RELR compresses relative relocation which has no impact on lazy binding.
I will update the NEWS entry with:

* Support for DT_RELR relative relocation format has been added to
  glibc.  This is a new ELF dynamic tag that improves the size of
  relative relocations in shared object files and position independent
  executables (PIE).  DT_RELR generation requires linker support for
  -z pack-relative-relocs option, which is supported for some targets
  in recent binutils versions.  Lazy binding doesn't apply to DT_RELR.

> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>
> > ---
> >  configure              | 37 ++++++++++++++++++++++++
> >  configure.ac           |  5 ++++
> >  elf/Makefile           | 25 ++++++++++++++++
> >  elf/dynamic-link.h     | 34 ++++++++++++++++++++++
> >  elf/get-dynamic-info.h |  3 ++
> >  elf/tst-relr-pie.c     |  1 +
> >  elf/tst-relr.c         | 65 ++++++++++++++++++++++++++++++++++++++++++
> >  7 files changed, 170 insertions(+)
> >  create mode 100644 elf/tst-relr-pie.c
> >  create mode 100644 elf/tst-relr.c
> >
> > diff --git a/configure b/configure
> > index 2a3cb49b0b..05b72ceebf 100755
> > --- a/configure
> > +++ b/configure
> > @@ -6115,6 +6115,43 @@ $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 <<EOF
> > +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"
> > +
> >  { $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 fa7d3c025b..f4ac25aa90 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -1367,6 +1367,11 @@ 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])
> > +
> >  LIBC_LINKER_FEATURE([--no-dynamic-linker],
> >                   [-Wl,--no-dynamic-linker],
> >                   [libc_cv_no_dynamic_linker=yes],
>
> Ok.
>
> > diff --git a/elf/Makefile b/elf/Makefile
> > index 09d3d88336..979dde046d 100644
> > --- a/elf/Makefile
> > +++ b/elf/Makefile
> > @@ -541,6 +541,25 @@ tests-special += \
> >    # tests-special
> >  endif
> >  endif
> > +ifeq ($(have-dt-relr),yes)
> > +tests += \
> > +  tst-relr \
> > +# tests
> > +ifeq ($(have-fpie),yes)
> > +tests += \
> > +  tst-relr-pie \
> > +# tests
> > +tests-pie += \
> > +  tst-relr-pie \
> > +# tests-pie
> > +tests-special += \
> > +  $(objpfx)check-tst-relr-pie.out \
> > +# tests-special
> > +endif
> > +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)
>
> Ok.
>
> > @@ -2735,3 +2754,9 @@ $(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so
> >               | sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \
> >               | grep GLIBC_ABI_DT_RELR > $@; \
> >       $(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)
> > 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++)                         \
>
> s/long/long int/

Fixed in v8.

> > +           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/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"
>
> Ok.
>
> > diff --git a/elf/tst-relr.c b/elf/tst-relr.c
> > new file mode 100644
> > index 0000000000..c634ce0d21
> > --- /dev/null
> > +++ b/elf/tst-relr.c
> > @@ -0,0 +1,65 @@
> > +/* 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
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <link.h>
> > +#include <stdbool.h>
> > +#include <array_length.h>
> > +#include <support/check.h>
> > +
> > +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 < array_length (arr); i++)
> > +    TEST_VERIFY ((arr[i] == 0 && val[i] == 0)
> > +              || (arr[i] == &o && val[i] == 1)
> > +              || (arr[i] == &x && val[i] == 2));
> > +
> > +  return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
>
> Ok.



-- 
H.J.