diff mbox series

[v6,3/5] Add GLIBC_ABI_DT_RELR for DT_RELR support

Message ID 20220310200329.1935466-4-hjl.tools@gmail.com
State New
Headers show
Series Support DT_RELR relative relocation format | expand

Commit Message

H.J. Lu March 10, 2022, 8:03 p.m. UTC 
The EI_ABIVERSION field of the ELF header in executables and shared
libraries can be bumped to indicate the minimum ABI requirement on the
dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
the Linux kernel ELF loader nor the existing dynamic linker.  Executables
will crash mysteriously if the dynamic linker doesn't support the ABI
features required by the EI_ABIVERSION field.  The dynamic linker should
be changed to check EI_ABIVERSION in executables.

Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
that the existing dynamic linkers will issue an error on executables with
GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.

Support __placeholder_only_for_empty_version_map as the placeholder symbol
used only for empty version map to generate GLIBC_ABI_DT_RELR without any
symbols.
---
 elf/Makefile         | 12 ++++++++++--
 elf/Versions         |  5 +++++
 elf/dl-version.c     | 33 +++++++++++++++++++++++++++++++--
 include/link.h       |  6 ++++++
 scripts/abilist.awk  |  2 ++
 scripts/versions.awk |  7 ++++++-
 6 files changed, 60 insertions(+), 5 deletions(-)

Comments

Adhemerval Zanella Netto March 29, 2022, 4:52 p.m. UTC | #1 
On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
> The EI_ABIVERSION field of the ELF header in executables and shared
> libraries can be bumped to indicate the minimum ABI requirement on the
> dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
> the Linux kernel ELF loader nor the existing dynamic linker.  Executables
> will crash mysteriously if the dynamic linker doesn't support the ABI
> features required by the EI_ABIVERSION field.  The dynamic linker should
> be changed to check EI_ABIVERSION in executables.
> 
> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
> that the existing dynamic linkers will issue an error on executables with
> GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
> 
> Support __placeholder_only_for_empty_version_map as the placeholder symbol
> used only for empty version map to generate GLIBC_ABI_DT_RELR without any
> symbols.

I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually 
supports DT_RELR, otherwise if the ABI start to support old glibcs might
wrongly assumes DT_RELR and fails with undefined behavior at runtime
(which this patch essentially tries to avoid).

> ---
>  elf/Makefile         | 12 ++++++++++--
>  elf/Versions         |  5 +++++
>  elf/dl-version.c     | 33 +++++++++++++++++++++++++++++++--
>  include/link.h       |  6 ++++++
>  scripts/abilist.awk  |  2 ++
>  scripts/versions.awk |  7 ++++++-
>  6 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index fd462ba315..d3118b9777 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1113,8 +1113,10 @@ $(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
>  $(eval $(call include_dsosort_tests,dso-sort-tests-2.def))
>  endif
>  
> -check-abi: $(objpfx)check-abi-ld.out
> -tests-special += $(objpfx)check-abi-ld.out
> +check-abi: $(objpfx)check-abi-ld.out \
> +	   $(objpfx)check-abi-version-libc.out
> +tests-special += $(objpfx)check-abi-ld.out \
> +	   $(objpfx)check-abi-version-libc.out

One line per definition.

>  update-abi: update-abi-ld
>  update-all-abi: update-all-abi-ld
>  
> @@ -2739,3 +2741,9 @@ $(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie
>  		| sed -ne '/required from libc.so/,$$ p' \
>  		| grep GLIBC_ABI_DT_RELR > $@; \
>  	$(evaluate-test)
> +
> +$(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so
> +	LC_ALL=C $(READELF) -V -W $< \
> +		| sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \
> +		| grep GLIBC_ABI_DT_RELR > $@; \
> +	$(evaluate-test)

This test should be enable iff for $(have-dt-relr) == yes.

> diff --git a/elf/Versions b/elf/Versions
> index 8bed855d8c..a9ff278de7 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -23,6 +23,11 @@ libc {
>    GLIBC_2.35 {
>      _dl_find_object;
>    }
> +  GLIBC_ABI_DT_RELR {
> +    # This symbol is used only for empty version map and will be removed
> +    # by scripts/versions.awk.
> +    __placeholder_only_for_empty_version_map;
> +  }

Same as before. Maybe use the same strategy we do for time64: add a config.h
define to HAVE_DTRELR (set by configure) and use it to set the symbol if
it is defined.

This define would be used on dl-version.c to also enable the l_abi_version
check.

>    GLIBC_PRIVATE {
>      # functions used in other libraries
>      __libc_early_init;
> diff --git a/elf/dl-version.c b/elf/dl-version.c
> index b47bd91727..720ec596a5 100644
> --- a/elf/dl-version.c
> +++ b/elf/dl-version.c
> @@ -214,12 +214,20 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>  	      while (1)
>  		{
>  		  /* Match the symbol.  */
> +		  const char *string = strtab + aux->vna_name;
>  		  result |= match_symbol (DSO_FILENAME (map->l_name),
>  					  map->l_ns, aux->vna_hash,
> -					  strtab + aux->vna_name,
> -					  needed->l_real, verbose,
> +					  string, needed->l_real, verbose,
>  					  aux->vna_flags & VER_FLG_WEAK);
>  
> +		  if (map->l_abi_version == lav_none
> +		      /* 0xfd0e42: _dl_elf_hash ("GLIBC_ABI_DT_RELR").  */
> +		      && aux->vna_hash == 0xfd0e42
> +		      && __glibc_likely (strcmp (string,
> +						 "GLIBC_ABI_DT_RELR")
> +					 == 0))
> +		    map->l_abi_version = lav_dt_relr_ref;
> +
>  		  /* Compare the version index.  */
>  		  if ((unsigned int) (aux->vna_other & 0x7fff) > ndx_high)
>  		    ndx_high = aux->vna_other & 0x7fff;
> @@ -253,6 +261,16 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>        ent = (ElfW(Verdef) *) (map->l_addr + def->d_un.d_ptr);
>        while (1)
>  	{
> +	  /* 0x0963cf85: _dl_elf_hash ("GLIBC_PRIVATE").  */
> +	  if (ent->vd_hash == 0x0963cf85)
> +	    {
> +	      ElfW(Verdaux) *aux = (ElfW(Verdaux) *) ((char *) ent
> +						      + ent->vd_aux);
> +	      if (__glibc_likely (strcmp ("GLIBC_PRIVATE",
> +					  strtab + aux->vda_name) == 0))
> +		map->l_abi_version = lav_private_def;
> +	    }
> +
>  	  if ((unsigned int) (ent->vd_ndx & 0x7fff) > ndx_high)
>  	    ndx_high = ent->vd_ndx & 0x7fff;
>  
> @@ -352,6 +370,17 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>  	}
>      }
>  
> +  /* Issue an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR
> +     dependency nor GLIBC_PRIVATE definition.  */
> +  if (map->l_info[DT_RELR] != NULL
> +      && __glibc_unlikely (map->l_abi_version == lav_none))
> +    {
> +      _dl_exception_create
> +	(&exception, DSO_FILENAME (map->l_name),
> +	 N_("DT_RELR without GLIBC_ABI_DT_RELR dependency"));
> +      goto call_error;
> +    }
> +
>    return result;
>  }
>  
> diff --git a/include/link.h b/include/link.h
> index 03db14c7b0..8ec5e35cf2 100644
> --- a/include/link.h
> +++ b/include/link.h
> @@ -177,6 +177,12 @@ struct link_map
>  	lt_library,		/* Library needed by main executable.  */
>  	lt_loaded		/* Extra run-time loaded shared object.  */
>        } l_type:2;
> +    enum			/* ABI dependency of this object.  */
> +      {
> +	lav_none,		/* No ABI dependency.  */
> +	lav_dt_relr_ref,	/* Need GLIBC_ABI_DT_RELR.  */
> +	lav_private_def		/* Define GLIBC_PRIVATE.  */
> +      } l_abi_version:2;
>      unsigned int l_relocated:1;	/* Nonzero if object's relocations done.  */
>      unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
>      unsigned int l_global:1;	/* Nonzero if object in _dl_global_scope.  */
> diff --git a/scripts/abilist.awk b/scripts/abilist.awk
> index 24a34ccbed..6cc7af6ac8 100644
> --- a/scripts/abilist.awk
> +++ b/scripts/abilist.awk
> @@ -55,6 +55,8 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) {
>    # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.
>    if (NF > 7 && $7 == ".hidden") next;
>  
> +  if (version ~ /^GLIBC_ABI_/ && !include_abi_version) next;
> +
>    if (version == "GLIBC_PRIVATE" && !include_private) next;
>  
>    desc = "";
> diff --git a/scripts/versions.awk b/scripts/versions.awk
> index 357ad1355e..d70b07bd1a 100644
> --- a/scripts/versions.awk
> +++ b/scripts/versions.awk
> @@ -185,8 +185,13 @@ END {
>  	closeversion(oldver, veryoldver);
>  	veryoldver = oldver;
>        }
> -      printf("%s {\n  global:\n", $2) > outfile;
>        oldver = $2;
> +      # Skip the placeholder symbol used only for empty version map.
> +      if ($3 == "__placeholder_only_for_empty_version_map;") {
> +	printf("%s {\n", $2) > outfile;
> +	continue;
> +      }
> +      printf("%s {\n  global:\n", $2) > outfile;
>      }
>      printf("   ") > outfile;
>      for (n = 3; n <= NF; ++n) {
H.J. Lu March 29, 2022, 10:29 p.m. UTC | #2 
On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
> > The EI_ABIVERSION field of the ELF header in executables and shared
> > libraries can be bumped to indicate the minimum ABI requirement on the
> > dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
> > the Linux kernel ELF loader nor the existing dynamic linker.  Executables
> > will crash mysteriously if the dynamic linker doesn't support the ABI
> > features required by the EI_ABIVERSION field.  The dynamic linker should
> > be changed to check EI_ABIVERSION in executables.
> >
> > Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
> > that the existing dynamic linkers will issue an error on executables with
> > GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
> > without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
> >
> > Support __placeholder_only_for_empty_version_map as the placeholder symbol
> > used only for empty version map to generate GLIBC_ABI_DT_RELR without any
> > symbols.
>
> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually
> supports DT_RELR, otherwise if the ABI start to support old glibcs might
> wrongly assumes DT_RELR and fails with undefined behavior at runtime
> (which this patch essentially tries to avoid).

The DT_RELR set enables DT_RELR in glibc for all targets.

> > ---
> >  elf/Makefile         | 12 ++++++++++--
> >  elf/Versions         |  5 +++++
> >  elf/dl-version.c     | 33 +++++++++++++++++++++++++++++++--
> >  include/link.h       |  6 ++++++
> >  scripts/abilist.awk  |  2 ++
> >  scripts/versions.awk |  7 ++++++-
> >  6 files changed, 60 insertions(+), 5 deletions(-)
> >
> > diff --git a/elf/Makefile b/elf/Makefile
> > index fd462ba315..d3118b9777 100644
> > --- a/elf/Makefile
> > +++ b/elf/Makefile
> > @@ -1113,8 +1113,10 @@ $(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
> >  $(eval $(call include_dsosort_tests,dso-sort-tests-2.def))
> >  endif
> >
> > -check-abi: $(objpfx)check-abi-ld.out
> > -tests-special += $(objpfx)check-abi-ld.out
> > +check-abi: $(objpfx)check-abi-ld.out \
> > +        $(objpfx)check-abi-version-libc.out
> > +tests-special += $(objpfx)check-abi-ld.out \
> > +        $(objpfx)check-abi-version-libc.out
>
> One line per definition.

Fixed in v7.

> >  update-abi: update-abi-ld
> >  update-all-abi: update-all-abi-ld
> >
> > @@ -2739,3 +2741,9 @@ $(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie
> >               | sed -ne '/required from libc.so/,$$ p' \
> >               | grep GLIBC_ABI_DT_RELR > $@; \
> >       $(evaluate-test)
> > +
> > +$(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so
> > +     LC_ALL=C $(READELF) -V -W $< \
> > +             | sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \
> > +             | grep GLIBC_ABI_DT_RELR > $@; \
> > +     $(evaluate-test)
>
> This test should be enable iff for $(have-dt-relr) == yes.

This test checks if libc.so has the GLIBC_ABI_DT_RELR
version definition.   Since it doesn't require a new linker,
there is no need to check $(have-dt-relr).   This test
passes with an old linker.

> > diff --git a/elf/Versions b/elf/Versions
> > index 8bed855d8c..a9ff278de7 100644
> > --- a/elf/Versions
> > +++ b/elf/Versions
> > @@ -23,6 +23,11 @@ libc {
> >    GLIBC_2.35 {
> >      _dl_find_object;
> >    }
> > +  GLIBC_ABI_DT_RELR {
> > +    # This symbol is used only for empty version map and will be removed
> > +    # by scripts/versions.awk.
> > +    __placeholder_only_for_empty_version_map;
> > +  }
>
> Same as before. Maybe use the same strategy we do for time64: add a config.h
> define to HAVE_DTRELR (set by configure) and use it to set the symbol if
> it is defined.
>
> This define would be used on dl-version.c to also enable the l_abi_version
> check.
>
> >    GLIBC_PRIVATE {
> >      # functions used in other libraries
> >      __libc_early_init;
> > diff --git a/elf/dl-version.c b/elf/dl-version.c
> > index b47bd91727..720ec596a5 100644
> > --- a/elf/dl-version.c
> > +++ b/elf/dl-version.c
> > @@ -214,12 +214,20 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> >             while (1)
> >               {
> >                 /* Match the symbol.  */
> > +               const char *string = strtab + aux->vna_name;
> >                 result |= match_symbol (DSO_FILENAME (map->l_name),
> >                                         map->l_ns, aux->vna_hash,
> > -                                       strtab + aux->vna_name,
> > -                                       needed->l_real, verbose,
> > +                                       string, needed->l_real, verbose,
> >                                         aux->vna_flags & VER_FLG_WEAK);
> >
> > +               if (map->l_abi_version == lav_none
> > +                   /* 0xfd0e42: _dl_elf_hash ("GLIBC_ABI_DT_RELR").  */
> > +                   && aux->vna_hash == 0xfd0e42
> > +                   && __glibc_likely (strcmp (string,
> > +                                              "GLIBC_ABI_DT_RELR")
> > +                                      == 0))
> > +                 map->l_abi_version = lav_dt_relr_ref;
> > +
> >                 /* Compare the version index.  */
> >                 if ((unsigned int) (aux->vna_other & 0x7fff) > ndx_high)
> >                   ndx_high = aux->vna_other & 0x7fff;
> > @@ -253,6 +261,16 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> >        ent = (ElfW(Verdef) *) (map->l_addr + def->d_un.d_ptr);
> >        while (1)
> >       {
> > +       /* 0x0963cf85: _dl_elf_hash ("GLIBC_PRIVATE").  */
> > +       if (ent->vd_hash == 0x0963cf85)
> > +         {
> > +           ElfW(Verdaux) *aux = (ElfW(Verdaux) *) ((char *) ent
> > +                                                   + ent->vd_aux);
> > +           if (__glibc_likely (strcmp ("GLIBC_PRIVATE",
> > +                                       strtab + aux->vda_name) == 0))
> > +             map->l_abi_version = lav_private_def;
> > +         }
> > +
> >         if ((unsigned int) (ent->vd_ndx & 0x7fff) > ndx_high)
> >           ndx_high = ent->vd_ndx & 0x7fff;
> >
> > @@ -352,6 +370,17 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> >       }
> >      }
> >
> > +  /* Issue an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR
> > +     dependency nor GLIBC_PRIVATE definition.  */
> > +  if (map->l_info[DT_RELR] != NULL
> > +      && __glibc_unlikely (map->l_abi_version == lav_none))
> > +    {
> > +      _dl_exception_create
> > +     (&exception, DSO_FILENAME (map->l_name),
> > +      N_("DT_RELR without GLIBC_ABI_DT_RELR dependency"));
> > +      goto call_error;
> > +    }
> > +
> >    return result;
> >  }
> >
> > diff --git a/include/link.h b/include/link.h
> > index 03db14c7b0..8ec5e35cf2 100644
> > --- a/include/link.h
> > +++ b/include/link.h
> > @@ -177,6 +177,12 @@ struct link_map
> >       lt_library,             /* Library needed by main executable.  */
> >       lt_loaded               /* Extra run-time loaded shared object.  */
> >        } l_type:2;
> > +    enum                     /* ABI dependency of this object.  */
> > +      {
> > +     lav_none,               /* No ABI dependency.  */
> > +     lav_dt_relr_ref,        /* Need GLIBC_ABI_DT_RELR.  */
> > +     lav_private_def         /* Define GLIBC_PRIVATE.  */
> > +      } l_abi_version:2;
> >      unsigned int l_relocated:1;      /* Nonzero if object's relocations done.  */
> >      unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
> >      unsigned int l_global:1; /* Nonzero if object in _dl_global_scope.  */
> > diff --git a/scripts/abilist.awk b/scripts/abilist.awk
> > index 24a34ccbed..6cc7af6ac8 100644
> > --- a/scripts/abilist.awk
> > +++ b/scripts/abilist.awk
> > @@ -55,6 +55,8 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) {
> >    # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.
> >    if (NF > 7 && $7 == ".hidden") next;
> >
> > +  if (version ~ /^GLIBC_ABI_/ && !include_abi_version) next;
> > +
> >    if (version == "GLIBC_PRIVATE" && !include_private) next;
> >
> >    desc = "";
> > diff --git a/scripts/versions.awk b/scripts/versions.awk
> > index 357ad1355e..d70b07bd1a 100644
> > --- a/scripts/versions.awk
> > +++ b/scripts/versions.awk
> > @@ -185,8 +185,13 @@ END {
> >       closeversion(oldver, veryoldver);
> >       veryoldver = oldver;
> >        }
> > -      printf("%s {\n  global:\n", $2) > outfile;
> >        oldver = $2;
> > +      # Skip the placeholder symbol used only for empty version map.
> > +      if ($3 == "__placeholder_only_for_empty_version_map;") {
> > +     printf("%s {\n", $2) > outfile;
> > +     continue;
> > +      }
> > +      printf("%s {\n  global:\n", $2) > outfile;
> >      }
> >      printf("   ") > outfile;
> >      for (n = 3; n <= NF; ++n) {
Adhemerval Zanella Netto March 30, 2022, 2:18 p.m. UTC | #3 
On 29/03/2022 19:29, H.J. Lu wrote:
> On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
>>> The EI_ABIVERSION field of the ELF header in executables and shared
>>> libraries can be bumped to indicate the minimum ABI requirement on the
>>> dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
>>> the Linux kernel ELF loader nor the existing dynamic linker.  Executables
>>> will crash mysteriously if the dynamic linker doesn't support the ABI
>>> features required by the EI_ABIVERSION field.  The dynamic linker should
>>> be changed to check EI_ABIVERSION in executables.
>>>
>>> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
>>> that the existing dynamic linkers will issue an error on executables with
>>> GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
>>> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
>>>
>>> Support __placeholder_only_for_empty_version_map as the placeholder symbol
>>> used only for empty version map to generate GLIBC_ABI_DT_RELR without any
>>> symbols.
>>
>> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually
>> supports DT_RELR, otherwise if the ABI start to support old glibcs might
>> wrongly assumes DT_RELR and fails with undefined behavior at runtime
>> (which this patch essentially tries to avoid).
> 
> The DT_RELR set enables DT_RELR in glibc for all targets.
If I understood correctly, if we add GLIBC_ABI_DT_RELR on 2.36 but only enable
it on some architecture on 2.37 and try to run a binary with DT_RELR enabled
on glibc 2.36, _dl_check_map_versions won't accuse a failure and it will
eventually only fail at runtime (since relocation won't be applied).  My
understanding we are trying to avoid such failures.

> 
>>> ---
>>>  elf/Makefile         | 12 ++++++++++--
>>>  elf/Versions         |  5 +++++
>>>  elf/dl-version.c     | 33 +++++++++++++++++++++++++++++++--
>>>  include/link.h       |  6 ++++++
>>>  scripts/abilist.awk  |  2 ++
>>>  scripts/versions.awk |  7 ++++++-
>>>  6 files changed, 60 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/elf/Makefile b/elf/Makefile
>>> index fd462ba315..d3118b9777 100644
>>> --- a/elf/Makefile
>>> +++ b/elf/Makefile
>>> @@ -1113,8 +1113,10 @@ $(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
>>>  $(eval $(call include_dsosort_tests,dso-sort-tests-2.def))
>>>  endif
>>>
>>> -check-abi: $(objpfx)check-abi-ld.out
>>> -tests-special += $(objpfx)check-abi-ld.out
>>> +check-abi: $(objpfx)check-abi-ld.out \
>>> +        $(objpfx)check-abi-version-libc.out
>>> +tests-special += $(objpfx)check-abi-ld.out \
>>> +        $(objpfx)check-abi-version-libc.out
>>
>> One line per definition.
> 
> Fixed in v7.
> 
>>>  update-abi: update-abi-ld
>>>  update-all-abi: update-all-abi-ld
>>>
>>> @@ -2739,3 +2741,9 @@ $(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie
>>>               | sed -ne '/required from libc.so/,$$ p' \
>>>               | grep GLIBC_ABI_DT_RELR > $@; \
>>>       $(evaluate-test)
>>> +
>>> +$(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so
>>> +     LC_ALL=C $(READELF) -V -W $< \
>>> +             | sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \
>>> +             | grep GLIBC_ABI_DT_RELR > $@; \
>>> +     $(evaluate-test)
>>
>> This test should be enable iff for $(have-dt-relr) == yes.
> 
> This test checks if libc.so has the GLIBC_ABI_DT_RELR
> version definition.   Since it doesn't require a new linker,
> there is no need to check $(have-dt-relr).   This test
> passes with an old linker.
> 
>>> diff --git a/elf/Versions b/elf/Versions
>>> index 8bed855d8c..a9ff278de7 100644
>>> --- a/elf/Versions
>>> +++ b/elf/Versions
>>> @@ -23,6 +23,11 @@ libc {
>>>    GLIBC_2.35 {
>>>      _dl_find_object;
>>>    }
>>> +  GLIBC_ABI_DT_RELR {
>>> +    # This symbol is used only for empty version map and will be removed
>>> +    # by scripts/versions.awk.
>>> +    __placeholder_only_for_empty_version_map;
>>> +  }
>>
>> Same as before. Maybe use the same strategy we do for time64: add a config.h
>> define to HAVE_DTRELR (set by configure) and use it to set the symbol if
>> it is defined.
>>
>> This define would be used on dl-version.c to also enable the l_abi_version
>> check.
>>
>>>    GLIBC_PRIVATE {
>>>      # functions used in other libraries
>>>      __libc_early_init;
>>> diff --git a/elf/dl-version.c b/elf/dl-version.c
>>> index b47bd91727..720ec596a5 100644
>>> --- a/elf/dl-version.c
>>> +++ b/elf/dl-version.c
>>> @@ -214,12 +214,20 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>>>             while (1)
>>>               {
>>>                 /* Match the symbol.  */
>>> +               const char *string = strtab + aux->vna_name;
>>>                 result |= match_symbol (DSO_FILENAME (map->l_name),
>>>                                         map->l_ns, aux->vna_hash,
>>> -                                       strtab + aux->vna_name,
>>> -                                       needed->l_real, verbose,
>>> +                                       string, needed->l_real, verbose,
>>>                                         aux->vna_flags & VER_FLG_WEAK);
>>>
>>> +               if (map->l_abi_version == lav_none
>>> +                   /* 0xfd0e42: _dl_elf_hash ("GLIBC_ABI_DT_RELR").  */
>>> +                   && aux->vna_hash == 0xfd0e42
>>> +                   && __glibc_likely (strcmp (string,
>>> +                                              "GLIBC_ABI_DT_RELR")
>>> +                                      == 0))
>>> +                 map->l_abi_version = lav_dt_relr_ref;
>>> +
>>>                 /* Compare the version index.  */
>>>                 if ((unsigned int) (aux->vna_other & 0x7fff) > ndx_high)
>>>                   ndx_high = aux->vna_other & 0x7fff;
>>> @@ -253,6 +261,16 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>>>        ent = (ElfW(Verdef) *) (map->l_addr + def->d_un.d_ptr);
>>>        while (1)
>>>       {
>>> +       /* 0x0963cf85: _dl_elf_hash ("GLIBC_PRIVATE").  */
>>> +       if (ent->vd_hash == 0x0963cf85)
>>> +         {
>>> +           ElfW(Verdaux) *aux = (ElfW(Verdaux) *) ((char *) ent
>>> +                                                   + ent->vd_aux);
>>> +           if (__glibc_likely (strcmp ("GLIBC_PRIVATE",
>>> +                                       strtab + aux->vda_name) == 0))
>>> +             map->l_abi_version = lav_private_def;
>>> +         }
>>> +
>>>         if ((unsigned int) (ent->vd_ndx & 0x7fff) > ndx_high)
>>>           ndx_high = ent->vd_ndx & 0x7fff;
>>>
>>> @@ -352,6 +370,17 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>>>       }
>>>      }
>>>
>>> +  /* Issue an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR
>>> +     dependency nor GLIBC_PRIVATE definition.  */
>>> +  if (map->l_info[DT_RELR] != NULL
>>> +      && __glibc_unlikely (map->l_abi_version == lav_none))
>>> +    {
>>> +      _dl_exception_create
>>> +     (&exception, DSO_FILENAME (map->l_name),
>>> +      N_("DT_RELR without GLIBC_ABI_DT_RELR dependency"));
>>> +      goto call_error;
>>> +    }
>>> +
>>>    return result;
>>>  }
>>>
>>> diff --git a/include/link.h b/include/link.h
>>> index 03db14c7b0..8ec5e35cf2 100644
>>> --- a/include/link.h
>>> +++ b/include/link.h
>>> @@ -177,6 +177,12 @@ struct link_map
>>>       lt_library,             /* Library needed by main executable.  */
>>>       lt_loaded               /* Extra run-time loaded shared object.  */
>>>        } l_type:2;
>>> +    enum                     /* ABI dependency of this object.  */
>>> +      {
>>> +     lav_none,               /* No ABI dependency.  */
>>> +     lav_dt_relr_ref,        /* Need GLIBC_ABI_DT_RELR.  */
>>> +     lav_private_def         /* Define GLIBC_PRIVATE.  */
>>> +      } l_abi_version:2;
>>>      unsigned int l_relocated:1;      /* Nonzero if object's relocations done.  */
>>>      unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
>>>      unsigned int l_global:1; /* Nonzero if object in _dl_global_scope.  */
>>> diff --git a/scripts/abilist.awk b/scripts/abilist.awk
>>> index 24a34ccbed..6cc7af6ac8 100644
>>> --- a/scripts/abilist.awk
>>> +++ b/scripts/abilist.awk
>>> @@ -55,6 +55,8 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) {
>>>    # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.
>>>    if (NF > 7 && $7 == ".hidden") next;
>>>
>>> +  if (version ~ /^GLIBC_ABI_/ && !include_abi_version) next;
>>> +
>>>    if (version == "GLIBC_PRIVATE" && !include_private) next;
>>>
>>>    desc = "";
>>> diff --git a/scripts/versions.awk b/scripts/versions.awk
>>> index 357ad1355e..d70b07bd1a 100644
>>> --- a/scripts/versions.awk
>>> +++ b/scripts/versions.awk
>>> @@ -185,8 +185,13 @@ END {
>>>       closeversion(oldver, veryoldver);
>>>       veryoldver = oldver;
>>>        }
>>> -      printf("%s {\n  global:\n", $2) > outfile;
>>>        oldver = $2;
>>> +      # Skip the placeholder symbol used only for empty version map.
>>> +      if ($3 == "__placeholder_only_for_empty_version_map;") {
>>> +     printf("%s {\n", $2) > outfile;
>>> +     continue;
>>> +      }
>>> +      printf("%s {\n  global:\n", $2) > outfile;
>>>      }
>>>      printf("   ") > outfile;
>>>      for (n = 3; n <= NF; ++n) {
> 
> 
>
H.J. Lu March 30, 2022, 2:41 p.m. UTC | #4 
On Wed, Mar 30, 2022 at 7:18 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 29/03/2022 19:29, H.J. Lu wrote:
> > On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
> >>> The EI_ABIVERSION field of the ELF header in executables and shared
> >>> libraries can be bumped to indicate the minimum ABI requirement on the
> >>> dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
> >>> the Linux kernel ELF loader nor the existing dynamic linker.  Executables
> >>> will crash mysteriously if the dynamic linker doesn't support the ABI
> >>> features required by the EI_ABIVERSION field.  The dynamic linker should
> >>> be changed to check EI_ABIVERSION in executables.
> >>>
> >>> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
> >>> that the existing dynamic linkers will issue an error on executables with
> >>> GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
> >>> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
> >>>
> >>> Support __placeholder_only_for_empty_version_map as the placeholder symbol
> >>> used only for empty version map to generate GLIBC_ABI_DT_RELR without any
> >>> symbols.
> >>
> >> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually
> >> supports DT_RELR, otherwise if the ABI start to support old glibcs might
> >> wrongly assumes DT_RELR and fails with undefined behavior at runtime
> >> (which this patch essentially tries to avoid).
> >
> > The DT_RELR set enables DT_RELR in glibc for all targets.
> If I understood correctly, if we add GLIBC_ABI_DT_RELR on 2.36 but only enable
> it on some architecture on 2.37 and try to run a binary with DT_RELR enabled
> on glibc 2.36, _dl_check_map_versions won't accuse a failure and it will
> eventually only fail at runtime (since relocation won't be applied).  My
> understanding we are trying to avoid such failures.

My patch set enables DT_RELR and GLIBC_ABI_DT_RELR on glibc 2.36
for all, regardless of binutils version.   There is no per-arch
behavior.    Only
DT_RELR run-time tests are enabled for linkers with -z pack-relative-relocs.

> >
> >>> ---
> >>>  elf/Makefile         | 12 ++++++++++--
> >>>  elf/Versions         |  5 +++++
> >>>  elf/dl-version.c     | 33 +++++++++++++++++++++++++++++++--
> >>>  include/link.h       |  6 ++++++
> >>>  scripts/abilist.awk  |  2 ++
> >>>  scripts/versions.awk |  7 ++++++-
> >>>  6 files changed, 60 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/elf/Makefile b/elf/Makefile
> >>> index fd462ba315..d3118b9777 100644
> >>> --- a/elf/Makefile
> >>> +++ b/elf/Makefile
> >>> @@ -1113,8 +1113,10 @@ $(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
> >>>  $(eval $(call include_dsosort_tests,dso-sort-tests-2.def))
> >>>  endif
> >>>
> >>> -check-abi: $(objpfx)check-abi-ld.out
> >>> -tests-special += $(objpfx)check-abi-ld.out
> >>> +check-abi: $(objpfx)check-abi-ld.out \
> >>> +        $(objpfx)check-abi-version-libc.out
> >>> +tests-special += $(objpfx)check-abi-ld.out \
> >>> +        $(objpfx)check-abi-version-libc.out
> >>
> >> One line per definition.
> >
> > Fixed in v7.
> >
> >>>  update-abi: update-abi-ld
> >>>  update-all-abi: update-all-abi-ld
> >>>
> >>> @@ -2739,3 +2741,9 @@ $(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie
> >>>               | sed -ne '/required from libc.so/,$$ p' \
> >>>               | grep GLIBC_ABI_DT_RELR > $@; \
> >>>       $(evaluate-test)
> >>> +
> >>> +$(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so
> >>> +     LC_ALL=C $(READELF) -V -W $< \
> >>> +             | sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \
> >>> +             | grep GLIBC_ABI_DT_RELR > $@; \
> >>> +     $(evaluate-test)
> >>
> >> This test should be enable iff for $(have-dt-relr) == yes.
> >
> > This test checks if libc.so has the GLIBC_ABI_DT_RELR
> > version definition.   Since it doesn't require a new linker,
> > there is no need to check $(have-dt-relr).   This test
> > passes with an old linker.
> >
> >>> diff --git a/elf/Versions b/elf/Versions
> >>> index 8bed855d8c..a9ff278de7 100644
> >>> --- a/elf/Versions
> >>> +++ b/elf/Versions
> >>> @@ -23,6 +23,11 @@ libc {
> >>>    GLIBC_2.35 {
> >>>      _dl_find_object;
> >>>    }
> >>> +  GLIBC_ABI_DT_RELR {
> >>> +    # This symbol is used only for empty version map and will be removed
> >>> +    # by scripts/versions.awk.
> >>> +    __placeholder_only_for_empty_version_map;
> >>> +  }
> >>
> >> Same as before. Maybe use the same strategy we do for time64: add a config.h
> >> define to HAVE_DTRELR (set by configure) and use it to set the symbol if
> >> it is defined.
> >>
> >> This define would be used on dl-version.c to also enable the l_abi_version
> >> check.
> >>
> >>>    GLIBC_PRIVATE {
> >>>      # functions used in other libraries
> >>>      __libc_early_init;
> >>> diff --git a/elf/dl-version.c b/elf/dl-version.c
> >>> index b47bd91727..720ec596a5 100644
> >>> --- a/elf/dl-version.c
> >>> +++ b/elf/dl-version.c
> >>> @@ -214,12 +214,20 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> >>>             while (1)
> >>>               {
> >>>                 /* Match the symbol.  */
> >>> +               const char *string = strtab + aux->vna_name;
> >>>                 result |= match_symbol (DSO_FILENAME (map->l_name),
> >>>                                         map->l_ns, aux->vna_hash,
> >>> -                                       strtab + aux->vna_name,
> >>> -                                       needed->l_real, verbose,
> >>> +                                       string, needed->l_real, verbose,
> >>>                                         aux->vna_flags & VER_FLG_WEAK);
> >>>
> >>> +               if (map->l_abi_version == lav_none
> >>> +                   /* 0xfd0e42: _dl_elf_hash ("GLIBC_ABI_DT_RELR").  */
> >>> +                   && aux->vna_hash == 0xfd0e42
> >>> +                   && __glibc_likely (strcmp (string,
> >>> +                                              "GLIBC_ABI_DT_RELR")
> >>> +                                      == 0))
> >>> +                 map->l_abi_version = lav_dt_relr_ref;
> >>> +
> >>>                 /* Compare the version index.  */
> >>>                 if ((unsigned int) (aux->vna_other & 0x7fff) > ndx_high)
> >>>                   ndx_high = aux->vna_other & 0x7fff;
> >>> @@ -253,6 +261,16 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> >>>        ent = (ElfW(Verdef) *) (map->l_addr + def->d_un.d_ptr);
> >>>        while (1)
> >>>       {
> >>> +       /* 0x0963cf85: _dl_elf_hash ("GLIBC_PRIVATE").  */
> >>> +       if (ent->vd_hash == 0x0963cf85)
> >>> +         {
> >>> +           ElfW(Verdaux) *aux = (ElfW(Verdaux) *) ((char *) ent
> >>> +                                                   + ent->vd_aux);
> >>> +           if (__glibc_likely (strcmp ("GLIBC_PRIVATE",
> >>> +                                       strtab + aux->vda_name) == 0))
> >>> +             map->l_abi_version = lav_private_def;
> >>> +         }
> >>> +
> >>>         if ((unsigned int) (ent->vd_ndx & 0x7fff) > ndx_high)
> >>>           ndx_high = ent->vd_ndx & 0x7fff;
> >>>
> >>> @@ -352,6 +370,17 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> >>>       }
> >>>      }
> >>>
> >>> +  /* Issue an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR
> >>> +     dependency nor GLIBC_PRIVATE definition.  */
> >>> +  if (map->l_info[DT_RELR] != NULL
> >>> +      && __glibc_unlikely (map->l_abi_version == lav_none))
> >>> +    {
> >>> +      _dl_exception_create
> >>> +     (&exception, DSO_FILENAME (map->l_name),
> >>> +      N_("DT_RELR without GLIBC_ABI_DT_RELR dependency"));
> >>> +      goto call_error;
> >>> +    }
> >>> +
> >>>    return result;
> >>>  }
> >>>
> >>> diff --git a/include/link.h b/include/link.h
> >>> index 03db14c7b0..8ec5e35cf2 100644
> >>> --- a/include/link.h
> >>> +++ b/include/link.h
> >>> @@ -177,6 +177,12 @@ struct link_map
> >>>       lt_library,             /* Library needed by main executable.  */
> >>>       lt_loaded               /* Extra run-time loaded shared object.  */
> >>>        } l_type:2;
> >>> +    enum                     /* ABI dependency of this object.  */
> >>> +      {
> >>> +     lav_none,               /* No ABI dependency.  */
> >>> +     lav_dt_relr_ref,        /* Need GLIBC_ABI_DT_RELR.  */
> >>> +     lav_private_def         /* Define GLIBC_PRIVATE.  */
> >>> +      } l_abi_version:2;
> >>>      unsigned int l_relocated:1;      /* Nonzero if object's relocations done.  */
> >>>      unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
> >>>      unsigned int l_global:1; /* Nonzero if object in _dl_global_scope.  */
> >>> diff --git a/scripts/abilist.awk b/scripts/abilist.awk
> >>> index 24a34ccbed..6cc7af6ac8 100644
> >>> --- a/scripts/abilist.awk
> >>> +++ b/scripts/abilist.awk
> >>> @@ -55,6 +55,8 @@ $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) {
> >>>    # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.
> >>>    if (NF > 7 && $7 == ".hidden") next;
> >>>
> >>> +  if (version ~ /^GLIBC_ABI_/ && !include_abi_version) next;
> >>> +
> >>>    if (version == "GLIBC_PRIVATE" && !include_private) next;
> >>>
> >>>    desc = "";
> >>> diff --git a/scripts/versions.awk b/scripts/versions.awk
> >>> index 357ad1355e..d70b07bd1a 100644
> >>> --- a/scripts/versions.awk
> >>> +++ b/scripts/versions.awk
> >>> @@ -185,8 +185,13 @@ END {
> >>>       closeversion(oldver, veryoldver);
> >>>       veryoldver = oldver;
> >>>        }
> >>> -      printf("%s {\n  global:\n", $2) > outfile;
> >>>        oldver = $2;
> >>> +      # Skip the placeholder symbol used only for empty version map.
> >>> +      if ($3 == "__placeholder_only_for_empty_version_map;") {
> >>> +     printf("%s {\n", $2) > outfile;
> >>> +     continue;
> >>> +      }
> >>> +      printf("%s {\n  global:\n", $2) > outfile;
> >>>      }
> >>>      printf("   ") > outfile;
> >>>      for (n = 3; n <= NF; ++n) {
> >
> >
> >
Adhemerval Zanella Netto March 30, 2022, 5:17 p.m. UTC | #5 
On 30/03/2022 11:41, H.J. Lu wrote:
> On Wed, Mar 30, 2022 at 7:18 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 29/03/2022 19:29, H.J. Lu wrote:
>>> On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
>>>>> The EI_ABIVERSION field of the ELF header in executables and shared
>>>>> libraries can be bumped to indicate the minimum ABI requirement on the
>>>>> dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
>>>>> the Linux kernel ELF loader nor the existing dynamic linker.  Executables
>>>>> will crash mysteriously if the dynamic linker doesn't support the ABI
>>>>> features required by the EI_ABIVERSION field.  The dynamic linker should
>>>>> be changed to check EI_ABIVERSION in executables.
>>>>>
>>>>> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
>>>>> that the existing dynamic linkers will issue an error on executables with
>>>>> GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
>>>>> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
>>>>>
>>>>> Support __placeholder_only_for_empty_version_map as the placeholder symbol
>>>>> used only for empty version map to generate GLIBC_ABI_DT_RELR without any
>>>>> symbols.
>>>>
>>>> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually
>>>> supports DT_RELR, otherwise if the ABI start to support old glibcs might
>>>> wrongly assumes DT_RELR and fails with undefined behavior at runtime
>>>> (which this patch essentially tries to avoid).
>>>
>>> The DT_RELR set enables DT_RELR in glibc for all targets.
>> If I understood correctly, if we add GLIBC_ABI_DT_RELR on 2.36 but only enable
>> it on some architecture on 2.37 and try to run a binary with DT_RELR enabled
>> on glibc 2.36, _dl_check_map_versions won't accuse a failure and it will
>> eventually only fail at runtime (since relocation won't be applied).  My
>> understanding we are trying to avoid such failures.
> 
> My patch set enables DT_RELR and GLIBC_ABI_DT_RELR on glibc 2.36
> for all, regardless of binutils version.   There is no per-arch
> behavior.    Only
> DT_RELR run-time tests are enabled for linkers with -z pack-relative-relocs.

But it still does not fail when the architecture does not actually implement
RELR even when static linker does:

$ clang --target=aarch64-linux-gnu -fuse-ld=lld -Wl,-z,pack-relative-relocs -fpie -o test -pie test.c
$ aarch64-glibc-linux-gnu-readelf -S test | grep relr\.dyn
  [ 9] .relr.dyn         RELR             0000000000000528  00000528
$ ./elf/ld.so --library-path . ./test
$

I would expect that even when binary adds GLIBC_ABI_DT_RELR, if dynamic linker
does not have DT_RELR it would fail early.
H.J. Lu March 30, 2022, 5:32 p.m. UTC | #6 
Does the linker add the version dependency? Does ld.so support DT_RELR?

On Wed, Mar 30, 2022, 10:17 AM Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

>
>
> On 30/03/2022 11:41, H.J. Lu wrote:
> > On Wed, Mar 30, 2022 at 7:18 AM Adhemerval Zanella
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 29/03/2022 19:29, H.J. Lu wrote:
> >>> On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella
> >>> <adhemerval.zanella@linaro.org> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
> >>>>> The EI_ABIVERSION field of the ELF header in executables and shared
> >>>>> libraries can be bumped to indicate the minimum ABI requirement on
> the
> >>>>> dynamic linker.  However, EI_ABIVERSION in executables isn't checked
> by
> >>>>> the Linux kernel ELF loader nor the existing dynamic linker.
> Executables
> >>>>> will crash mysteriously if the dynamic linker doesn't support the ABI
> >>>>> features required by the EI_ABIVERSION field.  The dynamic linker
> should
> >>>>> be changed to check EI_ABIVERSION in executables.
> >>>>>
> >>>>> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support
> so
> >>>>> that the existing dynamic linkers will issue an error on executables
> with
> >>>>> GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR
> entry
> >>>>> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
> >>>>>
> >>>>> Support __placeholder_only_for_empty_version_map as the placeholder
> symbol
> >>>>> used only for empty version map to generate GLIBC_ABI_DT_RELR
> without any
> >>>>> symbols.
> >>>>
> >>>> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI
> actually
> >>>> supports DT_RELR, otherwise if the ABI start to support old glibcs
> might
> >>>> wrongly assumes DT_RELR and fails with undefined behavior at runtime
> >>>> (which this patch essentially tries to avoid).
> >>>
> >>> The DT_RELR set enables DT_RELR in glibc for all targets.
> >> If I understood correctly, if we add GLIBC_ABI_DT_RELR on 2.36 but only
> enable
> >> it on some architecture on 2.37 and try to run a binary with DT_RELR
> enabled
> >> on glibc 2.36, _dl_check_map_versions won't accuse a failure and it will
> >> eventually only fail at runtime (since relocation won't be applied).  My
> >> understanding we are trying to avoid such failures.
> >
> > My patch set enables DT_RELR and GLIBC_ABI_DT_RELR on glibc 2.36
> > for all, regardless of binutils version.   There is no per-arch
> > behavior.    Only
> > DT_RELR run-time tests are enabled for linkers with -z
> pack-relative-relocs.
>
> But it still does not fail when the architecture does not actually
> implement
> RELR even when static linker does:
>
> $ clang --target=aarch64-linux-gnu -fuse-ld=lld
> -Wl,-z,pack-relative-relocs -fpie -o test -pie test.c
> $ aarch64-glibc-linux-gnu-readelf -S test | grep relr\.dyn
>   [ 9] .relr.dyn         RELR             0000000000000528  00000528
> $ ./elf/ld.so --library-path . ./test
> $
>
> I would expect that even when binary adds GLIBC_ABI_DT_RELR, if dynamic
> linker
> does not have DT_RELR it would fail early.
>
>
Adhemerval Zanella Netto March 30, 2022, 5:39 p.m. UTC | #7 
On 30/03/2022 14:32, H.J. Lu wrote:
> Does the linker add the version dependency? Does ld.so support DT_RELR?
> 

$ readelf -V test | grep GLIBC_ABI_DT_RELR
  0x0030:   Name: GLIBC_ABI_DT_RELR  Flags: none  Version: 4

But ld.so does not support DT_RELR

$ grep libc_cv_dt_relr config.log 
libc_cv_dt_relr=no


> On Wed, Mar 30, 2022, 10:17 AM Adhemerval Zanella <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
> 
> 
> 
>     On 30/03/2022 11:41, H.J. Lu wrote:
>     > On Wed, Mar 30, 2022 at 7:18 AM Adhemerval Zanella
>     > <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
>     >>
>     >>
>     >>
>     >> On 29/03/2022 19:29, H.J. Lu wrote:
>     >>> On Tue, Mar 29, 2022 at 9:52 AM Adhemerval Zanella
>     >>> <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
>     >>>>
>     >>>>
>     >>>>
>     >>>> On 10/03/2022 17:03, H.J. Lu via Libc-alpha wrote:
>     >>>>> The EI_ABIVERSION field of the ELF header in executables and shared
>     >>>>> libraries can be bumped to indicate the minimum ABI requirement on the
>     >>>>> dynamic linker.  However, EI_ABIVERSION in executables isn't checked by
>     >>>>> the Linux kernel ELF loader nor the existing dynamic linker.  Executables
>     >>>>> will crash mysteriously if the dynamic linker doesn't support the ABI
>     >>>>> features required by the EI_ABIVERSION field.  The dynamic linker should
>     >>>>> be changed to check EI_ABIVERSION in executables.
>     >>>>>
>     >>>>> Add a glibc version, GLIBC_ABI_DT_RELR, to indicate DT_RELR support so
>     >>>>> that the existing dynamic linkers will issue an error on executables with
>     >>>>> GLIBC_ABI_DT_RELR dependency.  Issue an error if there is a DT_RELR entry
>     >>>>> without GLIBC_ABI_DT_RELR dependency nor GLIBC_PRIVATE definition.
>     >>>>>
>     >>>>> Support __placeholder_only_for_empty_version_map as the placeholder symbol
>     >>>>> used only for empty version map to generate GLIBC_ABI_DT_RELR without any
>     >>>>> symbols.
>     >>>>
>     >>>> I think it only make sense to add the GLIBC_ABI_DT_RELR if the ABI actually
>     >>>> supports DT_RELR, otherwise if the ABI start to support old glibcs might
>     >>>> wrongly assumes DT_RELR and fails with undefined behavior at runtime
>     >>>> (which this patch essentially tries to avoid).
>     >>>
>     >>> The DT_RELR set enables DT_RELR in glibc for all targets.
>     >> If I understood correctly, if we add GLIBC_ABI_DT_RELR on 2.36 but only enable
>     >> it on some architecture on 2.37 and try to run a binary with DT_RELR enabled
>     >> on glibc 2.36, _dl_check_map_versions won't accuse a failure and it will
>     >> eventually only fail at runtime (since relocation won't be applied).  My
>     >> understanding we are trying to avoid such failures.
>     >
>     > My patch set enables DT_RELR and GLIBC_ABI_DT_RELR on glibc 2.36
>     > for all, regardless of binutils version.   There is no per-arch
>     > behavior.    Only
>     > DT_RELR run-time tests are enabled for linkers with -z pack-relative-relocs.
> 
>     But it still does not fail when the architecture does not actually implement
>     RELR even when static linker does:
> 
>     $ clang --target=aarch64-linux-gnu -fuse-ld=lld -Wl,-z,pack-relative-relocs -fpie -o test -pie test.c
>     $ aarch64-glibc-linux-gnu-readelf -S test | grep relr\.dyn
>       [ 9] .relr.dyn         RELR             0000000000000528  00000528
>     $ ./elf/ld.so --library-path . ./test
>     $
> 
>     I would expect that even when binary adds GLIBC_ABI_DT_RELR, if dynamic linker
>     does not have DT_RELR it would fail early.
>
Joseph Myers March 30, 2022, 7:22 p.m. UTC | #8 
On Wed, 30 Mar 2022, Adhemerval Zanella via Libc-alpha wrote:

> But it still does not fail when the architecture does not actually implement
> RELR even when static linker does:

The dynamic linker is supposed to support executing binaries using RELR 
relocations on all architectures - there's not meant to be anything 
architecture-specific about that support.  What's architecture-specific is 
the static linker support (thus, if missing in the static linker used to 
build and test glibc, then (a) the dynamic linker could still run binaries 
using RELR if they were built using some other static linker, but (b) none 
of the glibc binaries, either installed or test, will contain any RELR 
relocations themselves).
Adhemerval Zanella Netto March 30, 2022, 8:38 p.m. UTC | #9 
On 30/03/2022 16:22, Joseph Myers wrote:
> On Wed, 30 Mar 2022, Adhemerval Zanella via Libc-alpha wrote:
> 
>> But it still does not fail when the architecture does not actually implement
>> RELR even when static linker does:
> 
> The dynamic linker is supposed to support executing binaries using RELR 
> relocations on all architectures - there's not meant to be anything 
> architecture-specific about that support.  What's architecture-specific is 
> the static linker support (thus, if missing in the static linker used to 
> build and test glibc, then (a) the dynamic linker could still run binaries 
> using RELR if they were built using some other static linker, but (b) none 
> of the glibc binaries, either installed or test, will contain any RELR 
> relocations themselves).
> 

Indeed, I was confused by the expected support.  This part seems ok H.J.
Fangrui Song March 30, 2022, 9:32 p.m. UTC | #10 
On 2022-03-30, Adhemerval Zanella via Libc-alpha wrote:
>
>
>On 30/03/2022 16:22, Joseph Myers wrote:
>> On Wed, 30 Mar 2022, Adhemerval Zanella via Libc-alpha wrote:
>>
>>> But it still does not fail when the architecture does not actually implement
>>> RELR even when static linker does:
>>
>> The dynamic linker is supposed to support executing binaries using RELR
>> relocations on all architectures - there's not meant to be anything
>> architecture-specific about that support.  What's architecture-specific is
>> the static linker support (thus, if missing in the static linker used to
>> build and test glibc, then (a) the dynamic linker could still run binaries
>> using RELR if they were built using some other static linker, but (b) none
>> of the glibc binaries, either installed or test, will contain any RELR
>> relocations themselves).
>>
>
>Indeed, I was confused by the expected support.  This part seems ok H.J.

Agree. Having the generic support helps GNU ld developers to contribute
RELR for non-x86. 
When I first seen "Add --disable-default-dt-relr" I was thinking whether
it is necessary to add this so early, but then I realized that this
essentially turns the whole glibc testsuite into tests
for GNU ld's RELR support. binutils itself has very few run-time tests
and bugs tend to be difficult to surface.
diff mbox series

Patch

diff --git a/elf/Makefile b/elf/Makefile
index fd462ba315..d3118b9777 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1113,8 +1113,10 @@  $(eval $(call include_dsosort_tests,dso-sort-tests-1.def))
 $(eval $(call include_dsosort_tests,dso-sort-tests-2.def))
 endif
 
-check-abi: $(objpfx)check-abi-ld.out
-tests-special += $(objpfx)check-abi-ld.out
+check-abi: $(objpfx)check-abi-ld.out \
+	   $(objpfx)check-abi-version-libc.out
+tests-special += $(objpfx)check-abi-ld.out \
+	   $(objpfx)check-abi-version-libc.out
 update-abi: update-abi-ld
 update-all-abi: update-all-abi-ld
 
@@ -2739,3 +2741,9 @@  $(objpfx)check-tst-relr-pie.out: $(objpfx)tst-relr-pie
 		| sed -ne '/required from libc.so/,$$ p' \
 		| grep GLIBC_ABI_DT_RELR > $@; \
 	$(evaluate-test)
+
+$(objpfx)check-abi-version-libc.out: $(common-objpfx)libc.so
+	LC_ALL=C $(READELF) -V -W $< \
+		| sed -ne '/.gnu.version_d/, /.gnu.version_r/ p' \
+		| grep GLIBC_ABI_DT_RELR > $@; \
+	$(evaluate-test)
diff --git a/elf/Versions b/elf/Versions
index 8bed855d8c..a9ff278de7 100644
--- a/elf/Versions
+++ b/elf/Versions
@@ -23,6 +23,11 @@  libc {
   GLIBC_2.35 {
     _dl_find_object;
   }
+  GLIBC_ABI_DT_RELR {
+    # This symbol is used only for empty version map and will be removed
+    # by scripts/versions.awk.
+    __placeholder_only_for_empty_version_map;
+  }
   GLIBC_PRIVATE {
     # functions used in other libraries
     __libc_early_init;
diff --git a/elf/dl-version.c b/elf/dl-version.c
index b47bd91727..720ec596a5 100644
--- a/elf/dl-version.c
+++ b/elf/dl-version.c
@@ -214,12 +214,20 @@  _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
 	      while (1)
 		{
 		  /* Match the symbol.  */
+		  const char *string = strtab + aux->vna_name;
 		  result |= match_symbol (DSO_FILENAME (map->l_name),
 					  map->l_ns, aux->vna_hash,
-					  strtab + aux->vna_name,
-					  needed->l_real, verbose,
+					  string, needed->l_real, verbose,
 					  aux->vna_flags & VER_FLG_WEAK);
 
+		  if (map->l_abi_version == lav_none
+		      /* 0xfd0e42: _dl_elf_hash ("GLIBC_ABI_DT_RELR").  */
+		      && aux->vna_hash == 0xfd0e42
+		      && __glibc_likely (strcmp (string,
+						 "GLIBC_ABI_DT_RELR")
+					 == 0))
+		    map->l_abi_version = lav_dt_relr_ref;
+
 		  /* Compare the version index.  */
 		  if ((unsigned int) (aux->vna_other & 0x7fff) > ndx_high)
 		    ndx_high = aux->vna_other & 0x7fff;
@@ -253,6 +261,16 @@  _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
       ent = (ElfW(Verdef) *) (map->l_addr + def->d_un.d_ptr);
       while (1)
 	{
+	  /* 0x0963cf85: _dl_elf_hash ("GLIBC_PRIVATE").  */
+	  if (ent->vd_hash == 0x0963cf85)
+	    {
+	      ElfW(Verdaux) *aux = (ElfW(Verdaux) *) ((char *) ent
+						      + ent->vd_aux);
+	      if (__glibc_likely (strcmp ("GLIBC_PRIVATE",
+					  strtab + aux->vda_name) == 0))
+		map->l_abi_version = lav_private_def;
+	    }
+
 	  if ((unsigned int) (ent->vd_ndx & 0x7fff) > ndx_high)
 	    ndx_high = ent->vd_ndx & 0x7fff;
 
@@ -352,6 +370,17 @@  _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
 	}
     }
 
+  /* Issue an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR
+     dependency nor GLIBC_PRIVATE definition.  */
+  if (map->l_info[DT_RELR] != NULL
+      && __glibc_unlikely (map->l_abi_version == lav_none))
+    {
+      _dl_exception_create
+	(&exception, DSO_FILENAME (map->l_name),
+	 N_("DT_RELR without GLIBC_ABI_DT_RELR dependency"));
+      goto call_error;
+    }
+
   return result;
 }
 
diff --git a/include/link.h b/include/link.h
index 03db14c7b0..8ec5e35cf2 100644
--- a/include/link.h
+++ b/include/link.h
@@ -177,6 +177,12 @@  struct link_map
 	lt_library,		/* Library needed by main executable.  */
 	lt_loaded		/* Extra run-time loaded shared object.  */
       } l_type:2;
+    enum			/* ABI dependency of this object.  */
+      {
+	lav_none,		/* No ABI dependency.  */
+	lav_dt_relr_ref,	/* Need GLIBC_ABI_DT_RELR.  */
+	lav_private_def		/* Define GLIBC_PRIVATE.  */
+      } l_abi_version:2;
     unsigned int l_relocated:1;	/* Nonzero if object's relocations done.  */
     unsigned int l_init_called:1; /* Nonzero if DT_INIT function called.  */
     unsigned int l_global:1;	/* Nonzero if object in _dl_global_scope.  */
diff --git a/scripts/abilist.awk b/scripts/abilist.awk
index 24a34ccbed..6cc7af6ac8 100644
--- a/scripts/abilist.awk
+++ b/scripts/abilist.awk
@@ -55,6 +55,8 @@  $2 == "g" || $2 == "w" && (NF == 7 || NF == 8) {
   # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.
   if (NF > 7 && $7 == ".hidden") next;
 
+  if (version ~ /^GLIBC_ABI_/ && !include_abi_version) next;
+
   if (version == "GLIBC_PRIVATE" && !include_private) next;
 
   desc = "";
diff --git a/scripts/versions.awk b/scripts/versions.awk
index 357ad1355e..d70b07bd1a 100644
--- a/scripts/versions.awk
+++ b/scripts/versions.awk
@@ -185,8 +185,13 @@  END {
 	closeversion(oldver, veryoldver);
 	veryoldver = oldver;
       }
-      printf("%s {\n  global:\n", $2) > outfile;
       oldver = $2;
+      # Skip the placeholder symbol used only for empty version map.
+      if ($3 == "__placeholder_only_for_empty_version_map;") {
+	printf("%s {\n", $2) > outfile;
+	continue;
+      }
+      printf("%s {\n  global:\n", $2) > outfile;
     }
     printf("   ") > outfile;
     for (n = 3; n <= NF; ++n) {