diff mbox series

[v4,08/10] csu: Move static pie self relocation later [BZ #27072]

Message ID 4224b7c0428492696fe6d6c01739adcf69fc677d.1610986541.git.szabolcs.nagy@arm.com
State Superseded
Headers show
Series fix ifunc with static pie [BZ #27072] | expand

Commit Message

Szabolcs Nagy Jan. 18, 2021, 4:25 p.m. UTC
IFUNC resolvers may depend on tunables and cpu feature setup so
move static pie self relocation after those.

It is hard to guarantee that the ealy startup code does not rely
on relocations so this is a bit fragile. It would be more robust
to handle RELATIVE relocs early and only IRELATIVE relocs later,
but the current relocation processing code cannot do that.

The early startup code before relocation processing includes

  _dl_aux_init (auxvec);
  __libc_init_secure ();
  __tunables_init (__environ);
  ARCH_INIT_CPU_FEATURES ();

These are simple enough that RELATIVE relocs can be avoided.

__ehdr_start may require RELATIVE relocation so it was moved
later, fortunately ehdr and phdr are not used in the early code.

Fixes bug 27072.
---
 csu/libc-start.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Comments

Adhemerval Zanella Jan. 19, 2021, 2:07 p.m. UTC | #1
On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
> IFUNC resolvers may depend on tunables and cpu feature setup so
> move static pie self relocation after those.
> 
> It is hard to guarantee that the ealy startup code does not rely
> on relocations so this is a bit fragile. It would be more robust
> to handle RELATIVE relocs early and only IRELATIVE relocs later,
> but the current relocation processing code cannot do that.
> 
> The early startup code before relocation processing includes
> 
>   _dl_aux_init (auxvec);
>   __libc_init_secure ();
>   __tunables_init (__environ);
>   ARCH_INIT_CPU_FEATURES ();
> 
> These are simple enough that RELATIVE relocs can be avoided.
> 
> __ehdr_start may require RELATIVE relocation so it was moved
> later, fortunately ehdr and phdr are not used in the early code.
> 
> Fixes bug 27072.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  csu/libc-start.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 1e90dcb0a7..c2b59431a3 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -146,8 +146,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    int result;
>  
>  #ifndef SHARED
> -  _dl_relocate_static_pie ();
> -
>    char **ev = &argv[argc + 1];
>  
>    __environ = ev;
Ok.


> @@ -169,24 +167,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    }
>  #  endif
>    _dl_aux_init (auxvec);
> -  if (GL(dl_phdr) == NULL)
>  # endif
> -    {
> -      /* Starting from binutils-2.23, the linker will define the
> -         magic symbol __ehdr_start to point to our own ELF header
> -         if it is visible in a segment that also includes the phdrs.
> -         So we can set up _dl_phdr and _dl_phnum even without any
> -         information from auxv.  */
> -
> -      extern const ElfW(Ehdr) __ehdr_start
> -	__attribute__ ((weak, visibility ("hidden")));
> -      if (&__ehdr_start != NULL)
> -        {
> -          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
> -          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
> -          GL(dl_phnum) = __ehdr_start.e_phnum;
> -        }
> -    }
>  
>    /* Initialize very early so that tunables can use it.  */
>    __libc_init_secure ();

Ok.

> @@ -195,6 +176,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  
>    ARCH_INIT_CPU_FEATURES ();
>  
> +  /* Do static pie self relocation after tunables and cpu features
> +     are setup for ifunc resolvers. Before this point relocations
> +     must be avoided.  */
> +  _dl_relocate_static_pie ();
> +
>    /* Perform IREL{,A} relocations.  */
>    ARCH_SETUP_IREL ();
>  

Ok.

> @@ -206,6 +192,26 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>       hwcap and platform fields available in the TCB.  */
>    ARCH_APPLY_IREL ();
>  
> +# ifdef HAVE_AUX_VECTOR
> +  if (GL(dl_phdr) == NULL)
> +# endif
> +    {
> +      /* Starting from binutils-2.23, the linker will define the
> +         magic symbol __ehdr_start to point to our own ELF header
> +         if it is visible in a segment that also includes the phdrs.
> +         So we can set up _dl_phdr and _dl_phnum even without any
> +         information from auxv.  */
> +
> +      extern const ElfW(Ehdr) __ehdr_start
> +	__attribute__ ((weak, visibility ("hidden")));
> +      if (&__ehdr_start != NULL)
> +        {
> +          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
> +          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
> +          GL(dl_phnum) = __ehdr_start.e_phnum;
> +        }
> +    }
> +
>    /* Set up the stack checker's canary.  */
>    uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
>  # ifdef THREAD_SET_STACK_GUARD
> 

Ok.
Szabolcs Nagy Jan. 19, 2021, 2:35 p.m. UTC | #2
The 01/19/2021 11:07, Adhemerval Zanella wrote:
> On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
> > IFUNC resolvers may depend on tunables and cpu feature setup so
> > move static pie self relocation after those.
> > 
> > It is hard to guarantee that the ealy startup code does not rely
> > on relocations so this is a bit fragile. It would be more robust
> > to handle RELATIVE relocs early and only IRELATIVE relocs later,
> > but the current relocation processing code cannot do that.
> > 
> > The early startup code before relocation processing includes
> > 
> >   _dl_aux_init (auxvec);
> >   __libc_init_secure ();
> >   __tunables_init (__environ);
> >   ARCH_INIT_CPU_FEATURES ();
> > 
> > These are simple enough that RELATIVE relocs can be avoided.
> > 
> > __ehdr_start may require RELATIVE relocation so it was moved
> > later, fortunately ehdr and phdr are not used in the early code.
> > 
> > Fixes bug 27072.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


sigh, this is an old version of this patch, i made a
mistake putting the series together.

the problem is that _dl_phdr is used in ARCH_SETUP_TLS
(to get the tls program headers) so the __ehdr_start
magic should be before that (this only matters if auxv
lacks AT_PHDR for some reason, which should not happen
normally on linux, so testing won't show the problem)

i'm trying to figure out where i lost the new version..

thanks for the reviews.

> 
> > ---
> >  csu/libc-start.c | 44 +++++++++++++++++++++++++-------------------
> >  1 file changed, 25 insertions(+), 19 deletions(-)
> > 
> > diff --git a/csu/libc-start.c b/csu/libc-start.c
> > index 1e90dcb0a7..c2b59431a3 100644
> > --- a/csu/libc-start.c
> > +++ b/csu/libc-start.c
> > @@ -146,8 +146,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> >    int result;
> >  
> >  #ifndef SHARED
> > -  _dl_relocate_static_pie ();
> > -
> >    char **ev = &argv[argc + 1];
> >  
> >    __environ = ev;
> Ok.
> 
> 
> > @@ -169,24 +167,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> >    }
> >  #  endif
> >    _dl_aux_init (auxvec);
> > -  if (GL(dl_phdr) == NULL)
> >  # endif
> > -    {
> > -      /* Starting from binutils-2.23, the linker will define the
> > -         magic symbol __ehdr_start to point to our own ELF header
> > -         if it is visible in a segment that also includes the phdrs.
> > -         So we can set up _dl_phdr and _dl_phnum even without any
> > -         information from auxv.  */
> > -
> > -      extern const ElfW(Ehdr) __ehdr_start
> > -	__attribute__ ((weak, visibility ("hidden")));
> > -      if (&__ehdr_start != NULL)
> > -        {
> > -          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
> > -          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
> > -          GL(dl_phnum) = __ehdr_start.e_phnum;
> > -        }
> > -    }
> >  
> >    /* Initialize very early so that tunables can use it.  */
> >    __libc_init_secure ();
> 
> Ok.
> 
> > @@ -195,6 +176,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> >  
> >    ARCH_INIT_CPU_FEATURES ();
> >  
> > +  /* Do static pie self relocation after tunables and cpu features
> > +     are setup for ifunc resolvers. Before this point relocations
> > +     must be avoided.  */
> > +  _dl_relocate_static_pie ();
> > +
> >    /* Perform IREL{,A} relocations.  */
> >    ARCH_SETUP_IREL ();
> >  
> 
> Ok.
> 
> > @@ -206,6 +192,26 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> >       hwcap and platform fields available in the TCB.  */
> >    ARCH_APPLY_IREL ();
> >  
> > +# ifdef HAVE_AUX_VECTOR
> > +  if (GL(dl_phdr) == NULL)
> > +# endif
> > +    {
> > +      /* Starting from binutils-2.23, the linker will define the
> > +         magic symbol __ehdr_start to point to our own ELF header
> > +         if it is visible in a segment that also includes the phdrs.
> > +         So we can set up _dl_phdr and _dl_phnum even without any
> > +         information from auxv.  */
> > +
> > +      extern const ElfW(Ehdr) __ehdr_start
> > +	__attribute__ ((weak, visibility ("hidden")));
> > +      if (&__ehdr_start != NULL)
> > +        {
> > +          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
> > +          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
> > +          GL(dl_phnum) = __ehdr_start.e_phnum;
> > +        }
> > +    }
> > +
> >    /* Set up the stack checker's canary.  */
> >    uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
> >  # ifdef THREAD_SET_STACK_GUARD
> > 
> 
> Ok.
Adhemerval Zanella Jan. 19, 2021, 2:36 p.m. UTC | #3
On 19/01/2021 11:35, Szabolcs Nagy wrote:
> The 01/19/2021 11:07, Adhemerval Zanella wrote:
>> On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
>>> IFUNC resolvers may depend on tunables and cpu feature setup so
>>> move static pie self relocation after those.
>>>
>>> It is hard to guarantee that the ealy startup code does not rely
>>> on relocations so this is a bit fragile. It would be more robust
>>> to handle RELATIVE relocs early and only IRELATIVE relocs later,
>>> but the current relocation processing code cannot do that.
>>>
>>> The early startup code before relocation processing includes
>>>
>>>   _dl_aux_init (auxvec);
>>>   __libc_init_secure ();
>>>   __tunables_init (__environ);
>>>   ARCH_INIT_CPU_FEATURES ();
>>>
>>> These are simple enough that RELATIVE relocs can be avoided.
>>>
>>> __ehdr_start may require RELATIVE relocation so it was moved
>>> later, fortunately ehdr and phdr are not used in the early code.
>>>
>>> Fixes bug 27072.
>>
>> LGTM, thanks.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> 
> sigh, this is an old version of this patch, i made a
> mistake putting the series together.
> 
> the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> (to get the tls program headers) so the __ehdr_start
> magic should be before that (this only matters if auxv
> lacks AT_PHDR for some reason, which should not happen
> normally on linux, so testing won't show the problem)

By normally do you mean it might happen on a specific kernel version
or is it architecture specific?
H.J. Lu Jan. 19, 2021, 2:48 p.m. UTC | #4
On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > The 01/19/2021 11:07, Adhemerval Zanella wrote:
> >> On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
> >>> IFUNC resolvers may depend on tunables and cpu feature setup so
> >>> move static pie self relocation after those.
> >>>
> >>> It is hard to guarantee that the ealy startup code does not rely
> >>> on relocations so this is a bit fragile. It would be more robust
> >>> to handle RELATIVE relocs early and only IRELATIVE relocs later,
> >>> but the current relocation processing code cannot do that.
> >>>
> >>> The early startup code before relocation processing includes
> >>>
> >>>   _dl_aux_init (auxvec);
> >>>   __libc_init_secure ();
> >>>   __tunables_init (__environ);
> >>>   ARCH_INIT_CPU_FEATURES ();
> >>>
> >>> These are simple enough that RELATIVE relocs can be avoided.
> >>>
> >>> __ehdr_start may require RELATIVE relocation so it was moved
> >>> later, fortunately ehdr and phdr are not used in the early code.
> >>>
> >>> Fixes bug 27072.
> >>
> >> LGTM, thanks.
> >>
> >> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> >
> >
> > sigh, this is an old version of this patch, i made a
> > mistake putting the series together.
> >
> > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > (to get the tls program headers) so the __ehdr_start
> > magic should be before that (this only matters if auxv
> > lacks AT_PHDR for some reason, which should not happen
> > normally on linux, so testing won't show the problem)
>
> By normally do you mean it might happen on a specific kernel version
> or is it architecture specific?

I think we can leave __ehdr_start ASIS since it doesn't need RELATIVE
relocation.  I verified it by adding -Wl,-z,report-relative-reloc when building
elf/sln on x86.
Szabolcs Nagy Jan. 19, 2021, 3:24 p.m. UTC | #5
The 01/19/2021 06:48, H.J. Lu wrote:
> On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> >
> >
> > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > The 01/19/2021 11:07, Adhemerval Zanella wrote:
> > >> On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
> > >>> IFUNC resolvers may depend on tunables and cpu feature setup so
> > >>> move static pie self relocation after those.
> > >>>
> > >>> It is hard to guarantee that the ealy startup code does not rely
> > >>> on relocations so this is a bit fragile. It would be more robust
> > >>> to handle RELATIVE relocs early and only IRELATIVE relocs later,
> > >>> but the current relocation processing code cannot do that.
> > >>>
> > >>> The early startup code before relocation processing includes
> > >>>
> > >>>   _dl_aux_init (auxvec);
> > >>>   __libc_init_secure ();
> > >>>   __tunables_init (__environ);
> > >>>   ARCH_INIT_CPU_FEATURES ();
> > >>>
> > >>> These are simple enough that RELATIVE relocs can be avoided.
> > >>>
> > >>> __ehdr_start may require RELATIVE relocation so it was moved
> > >>> later, fortunately ehdr and phdr are not used in the early code.
> > >>>
> > >>> Fixes bug 27072.
> > >>
> > >> LGTM, thanks.
> > >>
> > >> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> > >
> > >
> > > sigh, this is an old version of this patch, i made a
> > > mistake putting the series together.
> > >
> > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > (to get the tls program headers) so the __ehdr_start
> > > magic should be before that (this only matters if auxv
> > > lacks AT_PHDR for some reason, which should not happen
> > > normally on linux, so testing won't show the problem)
> >
> > By normally do you mean it might happen on a specific kernel version
> > or is it architecture specific?

i guess __ehdr_start symbol can be useful and with it
glibc does not have to depend on auxv (which an elf
loader like valgrind/qemu-user may get wrong)

however it is only used as a fallback and on linux
AT_PHDR is always expected to be present. (i don't
know if this ever triggers)

> 
> I think we can leave __ehdr_start ASIS since it doesn't need RELATIVE
> relocation.  I verified it by adding -Wl,-z,report-relative-reloc when building
> elf/sln on x86.

it needs relative reloc on aarch64: it can be an undefined weak
symbol and that must be 0. a pc relative address computation
cannot give 0 (unless linker does some instruction rewriting,
but on aarch64 the address computation is multiple instructions
that can be spread far apart). so yeah it needs a GOT entry and
that will be either 0 or needs a RELATIVE reloc.
H.J. Lu Jan. 19, 2021, 3:32 p.m. UTC | #6
On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 01/19/2021 06:48, H.J. Lu wrote:
> > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> > >
> > >
> > >
> > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > The 01/19/2021 11:07, Adhemerval Zanella wrote:
> > > >> On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
> > > >>> IFUNC resolvers may depend on tunables and cpu feature setup so
> > > >>> move static pie self relocation after those.
> > > >>>
> > > >>> It is hard to guarantee that the ealy startup code does not rely
> > > >>> on relocations so this is a bit fragile. It would be more robust
> > > >>> to handle RELATIVE relocs early and only IRELATIVE relocs later,
> > > >>> but the current relocation processing code cannot do that.
> > > >>>
> > > >>> The early startup code before relocation processing includes
> > > >>>
> > > >>>   _dl_aux_init (auxvec);
> > > >>>   __libc_init_secure ();
> > > >>>   __tunables_init (__environ);
> > > >>>   ARCH_INIT_CPU_FEATURES ();
> > > >>>
> > > >>> These are simple enough that RELATIVE relocs can be avoided.
> > > >>>
> > > >>> __ehdr_start may require RELATIVE relocation so it was moved
> > > >>> later, fortunately ehdr and phdr are not used in the early code.
> > > >>>
> > > >>> Fixes bug 27072.
> > > >>
> > > >> LGTM, thanks.
> > > >>
> > > >> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> > > >
> > > >
> > > > sigh, this is an old version of this patch, i made a
> > > > mistake putting the series together.
> > > >
> > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > (to get the tls program headers) so the __ehdr_start
> > > > magic should be before that (this only matters if auxv
> > > > lacks AT_PHDR for some reason, which should not happen
> > > > normally on linux, so testing won't show the problem)
> > >
> > > By normally do you mean it might happen on a specific kernel version
> > > or is it architecture specific?
>
> i guess __ehdr_start symbol can be useful and with it
> glibc does not have to depend on auxv (which an elf
> loader like valgrind/qemu-user may get wrong)
>
> however it is only used as a fallback and on linux
> AT_PHDR is always expected to be present. (i don't
> know if this ever triggers)

Only used on Hurd?

> >
> > I think we can leave __ehdr_start ASIS since it doesn't need RELATIVE
> > relocation.  I verified it by adding -Wl,-z,report-relative-reloc when building
> > elf/sln on x86.
>
> it needs relative reloc on aarch64: it can be an undefined weak
> symbol and that must be 0. a pc relative address computation
> cannot give 0 (unless linker does some instruction rewriting,
> but on aarch64 the address computation is multiple instructions
> that can be spread far apart). so yeah it needs a GOT entry and
> that will be either 0 or needs a RELATIVE reloc.

On x86, I converted load from GOT to simple LEA without RELATIVE
in this case.   But this is an x86 specific optimization.
H.J. Lu Jan. 19, 2021, 4:47 p.m. UTC | #7
On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 01/19/2021 06:48, H.J. Lu wrote:
> > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > <libc-alpha@sourceware.org> wrote:
> > > >
> > > >
> > > >
> > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > The 01/19/2021 11:07, Adhemerval Zanella wrote:
> > > > >> On 18/01/2021 13:25, Szabolcs Nagy via Libc-alpha wrote:
> > > > >>> IFUNC resolvers may depend on tunables and cpu feature setup so
> > > > >>> move static pie self relocation after those.
> > > > >>>
> > > > >>> It is hard to guarantee that the ealy startup code does not rely
> > > > >>> on relocations so this is a bit fragile. It would be more robust
> > > > >>> to handle RELATIVE relocs early and only IRELATIVE relocs later,
> > > > >>> but the current relocation processing code cannot do that.
> > > > >>>
> > > > >>> The early startup code before relocation processing includes
> > > > >>>
> > > > >>>   _dl_aux_init (auxvec);
> > > > >>>   __libc_init_secure ();
> > > > >>>   __tunables_init (__environ);
> > > > >>>   ARCH_INIT_CPU_FEATURES ();
> > > > >>>
> > > > >>> These are simple enough that RELATIVE relocs can be avoided.
> > > > >>>
> > > > >>> __ehdr_start may require RELATIVE relocation so it was moved
> > > > >>> later, fortunately ehdr and phdr are not used in the early code.
> > > > >>>
> > > > >>> Fixes bug 27072.
> > > > >>
> > > > >> LGTM, thanks.
> > > > >>
> > > > >> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> > > > >
> > > > >
> > > > > sigh, this is an old version of this patch, i made a
> > > > > mistake putting the series together.
> > > > >
> > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > (to get the tls program headers) so the __ehdr_start
> > > > > magic should be before that (this only matters if auxv
> > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > normally on linux, so testing won't show the problem)
> > > >
> > > > By normally do you mean it might happen on a specific kernel version
> > > > or is it architecture specific?
> >
> > i guess __ehdr_start symbol can be useful and with it
> > glibc does not have to depend on auxv (which an elf
> > loader like valgrind/qemu-user may get wrong)
> >
> > however it is only used as a fallback and on linux
> > AT_PHDR is always expected to be present. (i don't
> > know if this ever triggers)
>
> Only used on Hurd?

Does arm64 linker always define __ehdr_start?  If yes, can you drop
"weak," to see if RELATIVE goes away?

> > >
> > > I think we can leave __ehdr_start ASIS since it doesn't need RELATIVE
> > > relocation.  I verified it by adding -Wl,-z,report-relative-reloc when building
> > > elf/sln on x86.
> >
> > it needs relative reloc on aarch64: it can be an undefined weak
> > symbol and that must be 0. a pc relative address computation
> > cannot give 0 (unless linker does some instruction rewriting,
> > but on aarch64 the address computation is multiple instructions
> > that can be spread far apart). so yeah it needs a GOT entry and
> > that will be either 0 or needs a RELATIVE reloc.
>
> On x86, I converted load from GOT to simple LEA without RELATIVE
> in this case.   But this is an x86 specific optimization.
>
> --
> H.J.
Szabolcs Nagy Jan. 19, 2021, 5:03 p.m. UTC | #8
The 01/19/2021 08:47, H.J. Lu wrote:
> On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > <libc-alpha@sourceware.org> wrote:
> > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > magic should be before that (this only matters if auxv
> > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > normally on linux, so testing won't show the problem)
> > > > >
> > > > > By normally do you mean it might happen on a specific kernel version
> > > > > or is it architecture specific?
> > >
> > > i guess __ehdr_start symbol can be useful and with it
> > > glibc does not have to depend on auxv (which an elf
> > > loader like valgrind/qemu-user may get wrong)
> > >
> > > however it is only used as a fallback and on linux
> > > AT_PHDR is always expected to be present. (i don't
> > > know if this ever triggers)
> >
> > Only used on Hurd?
> 
> Does arm64 linker always define __ehdr_start?  If yes, can you drop
> "weak," to see if RELATIVE goes away?

__ehdr_start support was added in binutils 2.23
so i guess all supported binutils has it which
means we can make it non-weak indeed.

good idea.

(we can also ignore auxv and rely on __ehdr_start only,
but for now just making it non-weak should be fine.)
H.J. Lu Jan. 19, 2021, 5:10 p.m. UTC | #9
On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 01/19/2021 08:47, H.J. Lu wrote:
> > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > magic should be before that (this only matters if auxv
> > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > normally on linux, so testing won't show the problem)
> > > > > >
> > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > or is it architecture specific?
> > > >
> > > > i guess __ehdr_start symbol can be useful and with it
> > > > glibc does not have to depend on auxv (which an elf
> > > > loader like valgrind/qemu-user may get wrong)
> > > >
> > > > however it is only used as a fallback and on linux
> > > > AT_PHDR is always expected to be present. (i don't
> > > > know if this ever triggers)
> > >
> > > Only used on Hurd?
> >
> > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > "weak," to see if RELATIVE goes away?
>
> __ehdr_start support was added in binutils 2.23

We may assume binutils >= 2.33 when building for static PIE
since all static PIE linkers should define __ehdr_start.

Does lld define __ehdr_start?

> so i guess all supported binutils has it which
> means we can make it non-weak indeed.
>
> good idea.
>
> (we can also ignore auxv and rely on __ehdr_start only,
> but for now just making it non-weak should be fine.)
>
Fangrui Song Jan. 19, 2021, 5:25 p.m. UTC | #10
On Tue, Jan 19, 2021 at 9:11 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 01/19/2021 08:47, H.J. Lu wrote:
> > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > >
> > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > or is it architecture specific?
> > > > >
> > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > glibc does not have to depend on auxv (which an elf
> > > > > loader like valgrind/qemu-user may get wrong)
> > > > >
> > > > > however it is only used as a fallback and on linux
> > > > > AT_PHDR is always expected to be present. (i don't
> > > > > know if this ever triggers)
> > > >
> > > > Only used on Hurd?
> > >
> > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > "weak," to see if RELATIVE goes away?
> >
> > __ehdr_start support was added in binutils 2.23
>
> We may assume binutils >= 2.33 when building for static PIE
> since all static PIE linkers should define __ehdr_start.
>
> Does lld define __ehdr_start?

LLD defines __ehdr_start as hidden if it is not a defined symbol in
-no-pie/-pie/-shared modes.
The behavior seems to match gold. GNU ld seems to use STB_LOCAL
STV_DEFAULT but the exterior behavior should be the same.

> > so i guess all supported binutils has it which
> > means we can make it non-weak indeed.
> >
> > good idea.
> >
> > (we can also ignore auxv and rely on __ehdr_start only,
> > but for now just making it non-weak should be fine.)
> >
>
>
> --
> H.J.
H.J. Lu Jan. 19, 2021, 5:33 p.m. UTC | #11
On Tue, Jan 19, 2021 at 9:26 AM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> On Tue, Jan 19, 2021 at 9:11 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > >
> > > The 01/19/2021 08:47, H.J. Lu wrote:
> > > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > > >
> > > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > > or is it architecture specific?
> > > > > >
> > > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > > glibc does not have to depend on auxv (which an elf
> > > > > > loader like valgrind/qemu-user may get wrong)
> > > > > >
> > > > > > however it is only used as a fallback and on linux
> > > > > > AT_PHDR is always expected to be present. (i don't
> > > > > > know if this ever triggers)
> > > > >
> > > > > Only used on Hurd?
> > > >
> > > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > > "weak," to see if RELATIVE goes away?
> > >
> > > __ehdr_start support was added in binutils 2.23
> >
> > We may assume binutils >= 2.33 when building for static PIE
> > since all static PIE linkers should define __ehdr_start.
> >
> > Does lld define __ehdr_start?
>
> LLD defines __ehdr_start as hidden if it is not a defined symbol in
> -no-pie/-pie/-shared modes.
> The behavior seems to match gold. GNU ld seems to use STB_LOCAL
> STV_DEFAULT but the exterior behavior should be the same.

I am not sure where you got such incorrect info.  Both ld and gold were changed
to STV_HIDDEN in 2013 by the same commit:

ommit cde7cb0129a884a060b99c7c83e8f5c9af728b0a
Author: Maciej W. Rozycki <macro@linux-mips.org>
Date:   Fri May 3 15:19:27 2013 +0000

            gold/
            PR ld/15365
            * layout.cc (Layout::finalize): Make __ehdr_start STV_HIDDEN.

            ld/
            PR ld/15365
            * emultempl/elf32.em (gld${EMULATION_NAME}_before_allocation):
            Restrict __ehdr_start's export class to no less than STV_HIDDEN.
Szabolcs Nagy Jan. 19, 2021, 5:38 p.m. UTC | #12
The 01/19/2021 09:10, H.J. Lu wrote:
> On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 01/19/2021 08:47, H.J. Lu wrote:
> > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > >
> > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > or is it architecture specific?
> > > > >
> > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > glibc does not have to depend on auxv (which an elf
> > > > > loader like valgrind/qemu-user may get wrong)
> > > > >
> > > > > however it is only used as a fallback and on linux
> > > > > AT_PHDR is always expected to be present. (i don't
> > > > > know if this ever triggers)
> > > >
> > > > Only used on Hurd?
> > >
> > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > "weak," to see if RELATIVE goes away?
> >
> > __ehdr_start support was added in binutils 2.23
> 
> We may assume binutils >= 2.33 when building for static PIE
> since all static PIE linkers should define __ehdr_start.

this piece of code is used for both static PIE and non-PIE,
but we already require binutils >= 2.25 for building glibc,
dropping weak should be fine.

i will resend the series with this.

> 
> Does lld define __ehdr_start?
> 
> > so i guess all supported binutils has it which
> > means we can make it non-weak indeed.
> >
> > good idea.
> >
> > (we can also ignore auxv and rely on __ehdr_start only,
> > but for now just making it non-weak should be fine.)
> >
> 
> 
> -- 
> H.J.
Fangrui Song Jan. 19, 2021, 5:38 p.m. UTC | #13
On Tue, Jan 19, 2021 at 9:34 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 9:26 AM Fāng-ruì Sòng <maskray@google.com> wrote:
> >
> > On Tue, Jan 19, 2021 at 9:11 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > >
> > > > The 01/19/2021 08:47, H.J. Lu wrote:
> > > > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > > > >
> > > > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > > > or is it architecture specific?
> > > > > > >
> > > > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > > > glibc does not have to depend on auxv (which an elf
> > > > > > > loader like valgrind/qemu-user may get wrong)
> > > > > > >
> > > > > > > however it is only used as a fallback and on linux
> > > > > > > AT_PHDR is always expected to be present. (i don't
> > > > > > > know if this ever triggers)
> > > > > >
> > > > > > Only used on Hurd?
> > > > >
> > > > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > > > "weak," to see if RELATIVE goes away?
> > > >
> > > > __ehdr_start support was added in binutils 2.23
> > >
> > > We may assume binutils >= 2.33 when building for static PIE
> > > since all static PIE linkers should define __ehdr_start.
> > >
> > > Does lld define __ehdr_start?
> >
> > LLD defines __ehdr_start as hidden if it is not a defined symbol in
> > -no-pie/-pie/-shared modes.
> > The behavior seems to match gold. GNU ld seems to use STB_LOCAL
> > STV_DEFAULT but the exterior behavior should be the same.
>
> I am not sure where you got such incorrect info.  Both ld and gold were changed
> to STV_HIDDEN in 2013 by the same commit:
>
> ommit cde7cb0129a884a060b99c7c83e8f5c9af728b0a
> Author: Maciej W. Rozycki <macro@linux-mips.org>
> Date:   Fri May 3 15:19:27 2013 +0000
>
>             gold/
>             PR ld/15365
>             * layout.cc (Layout::finalize): Make __ehdr_start STV_HIDDEN.
>
>             ld/
>             PR ld/15365
>             * emultempl/elf32.em (gld${EMULATION_NAME}_before_allocation):
>             Restrict __ehdr_start's export class to no less than STV_HIDDEN.
>
> --
> H.J.

% cat a.s
.global _start, __ehdr_start
_start:
  leaq __ehdr_start(%rip), %rax
% gcc -fuse-ld=bfd -nostdlib a.s -o a.so
% readelf -Ws a.so
...
     9: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT    1 __ehdr_start

When the binding is STB_LOCAL, the visibility does not really matter.

ELF spec: "A symbol with STB_LOCAL binding may not have STV_PROTECTED
visibility. A hidden symbol contained in a relocatable object must be
either removed or converted to STB_LOCAL binding by the link-editor
when the relocatable object is included in an executable file or
shared object."
H.J. Lu Jan. 19, 2021, 5:42 p.m. UTC | #14
On Tue, Jan 19, 2021 at 9:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 01/19/2021 09:10, H.J. Lu wrote:
> > On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > >
> > > The 01/19/2021 08:47, H.J. Lu wrote:
> > > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > > >
> > > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > > or is it architecture specific?
> > > > > >
> > > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > > glibc does not have to depend on auxv (which an elf
> > > > > > loader like valgrind/qemu-user may get wrong)
> > > > > >
> > > > > > however it is only used as a fallback and on linux
> > > > > > AT_PHDR is always expected to be present. (i don't
> > > > > > know if this ever triggers)
> > > > >
> > > > > Only used on Hurd?
> > > >
> > > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > > "weak," to see if RELATIVE goes away?
> > >
> > > __ehdr_start support was added in binutils 2.23
> >
> > We may assume binutils >= 2.33 when building for static PIE
> > since all static PIE linkers should define __ehdr_start.
>
> this piece of code is used for both static PIE and non-PIE,
> but we already require binutils >= 2.25 for building glibc,
> dropping weak should be fine.
>

It is safer to check BUILD_PIE_DEFAULT when dropping
weak.
Szabolcs Nagy Jan. 19, 2021, 5:47 p.m. UTC | #15
The 01/19/2021 09:42, H.J. Lu wrote:
> On Tue, Jan 19, 2021 at 9:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 01/19/2021 09:10, H.J. Lu wrote:
> > > On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > >
> > > > The 01/19/2021 08:47, H.J. Lu wrote:
> > > > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > > > >
> > > > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > > > or is it architecture specific?
> > > > > > >
> > > > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > > > glibc does not have to depend on auxv (which an elf
> > > > > > > loader like valgrind/qemu-user may get wrong)
> > > > > > >
> > > > > > > however it is only used as a fallback and on linux
> > > > > > > AT_PHDR is always expected to be present. (i don't
> > > > > > > know if this ever triggers)
> > > > > >
> > > > > > Only used on Hurd?
> > > > >
> > > > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > > > "weak," to see if RELATIVE goes away?
> > > >
> > > > __ehdr_start support was added in binutils 2.23
> > >
> > > We may assume binutils >= 2.33 when building for static PIE
> > > since all static PIE linkers should define __ehdr_start.
> >
> > this piece of code is used for both static PIE and non-PIE,
> > but we already require binutils >= 2.25 for building glibc,
> > dropping weak should be fine.
> >
> 
> It is safer to check BUILD_PIE_DEFAULT when dropping
> weak.

ok.

does static linking have weaker linker version requirement
than building glibc?
H.J. Lu Jan. 19, 2021, 5:53 p.m. UTC | #16
On Tue, Jan 19, 2021 at 9:47 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 01/19/2021 09:42, H.J. Lu wrote:
> > On Tue, Jan 19, 2021 at 9:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > >
> > > The 01/19/2021 09:10, H.J. Lu wrote:
> > > > On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > >
> > > > > The 01/19/2021 08:47, H.J. Lu wrote:
> > > > > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > > > > >
> > > > > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > > > > or is it architecture specific?
> > > > > > > >
> > > > > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > > > > glibc does not have to depend on auxv (which an elf
> > > > > > > > loader like valgrind/qemu-user may get wrong)
> > > > > > > >
> > > > > > > > however it is only used as a fallback and on linux
> > > > > > > > AT_PHDR is always expected to be present. (i don't
> > > > > > > > know if this ever triggers)
> > > > > > >
> > > > > > > Only used on Hurd?
> > > > > >
> > > > > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > > > > "weak," to see if RELATIVE goes away?
> > > > >
> > > > > __ehdr_start support was added in binutils 2.23
> > > >
> > > > We may assume binutils >= 2.33 when building for static PIE
> > > > since all static PIE linkers should define __ehdr_start.
> > >
> > > this piece of code is used for both static PIE and non-PIE,
> > > but we already require binutils >= 2.25 for building glibc,
> > > dropping weak should be fine.
> > >
> >
> > It is safer to check BUILD_PIE_DEFAULT when dropping
> > weak.
>
> ok.
>
> does static linking have weaker linker version requirement
> than building glibc?

Very unlikely.  But one may be forced to use the older linker
for some reason.
H.J. Lu Jan. 19, 2021, 5:59 p.m. UTC | #17
On Tue, Jan 19, 2021 at 9:53 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 9:47 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 01/19/2021 09:42, H.J. Lu wrote:
> > > On Tue, Jan 19, 2021 at 9:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > >
> > > > The 01/19/2021 09:10, H.J. Lu wrote:
> > > > > On Tue, Jan 19, 2021 at 9:03 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > >
> > > > > > The 01/19/2021 08:47, H.J. Lu wrote:
> > > > > > > On Tue, Jan 19, 2021 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > > > On Tue, Jan 19, 2021 at 7:24 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > > > > > > > > The 01/19/2021 06:48, H.J. Lu wrote:
> > > > > > > > > > On Tue, Jan 19, 2021 at 6:37 AM Adhemerval Zanella via Libc-alpha
> > > > > > > > > > <libc-alpha@sourceware.org> wrote:
> > > > > > > > > > > On 19/01/2021 11:35, Szabolcs Nagy wrote:
> > > > > > > > > > > > the problem is that _dl_phdr is used in ARCH_SETUP_TLS
> > > > > > > > > > > > (to get the tls program headers) so the __ehdr_start
> > > > > > > > > > > > magic should be before that (this only matters if auxv
> > > > > > > > > > > > lacks AT_PHDR for some reason, which should not happen
> > > > > > > > > > > > normally on linux, so testing won't show the problem)
> > > > > > > > > > >
> > > > > > > > > > > By normally do you mean it might happen on a specific kernel version
> > > > > > > > > > > or is it architecture specific?
> > > > > > > > >
> > > > > > > > > i guess __ehdr_start symbol can be useful and with it
> > > > > > > > > glibc does not have to depend on auxv (which an elf
> > > > > > > > > loader like valgrind/qemu-user may get wrong)
> > > > > > > > >
> > > > > > > > > however it is only used as a fallback and on linux
> > > > > > > > > AT_PHDR is always expected to be present. (i don't
> > > > > > > > > know if this ever triggers)
> > > > > > > >
> > > > > > > > Only used on Hurd?
> > > > > > >
> > > > > > > Does arm64 linker always define __ehdr_start?  If yes, can you drop
> > > > > > > "weak," to see if RELATIVE goes away?
> > > > > >
> > > > > > __ehdr_start support was added in binutils 2.23
> > > > >
> > > > > We may assume binutils >= 2.33 when building for static PIE
> > > > > since all static PIE linkers should define __ehdr_start.
> > > >
> > > > this piece of code is used for both static PIE and non-PIE,
> > > > but we already require binutils >= 2.25 for building glibc,
> > > > dropping weak should be fine.
> > > >
> > >
> > > It is safer to check BUILD_PIE_DEFAULT when dropping
> > > weak.
> >
> > ok.
> >
> > does static linking have weaker linker version requirement
> > than building glibc?
>
> Very unlikely.  But one may be forced to use the older linker
> for some reason.
>

BTW, I pushed

commit 22b79ed7f413cd980a7af0cf258da5bf82b6d5e5
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Fri Jan 15 06:46:12 2021 -0800

    Use <startup.h> in __libc_init_secure

to master.  You need to rebase.
diff mbox series

Patch

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 1e90dcb0a7..c2b59431a3 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -146,8 +146,6 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   int result;
 
 #ifndef SHARED
-  _dl_relocate_static_pie ();
-
   char **ev = &argv[argc + 1];
 
   __environ = ev;
@@ -169,24 +167,7 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   }
 #  endif
   _dl_aux_init (auxvec);
-  if (GL(dl_phdr) == NULL)
 # endif
-    {
-      /* Starting from binutils-2.23, the linker will define the
-         magic symbol __ehdr_start to point to our own ELF header
-         if it is visible in a segment that also includes the phdrs.
-         So we can set up _dl_phdr and _dl_phnum even without any
-         information from auxv.  */
-
-      extern const ElfW(Ehdr) __ehdr_start
-	__attribute__ ((weak, visibility ("hidden")));
-      if (&__ehdr_start != NULL)
-        {
-          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
-          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
-          GL(dl_phnum) = __ehdr_start.e_phnum;
-        }
-    }
 
   /* Initialize very early so that tunables can use it.  */
   __libc_init_secure ();
@@ -195,6 +176,11 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
 
   ARCH_INIT_CPU_FEATURES ();
 
+  /* Do static pie self relocation after tunables and cpu features
+     are setup for ifunc resolvers. Before this point relocations
+     must be avoided.  */
+  _dl_relocate_static_pie ();
+
   /* Perform IREL{,A} relocations.  */
   ARCH_SETUP_IREL ();
 
@@ -206,6 +192,26 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
      hwcap and platform fields available in the TCB.  */
   ARCH_APPLY_IREL ();
 
+# ifdef HAVE_AUX_VECTOR
+  if (GL(dl_phdr) == NULL)
+# endif
+    {
+      /* Starting from binutils-2.23, the linker will define the
+         magic symbol __ehdr_start to point to our own ELF header
+         if it is visible in a segment that also includes the phdrs.
+         So we can set up _dl_phdr and _dl_phnum even without any
+         information from auxv.  */
+
+      extern const ElfW(Ehdr) __ehdr_start
+	__attribute__ ((weak, visibility ("hidden")));
+      if (&__ehdr_start != NULL)
+        {
+          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
+          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
+          GL(dl_phnum) = __ehdr_start.e_phnum;
+        }
+    }
+
   /* Set up the stack checker's canary.  */
   uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
 # ifdef THREAD_SET_STACK_GUARD