csu: Skip ARCH_SETUP_IREL if _dl_relocate_static_pie applied IRELATIVE relocations [BZ #27164]
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
From: Siva Chandra Reddy <sivachandra@google.com>
For a static pie, _dl_relocate_static_pie applies IRELATIVE relocations
so ARCH_SETUP_IREL should not apply relocations again. The code
currently relies on ld -pie not defining
__rela_iplt_start/__rela_iplt_end (they end up as 0 as unresolved
undefined weak symbols).
However, LLD defines __rela_iplt_start/__rela_iplt_end regardless of
-no-pie or -pie, so in an LLD linked static pie, ARCH_SETUP_IREL would
re-apply the relocations in the range of [__rela_iplt_start,
__rela_iplt_end), causing a segfault.
Change _dl_relocate_static_pie to return an int, indicating whether the
relocations have been applied. This makes the intention clearer and
makes glibc buildable with LLD>=9.0 if we allow LLD at configure time.
In addition, this enables a future simplification to GNU ld: we can drop
a linker script difference between -no-pie and -pie.
Co-authored-by: Fangrui Song <maskray@google.com>
---
csu/libc-start.c | 8 +++++---
csu/static-reloc.c | 3 ++-
elf/dl-reloc-static-pie.c | 4 +++-
sysdeps/generic/ldsodefs.h | 7 ++++---
4 files changed, 14 insertions(+), 8 deletions(-)
Comments
On Thu, Jul 8, 2021 at 3:10 PM Fangrui Song <maskray@google.com> wrote:
>
> From: Siva Chandra Reddy <sivachandra@google.com>
>
> For a static pie, _dl_relocate_static_pie applies IRELATIVE relocations
> so ARCH_SETUP_IREL should not apply relocations again. The code
> currently relies on ld -pie not defining
> __rela_iplt_start/__rela_iplt_end (they end up as 0 as unresolved
> undefined weak symbols).
>
> However, LLD defines __rela_iplt_start/__rela_iplt_end regardless of
> -no-pie or -pie, so in an LLD linked static pie, ARCH_SETUP_IREL would
This is a bug in LLD. The fix is simple. Please fix it.
> re-apply the relocations in the range of [__rela_iplt_start,
> __rela_iplt_end), causing a segfault.
>
> Change _dl_relocate_static_pie to return an int, indicating whether the
> relocations have been applied. This makes the intention clearer and
> makes glibc buildable with LLD>=9.0 if we allow LLD at configure time.
>
> In addition, this enables a future simplification to GNU ld: we can drop
> a linker script difference between -no-pie and -pie.
>
> Co-authored-by: Fangrui Song <maskray@google.com>
> ---
> csu/libc-start.c | 8 +++++---
> csu/static-reloc.c | 3 ++-
> elf/dl-reloc-static-pie.c | 4 +++-
> sysdeps/generic/ldsodefs.h | 7 ++++---
> 4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 5b5913e7bf..32a69c58a2 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -296,10 +296,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> /* 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 ();
> + int relocs_applied = _dl_relocate_static_pie ();
>
> /* Perform IREL{,A} relocations. */
> - ARCH_SETUP_IREL ();
> + if (!relocs_applied)
> + ARCH_SETUP_IREL ();
>
> /* The stack guard goes into the TCB, so initialize it early. */
> ARCH_SETUP_TLS ();
> @@ -307,7 +308,8 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> /* In some architectures, IREL{,A} relocations happen after TLS setup in
> order to let IFUNC resolvers benefit from TCB information, e.g. powerpc's
> hwcap and platform fields available in the TCB. */
> - ARCH_APPLY_IREL ();
> + if (!relocs_applied)
> + ARCH_APPLY_IREL ();
>
> /* Set up the stack checker's canary. */
> uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
> diff --git a/csu/static-reloc.c b/csu/static-reloc.c
> index 972c524f28..9046d9f6a3 100644
> --- a/csu/static-reloc.c
> +++ b/csu/static-reloc.c
> @@ -19,8 +19,9 @@
> #if ENABLE_STATIC_PIE
> #include <ldsodefs.h>
>
> -void
> +int
> _dl_relocate_static_pie (void)
> {
> + return 0;
> }
> #endif
> diff --git a/elf/dl-reloc-static-pie.c b/elf/dl-reloc-static-pie.c
> index d5bd2f31e9..b707ef4bf1 100644
> --- a/elf/dl-reloc-static-pie.c
> +++ b/elf/dl-reloc-static-pie.c
> @@ -25,7 +25,7 @@
>
> /* Relocate static executable with PIE. */
>
> -void
> +int
> _dl_relocate_static_pie (void)
> {
> struct link_map *main_map = _dl_get_dl_main_map ();
> @@ -66,5 +66,7 @@ _dl_relocate_static_pie (void)
> with the run-time address of the r_debug structure */
> main_map->l_info[DT_DEBUG]->d_un.d_ptr = (ElfW(Addr)) r;
> # endif
> +
> + return 1;
> }
> #endif
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 176394de4d..a3996808f3 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1200,14 +1200,15 @@ void __tls_init_tp (void) attribute_hidden;
> void __libc_setup_tls (void);
>
> # if ENABLE_STATIC_PIE
> -/* Relocate static executable with PIE. */
> -extern void _dl_relocate_static_pie (void) attribute_hidden;
> +/* Relocate static executable with PIE. Returns 1 if relocations have
> + been applied. */
> +extern int _dl_relocate_static_pie (void) attribute_hidden;
>
> /* Get a pointer to _dl_main_map. */
> extern struct link_map * _dl_get_dl_main_map (void)
> __attribute__ ((visibility ("hidden")));
> # else
> -# define _dl_relocate_static_pie()
> +# define _dl_relocate_static_pie() 0
> # endif
> #endif
>
> --
> 2.32.0.93.g670b81a890-goog
>
On 7/8/21 6:10 PM, Fangrui Song wrote:
> From: Siva Chandra Reddy <sivachandra@google.com>
>
> For a static pie, _dl_relocate_static_pie applies IRELATIVE relocations
> so ARCH_SETUP_IREL should not apply relocations again. The code
> currently relies on ld -pie not defining
> __rela_iplt_start/__rela_iplt_end (they end up as 0 as unresolved
> undefined weak symbols).
Correct, this is how PIE and static PIE were designed by HJ.
> However, LLD defines __rela_iplt_start/__rela_iplt_end regardless of
> -no-pie or -pie, so in an LLD linked static pie, ARCH_SETUP_IREL would
> re-apply the relocations in the range of [__rela_iplt_start,
> __rela_iplt_end), causing a segfault.
The reason this issue has been raised is that our joint downstream
users are unable to use lld on existing systems to compile and test
static PIE binaries.
Ryan Houdek raised it on IRC, and I asked them to file a bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=28066
The average downstream users may take anywhere from 12-18 months to
get a new glibc in their systems.
If lld were to make the change today that would enable all downstream
users to be able to use lld to compile static PIE without needing to
get an updated glibc.
From such a perspective the lld change to match binutils enables the
feature for the most number of users the fastest.
> Change _dl_relocate_static_pie to return an int, indicating whether the
> relocations have been applied. This makes the intention clearer and
> makes glibc buildable with LLD>=9.0 if we allow LLD at configure time.
>
> In addition, this enables a future simplification to GNU ld: we can drop
> a linker script difference between -no-pie and -pie.
>
> Co-authored-by: Fangrui Song <maskray@google.com>
I'm not inclined to accept this path. Not because I think the patch is
technically wrong, but because fixing lld enables it immediately for the
most number of users (making the patch moot).
On 2021-07-12, Carlos O'Donell wrote:
>On 7/8/21 6:10 PM, Fangrui Song wrote:
>> From: Siva Chandra Reddy <sivachandra@google.com>
>>
>> For a static pie, _dl_relocate_static_pie applies IRELATIVE relocations
>> so ARCH_SETUP_IREL should not apply relocations again. The code
>> currently relies on ld -pie not defining
>> __rela_iplt_start/__rela_iplt_end (they end up as 0 as unresolved
>> undefined weak symbols).
>
>Correct, this is how PIE and static PIE were designed by HJ.
>
>> However, LLD defines __rela_iplt_start/__rela_iplt_end regardless of
>> -no-pie or -pie, so in an LLD linked static pie, ARCH_SETUP_IREL would
>> re-apply the relocations in the range of [__rela_iplt_start,
>> __rela_iplt_end), causing a segfault.
>
>The reason this issue has been raised is that our joint downstream
>users are unable to use lld on existing systems to compile and test
>static PIE binaries.
>
>Ryan Houdek raised it on IRC, and I asked them to file a bug:
>https://sourceware.org/bugzilla/show_bug.cgi?id=28066
>
>The average downstream users may take anywhere from 12-18 months to
>get a new glibc in their systems.
>
>If lld were to make the change today that would enable all downstream
>users to be able to use lld to compile static PIE without needing to
>get an updated glibc.
>
>From such a perspective the lld change to match binutils enables the
>feature for the most number of users the fastest.
>
>> Change _dl_relocate_static_pie to return an int, indicating whether the
>> relocations have been applied. This makes the intention clearer and
>> makes glibc buildable with LLD>=9.0 if we allow LLD at configure time.
>>
>> In addition, this enables a future simplification to GNU ld: we can drop
>> a linker script difference between -no-pie and -pie.
>>
>> Co-authored-by: Fangrui Song <maskray@google.com>
>
>I'm not inclined to accept this path. Not because I think the patch is
>technically wrong, but because fixing lld enables it immediately for the
>most number of users (making the patch moot).
True, changing lld is the fastest.
(I am not sure the word "fix" is appropriate because it isn't clear lld
is doing wrong here. HJ's argument "he designed __rela_iplt_start that
way" wasn't convincing. Static pie is new. I don't see why the old
mechanism doesn't deserve a refresh. (It is not my own opinion that
-static-pie/--no-dynamic-linker/-z dynamic-undefined-weak were not as
elegant as they could be.))
A toolchain project can do some workaround for a libc if this choice
makes a large community happy. However, I think it is important not to
take it granted. It is inadequate to just dismiss toolchain developers'
reasonable complaints. The libc should actively fix the issues so that
the toolchain will not need to bear unneeded code in the future.
I actually have contributed quite a few lld/ELF patches to work around
glibc. For this one I just feel it is not right to just patch lld/ELF
without fixing glibc.
On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
> A toolchain project can do some workaround for a libc if this choice
> makes a large community happy. However, I think it is important not to
> take it granted. It is inadequate to just dismiss toolchain developers'
> reasonable complaints. The libc should actively fix the issues so that
> the toolchain will not need to bear unneeded code in the future.
>
> I actually have contributed quite a few lld/ELF patches to work around
> glibc. For this one I just feel it is not right to just patch lld/ELF
> without fixing glibc.
What's the utility of having the __rela_iplt{_start,_end} symbols in all
binaries other than, maybe, simplifying the static linker
implementation? How does it improve things for the generated
application code in the end? AFAICT it is doing the opposite by
requiring application startup to add a conditional to work around the
presence of a redundant symbol.
Siddhesh
On 2021-07-13, Siddhesh Poyarekar wrote:
>On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
>>A toolchain project can do some workaround for a libc if this choice
>>makes a large community happy. However, I think it is important not to
>>take it granted. It is inadequate to just dismiss toolchain developers'
>>reasonable complaints. The libc should actively fix the issues so that
>>the toolchain will not need to bear unneeded code in the future.
>>
>>I actually have contributed quite a few lld/ELF patches to work around
>>glibc. For this one I just feel it is not right to just patch lld/ELF
>>without fixing glibc.
>
>What's the utility of having the __rela_iplt{_start,_end} symbols in
>all binaries other than, maybe, simplifying the static linker
>implementation? How does it improve things for the generated
>application code in the end? AFAICT it is doing the opposite by
>requiring application startup to add a conditional to work around the
>presence of a redundant symbol.
>
>Siddhesh
Please see the sentence from the first message
"In addition, this enables a future simplification to GNU ld: we can
drop a linker script difference between -no-pie and -pie."
This is the only difference other than image base difference.
On Tue, Jul 13, 2021 at 4:07 PM Fangrui Song via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On 2021-07-13, Siddhesh Poyarekar wrote:
> >On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
> >>A toolchain project can do some workaround for a libc if this choice
> >>makes a large community happy. However, I think it is important not to
> >>take it granted. It is inadequate to just dismiss toolchain developers'
> >>reasonable complaints. The libc should actively fix the issues so that
> >>the toolchain will not need to bear unneeded code in the future.
> >>
> >>I actually have contributed quite a few lld/ELF patches to work around
> >>glibc. For this one I just feel it is not right to just patch lld/ELF
> >>without fixing glibc.
> >
> >What's the utility of having the __rela_iplt{_start,_end} symbols in
> >all binaries other than, maybe, simplifying the static linker
> >implementation? How does it improve things for the generated
> >application code in the end? AFAICT it is doing the opposite by
> >requiring application startup to add a conditional to work around the
> >presence of a redundant symbol.
> >
> >Siddhesh
>
> Please see the sentence from the first message
> "In addition, this enables a future simplification to GNU ld: we can
> drop a linker script difference between -no-pie and -pie."
Did you mean non-PIE static and PIE static? Neither PIE nor PDE
define __rela_iplt{_start,_end}.
> This is the only difference other than image base difference.
There are many differences between non-PIE static and PIE static.
Non-PIE static doesn't have DT_XXX sections.
On Tue, Jul 13, 2021 at 4:21 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 4:07 PM Fangrui Song via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > On 2021-07-13, Siddhesh Poyarekar wrote:
> > >On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
> > >>A toolchain project can do some workaround for a libc if this choice
> > >>makes a large community happy. However, I think it is important not to
> > >>take it granted. It is inadequate to just dismiss toolchain developers'
> > >>reasonable complaints. The libc should actively fix the issues so that
> > >>the toolchain will not need to bear unneeded code in the future.
> > >>
> > >>I actually have contributed quite a few lld/ELF patches to work around
> > >>glibc. For this one I just feel it is not right to just patch lld/ELF
> > >>without fixing glibc.
> > >
> > >What's the utility of having the __rela_iplt{_start,_end} symbols in
> > >all binaries other than, maybe, simplifying the static linker
> > >implementation? How does it improve things for the generated
> > >application code in the end? AFAICT it is doing the opposite by
> > >requiring application startup to add a conditional to work around the
> > >presence of a redundant symbol.
> > >
> > >Siddhesh
> >
> > Please see the sentence from the first message
> > "In addition, this enables a future simplification to GNU ld: we can
> > drop a linker script difference between -no-pie and -pie."
>
> Did you mean non-PIE static and PIE static? Neither PIE nor PDE
> define __rela_iplt{_start,_end}.
>
> > This is the only difference other than image base difference.
>
> There are many differences between non-PIE static and PIE static.
> Non-PIE static doesn't have DT_XXX sections.
% diff -U1 =(ld.bfd --verbose) =(ld.bfd -pie --verbose)
--- /tmp/zshEtZMxJ 2021-07-13 16:30:50.228732445 -0700
+++ /tmp/zshNM1wJL 2021-07-13 16:30:50.232732450 -0700
@@ -12,3 +12,3 @@
==================================================
-/* Script for -z combreloc -z separate-code */
+/* Script for -pie -z combreloc -z separate-code */
/* Copyright (C) 2014-2020 Free Software Foundation, Inc.
@@ -24,3 +24,3 @@
{
- PROVIDE (__executable_start = SEGMENT_START("text-segment",
0x400000)); . = SEGMENT_START("text-segment", 0x400000) +
SIZEOF_HEADERS;
+ PROVIDE (__executable_start = SEGMENT_START("text-segment", 0)); .
= SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
.interp : { *(.interp) }
@@ -55,5 +55,3 @@
*(.rela.plt)
- PROVIDE_HIDDEN (__rela_iplt_start = .);
*(.rela.iplt)
- PROVIDE_HIDDEN (__rela_iplt_end = .);
}
On Tue, Jul 13, 2021 at 4:31 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> On Tue, Jul 13, 2021 at 4:21 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 4:07 PM Fangrui Song via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> > >
> > > On 2021-07-13, Siddhesh Poyarekar wrote:
> > > >On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
> > > >>A toolchain project can do some workaround for a libc if this choice
> > > >>makes a large community happy. However, I think it is important not to
> > > >>take it granted. It is inadequate to just dismiss toolchain developers'
> > > >>reasonable complaints. The libc should actively fix the issues so that
> > > >>the toolchain will not need to bear unneeded code in the future.
> > > >>
> > > >>I actually have contributed quite a few lld/ELF patches to work around
> > > >>glibc. For this one I just feel it is not right to just patch lld/ELF
> > > >>without fixing glibc.
> > > >
> > > >What's the utility of having the __rela_iplt{_start,_end} symbols in
> > > >all binaries other than, maybe, simplifying the static linker
> > > >implementation? How does it improve things for the generated
> > > >application code in the end? AFAICT it is doing the opposite by
> > > >requiring application startup to add a conditional to work around the
> > > >presence of a redundant symbol.
> > > >
> > > >Siddhesh
> > >
> > > Please see the sentence from the first message
> > > "In addition, this enables a future simplification to GNU ld: we can
> > > drop a linker script difference between -no-pie and -pie."
> >
> > Did you mean non-PIE static and PIE static? Neither PIE nor PDE
> > define __rela_iplt{_start,_end}.
> >
> > > This is the only difference other than image base difference.
> >
> > There are many differences between non-PIE static and PIE static.
> > Non-PIE static doesn't have DT_XXX sections.
>
> % diff -U1 =(ld.bfd --verbose) =(ld.bfd -pie --verbose)
> --- /tmp/zshEtZMxJ 2021-07-13 16:30:50.228732445 -0700
> +++ /tmp/zshNM1wJL 2021-07-13 16:30:50.232732450 -0700
> @@ -12,3 +12,3 @@
> ==================================================
> -/* Script for -z combreloc -z separate-code */
> +/* Script for -pie -z combreloc -z separate-code */
> /* Copyright (C) 2014-2020 Free Software Foundation, Inc.
> @@ -24,3 +24,3 @@
> {
> - PROVIDE (__executable_start = SEGMENT_START("text-segment",
> 0x400000)); . = SEGMENT_START("text-segment", 0x400000) +
> SIZEOF_HEADERS;
> + PROVIDE (__executable_start = SEGMENT_START("text-segment", 0)); .
> = SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
> .interp : { *(.interp) }
> @@ -55,5 +55,3 @@
> *(.rela.plt)
> - PROVIDE_HIDDEN (__rela_iplt_start = .);
> *(.rela.iplt)
> - PROVIDE_HIDDEN (__rela_iplt_end = .);
> }
Here is the deal:
1. ld uses the same linker script for both PDE static and PDE. We need
__rela_iplt{_start,_end} for PDE static. That is why there are
PROVIDE_HIDDEN (__rela_iplt_start = .);
*(.rela.iplt)
PROVIDE_HIDDEN (__rela_iplt_end = .);
Since PDE is linked against libc.so which doesn't reference
__rela_iplt{_start,_end}, these symbols are not defined for PDE.
2. ld uses the same linker script for both PIE static and PIE. There is
no need for __rela_iplt{_start,_end}.
Are you suggesting to use the same linker scripts for PDE, PDE static,
PIE and PIE static?
On Tue, Jul 13, 2021 at 4:48 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 4:31 PM Fāng-ruì Sòng <maskray@google.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 4:21 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Jul 13, 2021 at 4:07 PM Fangrui Song via Libc-alpha
> > > <libc-alpha@sourceware.org> wrote:
> > > >
> > > > On 2021-07-13, Siddhesh Poyarekar wrote:
> > > > >On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
> > > > >>A toolchain project can do some workaround for a libc if this choice
> > > > >>makes a large community happy. However, I think it is important not to
> > > > >>take it granted. It is inadequate to just dismiss toolchain developers'
> > > > >>reasonable complaints. The libc should actively fix the issues so that
> > > > >>the toolchain will not need to bear unneeded code in the future.
> > > > >>
> > > > >>I actually have contributed quite a few lld/ELF patches to work around
> > > > >>glibc. For this one I just feel it is not right to just patch lld/ELF
> > > > >>without fixing glibc.
> > > > >
> > > > >What's the utility of having the __rela_iplt{_start,_end} symbols in
> > > > >all binaries other than, maybe, simplifying the static linker
> > > > >implementation? How does it improve things for the generated
> > > > >application code in the end? AFAICT it is doing the opposite by
> > > > >requiring application startup to add a conditional to work around the
> > > > >presence of a redundant symbol.
> > > > >
> > > > >Siddhesh
> > > >
> > > > Please see the sentence from the first message
> > > > "In addition, this enables a future simplification to GNU ld: we can
> > > > drop a linker script difference between -no-pie and -pie."
> > >
> > > Did you mean non-PIE static and PIE static? Neither PIE nor PDE
> > > define __rela_iplt{_start,_end}.
> > >
> > > > This is the only difference other than image base difference.
> > >
> > > There are many differences between non-PIE static and PIE static.
> > > Non-PIE static doesn't have DT_XXX sections.
> >
> > % diff -U1 =(ld.bfd --verbose) =(ld.bfd -pie --verbose)
> > --- /tmp/zshEtZMxJ 2021-07-13 16:30:50.228732445 -0700
> > +++ /tmp/zshNM1wJL 2021-07-13 16:30:50.232732450 -0700
> > @@ -12,3 +12,3 @@
> > ==================================================
> > -/* Script for -z combreloc -z separate-code */
> > +/* Script for -pie -z combreloc -z separate-code */
> > /* Copyright (C) 2014-2020 Free Software Foundation, Inc.
> > @@ -24,3 +24,3 @@
> > {
> > - PROVIDE (__executable_start = SEGMENT_START("text-segment",
> > 0x400000)); . = SEGMENT_START("text-segment", 0x400000) +
> > SIZEOF_HEADERS;
> > + PROVIDE (__executable_start = SEGMENT_START("text-segment", 0)); .
> > = SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
> > .interp : { *(.interp) }
> > @@ -55,5 +55,3 @@
> > *(.rela.plt)
> > - PROVIDE_HIDDEN (__rela_iplt_start = .);
> > *(.rela.iplt)
> > - PROVIDE_HIDDEN (__rela_iplt_end = .);
> > }
>
> Here is the deal:
>
> 1. ld uses the same linker script for both PDE static and PDE. We need
> __rela_iplt{_start,_end} for PDE static. That is why there are
>
> PROVIDE_HIDDEN (__rela_iplt_start = .);
> *(.rela.iplt)
> PROVIDE_HIDDEN (__rela_iplt_end = .);
>
> Since PDE is linked against libc.so which doesn't reference
> __rela_iplt{_start,_end}, these symbols are not defined for PDE.
>
> 2. ld uses the same linker script for both PIE static and PIE. There is
> no need for __rela_iplt{_start,_end}.
>
> Are you suggesting to use the same linker scripts for PDE, PDE static,
> PIE and PIE static?
Because of the image base difference, PDE/PIE linker scripts cannot be
entirely identical,
but symbol differences should be reduced.
BTW: diff -u =(ld.bfd -pie --verbose) =(ld.bfd -shared --verbose) has
some PROVIDE_HIDDEN differences.
Since these symbols are PROVIDE style, these differences are
artificial and should be reduced as well.
On Tue, Jul 13, 2021 at 4:58 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> On Tue, Jul 13, 2021 at 4:48 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 4:31 PM Fāng-ruì Sòng <maskray@google.com> wrote:
> > >
> > > On Tue, Jul 13, 2021 at 4:21 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Tue, Jul 13, 2021 at 4:07 PM Fangrui Song via Libc-alpha
> > > > <libc-alpha@sourceware.org> wrote:
> > > > >
> > > > > On 2021-07-13, Siddhesh Poyarekar wrote:
> > > > > >On 7/13/21 1:36 PM, Fangrui Song via Libc-alpha wrote:
> > > > > >>A toolchain project can do some workaround for a libc if this choice
> > > > > >>makes a large community happy. However, I think it is important not to
> > > > > >>take it granted. It is inadequate to just dismiss toolchain developers'
> > > > > >>reasonable complaints. The libc should actively fix the issues so that
> > > > > >>the toolchain will not need to bear unneeded code in the future.
> > > > > >>
> > > > > >>I actually have contributed quite a few lld/ELF patches to work around
> > > > > >>glibc. For this one I just feel it is not right to just patch lld/ELF
> > > > > >>without fixing glibc.
> > > > > >
> > > > > >What's the utility of having the __rela_iplt{_start,_end} symbols in
> > > > > >all binaries other than, maybe, simplifying the static linker
> > > > > >implementation? How does it improve things for the generated
> > > > > >application code in the end? AFAICT it is doing the opposite by
> > > > > >requiring application startup to add a conditional to work around the
> > > > > >presence of a redundant symbol.
> > > > > >
> > > > > >Siddhesh
> > > > >
> > > > > Please see the sentence from the first message
> > > > > "In addition, this enables a future simplification to GNU ld: we can
> > > > > drop a linker script difference between -no-pie and -pie."
> > > >
> > > > Did you mean non-PIE static and PIE static? Neither PIE nor PDE
> > > > define __rela_iplt{_start,_end}.
> > > >
> > > > > This is the only difference other than image base difference.
> > > >
> > > > There are many differences between non-PIE static and PIE static.
> > > > Non-PIE static doesn't have DT_XXX sections.
> > >
> > > % diff -U1 =(ld.bfd --verbose) =(ld.bfd -pie --verbose)
> > > --- /tmp/zshEtZMxJ 2021-07-13 16:30:50.228732445 -0700
> > > +++ /tmp/zshNM1wJL 2021-07-13 16:30:50.232732450 -0700
> > > @@ -12,3 +12,3 @@
> > > ==================================================
> > > -/* Script for -z combreloc -z separate-code */
> > > +/* Script for -pie -z combreloc -z separate-code */
> > > /* Copyright (C) 2014-2020 Free Software Foundation, Inc.
> > > @@ -24,3 +24,3 @@
> > > {
> > > - PROVIDE (__executable_start = SEGMENT_START("text-segment",
> > > 0x400000)); . = SEGMENT_START("text-segment", 0x400000) +
> > > SIZEOF_HEADERS;
> > > + PROVIDE (__executable_start = SEGMENT_START("text-segment", 0)); .
> > > = SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
> > > .interp : { *(.interp) }
> > > @@ -55,5 +55,3 @@
> > > *(.rela.plt)
> > > - PROVIDE_HIDDEN (__rela_iplt_start = .);
> > > *(.rela.iplt)
> > > - PROVIDE_HIDDEN (__rela_iplt_end = .);
> > > }
> >
> > Here is the deal:
> >
> > 1. ld uses the same linker script for both PDE static and PDE. We need
> > __rela_iplt{_start,_end} for PDE static. That is why there are
> >
> > PROVIDE_HIDDEN (__rela_iplt_start = .);
> > *(.rela.iplt)
> > PROVIDE_HIDDEN (__rela_iplt_end = .);
> >
> > Since PDE is linked against libc.so which doesn't reference
> > __rela_iplt{_start,_end}, these symbols are not defined for PDE.
> >
> > 2. ld uses the same linker script for both PIE static and PIE. There is
> > no need for __rela_iplt{_start,_end}.
> >
> > Are you suggesting to use the same linker scripts for PDE, PDE static,
> > PIE and PIE static?
>
> Because of the image base difference, PDE/PIE linker scripts cannot be
> entirely identical,
__rela_iplt{_start,_end} is just another difference.
> but symbol differences should be reduced.
I don't think this should be the reason to change glibc.
> BTW: diff -u =(ld.bfd -pie --verbose) =(ld.bfd -shared --verbose) has
> some PROVIDE_HIDDEN differences.
> Since these symbols are PROVIDE style, these differences are
> artificial and should be reduced as well.
On 7/14/21 4:36 AM, Fangrui Song wrote:
> Please see the sentence from the first message
> "In addition, this enables a future simplification to GNU ld: we can
> drop a linker script difference between -no-pie and -pie."
>
> This is the only difference other than image base difference.
I did see the first message and indeed, all messages in the thread
before asking that question. Perhaps my wording wasn't specific enough:
what is the benefit to *users* of having those symbols in dynamic
binaries? If the only reason what you mentioned in your first email,
then this patch proposes to add runtime overhead to make what is
essentially a cosmetic difference to GNU ld. That doesn't make sense.
In that context, lld behaviour needs to be changed since it adds symbols
specifically meant to enable ifunc resolution in non-pie static binaries
to all generated binaries without any known benefit to end user
applications.
Siddhesh
On Tue, Jul 13, 2021 at 7:15 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 7/14/21 4:36 AM, Fangrui Song wrote:
> > Please see the sentence from the first message
> > "In addition, this enables a future simplification to GNU ld: we can
> > drop a linker script difference between -no-pie and -pie."
> >
> > This is the only difference other than image base difference.
>
> I did see the first message and indeed, all messages in the thread
> before asking that question. Perhaps my wording wasn't specific enough:
> what is the benefit to *users* of having those symbols in dynamic
> binaries? If the only reason what you mentioned in your first email,
> then this patch proposes to add runtime overhead to make what is
> essentially a cosmetic difference to GNU ld. That doesn't make sense.
>
> In that context, lld behaviour needs to be changed since it adds symbols
> specifically meant to enable ifunc resolution in non-pie static binaries
> to all generated binaries without any known benefit to end user
> applications.
>
> Siddhesh
Circling back... Finally I conceded and changed LLD 13.0.0 to not
define __rela_iplt_start/__rela_iplt_end for -pie, for the sake of
users.
For the record, Android bionic uses the same IRELATIVE code path for
both static non-pie and static pie.
They had to adapt to undefined __rela_iplt_start for static pie:
https://r.android.com/1809796
I described this issue in detail in a chapter of
https://maskray.me/blog/2021-09-05-build-glibc-with-lld
@@ -296,10 +296,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
/* 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 ();
+ int relocs_applied = _dl_relocate_static_pie ();
/* Perform IREL{,A} relocations. */
- ARCH_SETUP_IREL ();
+ if (!relocs_applied)
+ ARCH_SETUP_IREL ();
/* The stack guard goes into the TCB, so initialize it early. */
ARCH_SETUP_TLS ();
@@ -307,7 +308,8 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
/* In some architectures, IREL{,A} relocations happen after TLS setup in
order to let IFUNC resolvers benefit from TCB information, e.g. powerpc's
hwcap and platform fields available in the TCB. */
- ARCH_APPLY_IREL ();
+ if (!relocs_applied)
+ ARCH_APPLY_IREL ();
/* Set up the stack checker's canary. */
uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
@@ -19,8 +19,9 @@
#if ENABLE_STATIC_PIE
#include <ldsodefs.h>
-void
+int
_dl_relocate_static_pie (void)
{
+ return 0;
}
#endif
@@ -25,7 +25,7 @@
/* Relocate static executable with PIE. */
-void
+int
_dl_relocate_static_pie (void)
{
struct link_map *main_map = _dl_get_dl_main_map ();
@@ -66,5 +66,7 @@ _dl_relocate_static_pie (void)
with the run-time address of the r_debug structure */
main_map->l_info[DT_DEBUG]->d_un.d_ptr = (ElfW(Addr)) r;
# endif
+
+ return 1;
}
#endif
@@ -1200,14 +1200,15 @@ void __tls_init_tp (void) attribute_hidden;
void __libc_setup_tls (void);
# if ENABLE_STATIC_PIE
-/* Relocate static executable with PIE. */
-extern void _dl_relocate_static_pie (void) attribute_hidden;
+/* Relocate static executable with PIE. Returns 1 if relocations have
+ been applied. */
+extern int _dl_relocate_static_pie (void) attribute_hidden;
/* Get a pointer to _dl_main_map. */
extern struct link_map * _dl_get_dl_main_map (void)
__attribute__ ((visibility ("hidden")));
# else
-# define _dl_relocate_static_pie()
+# define _dl_relocate_static_pie() 0
# endif
#endif