[PATCHv2] Fix float128 IFUNC relocations on ppc64le [BZ #21707]
Commit Message
Changes since version 1:
- Added a testcase. This is now validating both statically and
dynamically linked executables.
- Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
- Added a comment to csu/libc-start.c
- Added a comment to csu/libc-tls.c
-- 8< --
The patch proposed by Peter Bergner [1] to libgc in order to fix
[BZ #21707] adds a dependency on a symbol provided by the loader,
forcing the loader to be linked to tests after libgcc was linked.
It also requires to read the thread pointer during IRELA relocations.
Tested on powerpc, powerpc64, powerpc64le, s390x and x86_64.
[1] https://sourceware.org/ml/libc-alpha/2017-06/msg01383.html
2017-07-08 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>
[BZ #21707]
* csu/libc-start.c (LIBC_START_MAIN): Perform IREL{,A}
relocations after initializing the TCB on statically linked
executables..
* csu/libc-tls.c (__libc_setup_tls): Add a comment about
IREL{,A} relocations.
* elf/Makefile (tests-static-normal): Add tst-tlsifunc-static.
(tests): Add tst-tlsifunc.
* elf/tst-tlsifunc.c: New file.
* elf/tst-tlsifunc-static.c: Likewise.
* sysdeps/powerpc/powerpc64le/Makefile (f128-loader-link): New
variable.
[$(subdir) = math] (test-float128% test-ifloat128%): Force
linking to the loader after linking to libgcc.
[$(subdir) = wcsmbs stdlib] (bug-strtod bug-strtod2 bug-strtod2)
(tst-strtod-round tst-wcstod-round tst-strtod6 tst-strrom)
(tst-strfrom-locale strfrom-skeleton): Likewise.
---
csu/libc-start.c | 11 +++---
csu/libc-tls.c | 2 ++
elf/Makefile | 5 +--
elf/tst-tlsifunc-static.c | 19 +++++++++++
elf/tst-tlsifunc.c | 66 ++++++++++++++++++++++++++++++++++++
sysdeps/powerpc/powerpc64le/Makefile | 10 ++++++
6 files changed, 107 insertions(+), 6 deletions(-)
create mode 100644 elf/tst-tlsifunc-static.c
create mode 100644 elf/tst-tlsifunc.c
Comments
On 07/08/2017 08:31 PM, Tulio Magno Quites Machado Filho wrote:
> Changes since version 1:
>
> - Added a testcase. This is now validating both statically and
> dynamically linked executables.
> - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
> - Added a comment to csu/libc-start.c
> - Added a comment to csu/libc-tls.c
>
> -- 8< --
>
> The patch proposed by Peter Bergner [1] to libgc in order to fix
Typo, should be “libgcc”.
> +__thread int bar;
> +static int * bar_ptr = NULL;
No space after “*”?
> + else {
> + printf ("FAIL: IFUNC resolver not called once.\n");
> + ret = 1;
> + }
> +
> + if (&bar == bar_ptr)
> + printf ("PASS: Address read from IFUNC resolver is correct.\n");
> + else {
> + printf ("FAIL: Address read from IFUNC resolver is incorrect.\n");
> + ret = 1;
> + }
The “{” should be on its own line.
Thanks,
Florian
On Sat, Jul 8, 2017 at 11:31 AM, Tulio Magno Quites Machado Filho
<tuliom@linux.vnet.ibm.com> wrote:
> Changes since version 1:
>
> - Added a testcase. This is now validating both statically and
> dynamically linked executables.
> - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
> - Added a comment to csu/libc-start.c
> - Added a comment to csu/libc-tls.c
>
> -- 8< --
>
> The patch proposed by Peter Bergner [1] to libgc in order to fix
> [BZ #21707] adds a dependency on a symbol provided by the loader,
> forcing the loader to be linked to tests after libgcc was linked.
>
> It also requires to read the thread pointer during IRELA relocations.
>
> Tested on powerpc, powerpc64, powerpc64le, s390x and x86_64.
>
> [1] https://sourceware.org/ml/libc-alpha/2017-06/msg01383.html
>
> 2017-07-08 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>
>
> [BZ #21707]
> * csu/libc-start.c (LIBC_START_MAIN): Perform IREL{,A}
> relocations after initializing the TCB on statically linked
> executables..
> * csu/libc-tls.c (__libc_setup_tls): Add a comment about
> IREL{,A} relocations.
> * elf/Makefile (tests-static-normal): Add tst-tlsifunc-static.
> (tests): Add tst-tlsifunc.
> * elf/tst-tlsifunc.c: New file.
> * elf/tst-tlsifunc-static.c: Likewise.
> * sysdeps/powerpc/powerpc64le/Makefile (f128-loader-link): New
> variable.
> [$(subdir) = math] (test-float128% test-ifloat128%): Force
> linking to the loader after linking to libgcc.
> [$(subdir) = wcsmbs stdlib] (bug-strtod bug-strtod2 bug-strtod2)
> (tst-strtod-round tst-wcstod-round tst-strtod6 tst-strrom)
> (tst-strfrom-locale strfrom-skeleton): Likewise.
> ---
> csu/libc-start.c | 11 +++---
> csu/libc-tls.c | 2 ++
> elf/Makefile | 5 +--
> elf/tst-tlsifunc-static.c | 19 +++++++++++
> elf/tst-tlsifunc.c | 66 ++++++++++++++++++++++++++++++++++++
> sysdeps/powerpc/powerpc64le/Makefile | 10 ++++++
> 6 files changed, 107 insertions(+), 6 deletions(-)
> create mode 100644 elf/tst-tlsifunc-static.c
> create mode 100644 elf/tst-tlsifunc.c
>
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index c2dd159..84b7f99 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -188,12 +188,15 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>
> ARCH_INIT_CPU_FEATURES ();
>
> - /* Perform IREL{,A} relocations. */
> - apply_irel ();
> -
> /* The stack guard goes into the TCB, so initialize it early. */
> __libc_setup_tls ();
>
> + /* Perform IREL{,A} relocations.
> + Note: the relocations must happen after TLS initialization so that
> + IFUNC resolvers can benefit from thread-local storage, e.g. powerpc's
> + hwcap and platform fields available in the TCB. */
> + apply_irel ();
> +
> /* Set up the stack checker's canary. */
> uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
> # ifdef THREAD_SET_STACK_GUARD
> @@ -224,7 +227,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> __pointer_chk_guard_local = pointer_chk_guard;
> # endif
>
> -#endif
> +#endif /* !SHARED */
>
apply_irel should be called as early as possible. If it doesn't work for
powerpc, we should provide a generic version and powerpc can
provide a special one, I oppose changing the order in generic file.
H.J.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Sat, Jul 8, 2017 at 11:31 AM, Tulio Magno Quites Machado Filho
> <tuliom@linux.vnet.ibm.com> wrote:
>> Changes since version 1:
>>
>> - Added a testcase. This is now validating both statically and
>> dynamically linked executables.
>> - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
>> - Added a comment to csu/libc-start.c
>> - Added a comment to csu/libc-tls.c
>>
>> -- 8< --
>>
>> The patch proposed by Peter Bergner [1] to libgc in order to fix
>> [BZ #21707] adds a dependency on a symbol provided by the loader,
>> forcing the loader to be linked to tests after libgcc was linked.
>>
>> It also requires to read the thread pointer during IRELA relocations.
>>
>> Tested on powerpc, powerpc64, powerpc64le, s390x and x86_64.
>>
>> [1] https://sourceware.org/ml/libc-alpha/2017-06/msg01383.html
>>
>> 2017-07-08 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>
>>
>> [BZ #21707]
>> * csu/libc-start.c (LIBC_START_MAIN): Perform IREL{,A}
>> relocations after initializing the TCB on statically linked
>> executables..
>> * csu/libc-tls.c (__libc_setup_tls): Add a comment about
>> IREL{,A} relocations.
>> * elf/Makefile (tests-static-normal): Add tst-tlsifunc-static.
>> (tests): Add tst-tlsifunc.
>> * elf/tst-tlsifunc.c: New file.
>> * elf/tst-tlsifunc-static.c: Likewise.
>> * sysdeps/powerpc/powerpc64le/Makefile (f128-loader-link): New
>> variable.
>> [$(subdir) = math] (test-float128% test-ifloat128%): Force
>> linking to the loader after linking to libgcc.
>> [$(subdir) = wcsmbs stdlib] (bug-strtod bug-strtod2 bug-strtod2)
>> (tst-strtod-round tst-wcstod-round tst-strtod6 tst-strrom)
>> (tst-strfrom-locale strfrom-skeleton): Likewise.
>> ---
>> csu/libc-start.c | 11 +++---
>> csu/libc-tls.c | 2 ++
>> elf/Makefile | 5 +--
>> elf/tst-tlsifunc-static.c | 19 +++++++++++
>> elf/tst-tlsifunc.c | 66 ++++++++++++++++++++++++++++++++++++
>> sysdeps/powerpc/powerpc64le/Makefile | 10 ++++++
>> 6 files changed, 107 insertions(+), 6 deletions(-)
>> create mode 100644 elf/tst-tlsifunc-static.c
>> create mode 100644 elf/tst-tlsifunc.c
>>
>> diff --git a/csu/libc-start.c b/csu/libc-start.c
>> index c2dd159..84b7f99 100644
>> --- a/csu/libc-start.c
>> +++ b/csu/libc-start.c
>> @@ -188,12 +188,15 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>>
>> ARCH_INIT_CPU_FEATURES ();
>>
>> - /* Perform IREL{,A} relocations. */
>> - apply_irel ();
>> -
>> /* The stack guard goes into the TCB, so initialize it early. */
>> __libc_setup_tls ();
>>
>> + /* Perform IREL{,A} relocations.
>> + Note: the relocations must happen after TLS initialization so that
>> + IFUNC resolvers can benefit from thread-local storage, e.g. powerpc's
>> + hwcap and platform fields available in the TCB. */
>> + apply_irel ();
>> +
>> /* Set up the stack checker's canary. */
>> uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
>> # ifdef THREAD_SET_STACK_GUARD
>> @@ -224,7 +227,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>> __pointer_chk_guard_local = pointer_chk_guard;
>> # endif
>>
>> -#endif
>> +#endif /* !SHARED */
>>
>
> apply_irel should be called as early as possible.
Why? Could you elaborate, please?
Florian Weimer <fweimer@redhat.com> writes:
> On 07/08/2017 08:31 PM, Tulio Magno Quites Machado Filho wrote:
>> Changes since version 1:
>>
>> - Added a testcase. This is now validating both statically and
>> dynamically linked executables.
>> - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
>> - Added a comment to csu/libc-start.c
>> - Added a comment to csu/libc-tls.c
>>
>> -- 8< --
>>
>> The patch proposed by Peter Bergner [1] to libgc in order to fix
>
> Typo, should be “libgcc”.
Fixed locally.
>> +__thread int bar;
>> +static int * bar_ptr = NULL;
>
> No space after “*”?
Fixed.
>> + else {
>> + printf ("FAIL: IFUNC resolver not called once.\n");
>> + ret = 1;
>> + }
>> +
>> + if (&bar == bar_ptr)
>> + printf ("PASS: Address read from IFUNC resolver is correct.\n");
>> + else {
>> + printf ("FAIL: Address read from IFUNC resolver is incorrect.\n");
>> + ret = 1;
>> + }
>
> The “{” should be on its own line.
Fixed both cases and re-indented the lines:
else
{
printf ("FAIL: IFUNC resolver not called once.\n");
ret = 1;
}
if (&bar == bar_ptr)
printf ("PASS: Address read from IFUNC resolver is correct.\n");
else
{
printf ("FAIL: Address read from IFUNC resolver is incorrect.\n");
ret = 1;
}
Thanks!
On Sat, Jul 8, 2017 at 4:12 PM, Tulio Magno Quites Machado Filho
<tuliom@linux.vnet.ibm.com> wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>> On Sat, Jul 8, 2017 at 11:31 AM, Tulio Magno Quites Machado Filho
>> <tuliom@linux.vnet.ibm.com> wrote:
>>> Changes since version 1:
>>>
>>> - Added a testcase. This is now validating both statically and
>>> dynamically linked executables.
>>> - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
>>> - Added a comment to csu/libc-start.c
>>> - Added a comment to csu/libc-tls.c
>>>
>>> -- 8< --
>>>
>>> The patch proposed by Peter Bergner [1] to libgc in order to fix
>>> [BZ #21707] adds a dependency on a symbol provided by the loader,
>>> forcing the loader to be linked to tests after libgcc was linked.
>>>
>>> It also requires to read the thread pointer during IRELA relocations.
>>>
>>> Tested on powerpc, powerpc64, powerpc64le, s390x and x86_64.
>>>
>>> [1] https://sourceware.org/ml/libc-alpha/2017-06/msg01383.html
>>>
>>> 2017-07-08 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>
>>>
>>> [BZ #21707]
>>> * csu/libc-start.c (LIBC_START_MAIN): Perform IREL{,A}
>>> relocations after initializing the TCB on statically linked
>>> executables..
>>> * csu/libc-tls.c (__libc_setup_tls): Add a comment about
>>> IREL{,A} relocations.
>>> * elf/Makefile (tests-static-normal): Add tst-tlsifunc-static.
>>> (tests): Add tst-tlsifunc.
>>> * elf/tst-tlsifunc.c: New file.
>>> * elf/tst-tlsifunc-static.c: Likewise.
>>> * sysdeps/powerpc/powerpc64le/Makefile (f128-loader-link): New
>>> variable.
>>> [$(subdir) = math] (test-float128% test-ifloat128%): Force
>>> linking to the loader after linking to libgcc.
>>> [$(subdir) = wcsmbs stdlib] (bug-strtod bug-strtod2 bug-strtod2)
>>> (tst-strtod-round tst-wcstod-round tst-strtod6 tst-strrom)
>>> (tst-strfrom-locale strfrom-skeleton): Likewise.
>>> ---
>>> csu/libc-start.c | 11 +++---
>>> csu/libc-tls.c | 2 ++
>>> elf/Makefile | 5 +--
>>> elf/tst-tlsifunc-static.c | 19 +++++++++++
>>> elf/tst-tlsifunc.c | 66 ++++++++++++++++++++++++++++++++++++
>>> sysdeps/powerpc/powerpc64le/Makefile | 10 ++++++
>>> 6 files changed, 107 insertions(+), 6 deletions(-)
>>> create mode 100644 elf/tst-tlsifunc-static.c
>>> create mode 100644 elf/tst-tlsifunc.c
>>>
>>> diff --git a/csu/libc-start.c b/csu/libc-start.c
>>> index c2dd159..84b7f99 100644
>>> --- a/csu/libc-start.c
>>> +++ b/csu/libc-start.c
>>> @@ -188,12 +188,15 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>>>
>>> ARCH_INIT_CPU_FEATURES ();
>>>
>>> - /* Perform IREL{,A} relocations. */
>>> - apply_irel ();
>>> -
>>> /* The stack guard goes into the TCB, so initialize it early. */
>>> __libc_setup_tls ();
>>>
>>> + /* Perform IREL{,A} relocations.
>>> + Note: the relocations must happen after TLS initialization so that
>>> + IFUNC resolvers can benefit from thread-local storage, e.g. powerpc's
>>> + hwcap and platform fields available in the TCB. */
>>> + apply_irel ();
>>> +
>>> /* Set up the stack checker's canary. */
>>> uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
>>> # ifdef THREAD_SET_STACK_GUARD
>>> @@ -224,7 +227,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>>> __pointer_chk_guard_local = pointer_chk_guard;
>>> # endif
>>>
>>> -#endif
>>> +#endif /* !SHARED */
>>>
>>
>> apply_irel should be called as early as possible.
>
> Why? Could you elaborate, please?
>
To use IFUNC in static executables, apply_irel should be called before
any functions with IFUNC implementation is called. At the moment,
a few functions are used before apply_irel is called. To address it,
we can move apply_irel forward. Call it later makes it worse.
H.J.
On 8 Jul 2017, Tulio Magno Quites Machado Filho said this:
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index c2dd159..84b7f99 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -188,12 +188,15 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>
> ARCH_INIT_CPU_FEATURES ();
>
> - /* Perform IREL{,A} relocations. */
> - apply_irel ();
> -
> /* The stack guard goes into the TCB, so initialize it early. */
> __libc_setup_tls ();
>
> + /* Perform IREL{,A} relocations.
> + Note: the relocations must happen after TLS initialization so that
> + IFUNC resolvers can benefit from thread-local storage, e.g. powerpc's
> + hwcap and platform fields available in the TCB. */
> + apply_irel ();
> +
> /* Set up the stack checker's canary. */
> uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
> # ifdef THREAD_SET_STACK_GUARD
Note: I originally moved this above __libc_setup_tls() so that IFUNCs
could be stack-protected on statically-linked applications, but
subsequently found that this caused trouble with dynamically-linked
programs, so ended up de-protecting all the IFUNC resolvers anyway.
(See commit 003a27e8195470f470f4d9384ca70d4e9fc8bd1b.)
If this passes make check with --enable-stack-protector=all, I'm happy
with it.
On Sat, 8 Jul 2017, Tulio Magno Quites Machado Filho wrote:
> diff --git a/elf/Makefile b/elf/Makefile
> index 201b328..b603e89 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -147,12 +147,13 @@ tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \
> tst-tlsalign-static tst-tlsalign-extern-static \
> tst-linkall-static tst-env-setuid tst-env-setuid-tunables
> tests-static-internal := tst-tls1-static tst-tls2-static \
> - tst-ptrguard1-static tst-stackguard1-static
> + tst-ptrguard1-static tst-stackguard1-static \
> + tst-tlsifunc-static
>
> tests := tst-tls9 tst-leaks1 \
> tst-array1 tst-array2 tst-array3 tst-array4 tst-array5 \
> tst-auxv
> -tests-internal := tst-tls1 tst-tls2 $(tests-static-internal)
> +tests-internal := tst-tls1 tst-tls2 tst-tlsifunc $(tests-static-internal)
I'd expect these additions to need to be conditional on IFUNC support
(ifneq (no,$(multi-arch)), as already used in elf/Makefile) to avoid
problems on architectures lacking such support.
On 07/08/2017 02:31 PM, Tulio Magno Quites Machado Filho wrote:
> Changes since version 1:
>
> - Added a testcase. This is now validating both statically and
> dynamically linked executables.
> - Fixed an issue in the $(foreach ..) in sysdeps/powerpc/powerpc64le/Makefile.
> - Added a comment to csu/libc-start.c
> - Added a comment to csu/libc-tls.c
My initial audit showed that no early loader usage of memcpy and memmove would
call IFUNC resolvers, and so this change was safe.
However, HJ is opposed to this change specifically for this reason because he
has optimizations to do what we said is not happening e.g. early IFUNC usage.
Therefore I see no way forward but to split the IFUNC initialization into two
pieces, something like:
setup_irel();
__libc_setup_tls ();
apply_irel();
Where ppc64le can do *everything* in setup_irel(); and nothing in apply_irel();
as a *temporary* bandaid for glibc 2.26.
In the future we need to discuss in more detail exactly what IFUNC's are allowed
and not allowed to do, and if we argue that they can use TLS then we will need
to adjust our infrastructure to setup TLS before the IFUNCs are used, and that
may mean doing a bootstrap/reapplication of IFUNC relocations along the same
vein as Florian was working on during the last release cycle.
We should all acknowledge the following:
* It is important that static and dynamic applications behaviour is as close to
similar as possible. This reduces problems for developers.
* We should strive to reduce per-architecture IFUNC differences since this again
is a complication for developers.
With these things in mind we know that making this fix for ppc64le is a temporary
solution that we need to backout once we have better IFUNC infrastructure.
Nix <nix@esperi.org.uk> writes:
> On 8 Jul 2017, Tulio Magno Quites Machado Filho said this:
>
>> diff --git a/csu/libc-start.c b/csu/libc-start.c
>> index c2dd159..84b7f99 100644
>> --- a/csu/libc-start.c
>> +++ b/csu/libc-start.c
>> @@ -188,12 +188,15 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>>
>> ARCH_INIT_CPU_FEATURES ();
>>
>> - /* Perform IREL{,A} relocations. */
>> - apply_irel ();
>> -
>> /* The stack guard goes into the TCB, so initialize it early. */
>> __libc_setup_tls ();
>>
>> + /* Perform IREL{,A} relocations.
>> + Note: the relocations must happen after TLS initialization so that
>> + IFUNC resolvers can benefit from thread-local storage, e.g. powerpc's
>> + hwcap and platform fields available in the TCB. */
>> + apply_irel ();
>> +
>> /* Set up the stack checker's canary. */
>> uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
>> # ifdef THREAD_SET_STACK_GUARD
>
> Note: I originally moved this above __libc_setup_tls() so that IFUNCs
> could be stack-protected on statically-linked applications, but
> subsequently found that this caused trouble with dynamically-linked
> programs, so ended up de-protecting all the IFUNC resolvers anyway.
> (See commit 003a27e8195470f470f4d9384ca70d4e9fc8bd1b.)
>
> If this passes make check with --enable-stack-protector=all, I'm happy
> with it.
Tested and confirmed no new failures with --enable-stack-protector=all.
By the way, I noticed 2 unrelated failures:
https://sourceware.org/bugzilla/show_bug.cgi?id=21744
https://sourceware.org/bugzilla/show_bug.cgi?id=21745
On 10 Jul 2017, Tulio Magno Quites Machado Filho stated:
> By the way, I noticed 2 unrelated failures:
> https://sourceware.org/bugzilla/show_bug.cgi?id=21744
I'll have a look at this one soon, if nobody else gets to it. I wonder
if it's spotting an actual overflow? (You'd expect more output in that
case. So it's probably something else that needs stack-protection
disabling.)
> https://sourceware.org/bugzilla/show_bug.cgi?id=21745
__stack_chk_fail should be a hidden symbol in libc.so. Commit
524a8ef2ad76af8ac049293d993a1856b0d888fb is apparently not working for
you: I wonder why not? (Adhermerval Cc:ed because he knows more about
what's going on here than I do. :) )
Joseph Myers <joseph@codesourcery.com> writes:
> On Sat, 8 Jul 2017, Tulio Magno Quites Machado Filho wrote:
>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 201b328..b603e89 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -147,12 +147,13 @@ tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \
>> tst-tlsalign-static tst-tlsalign-extern-static \
>> tst-linkall-static tst-env-setuid tst-env-setuid-tunables
>> tests-static-internal := tst-tls1-static tst-tls2-static \
>> - tst-ptrguard1-static tst-stackguard1-static
>> + tst-ptrguard1-static tst-stackguard1-static \
>> + tst-tlsifunc-static
>>
>> tests := tst-tls9 tst-leaks1 \
>> tst-array1 tst-array2 tst-array3 tst-array4 tst-array5 \
>> tst-auxv
>> -tests-internal := tst-tls1 tst-tls2 $(tests-static-internal)
>> +tests-internal := tst-tls1 tst-tls2 tst-tlsifunc $(tests-static-internal)
>
> I'd expect these additions to need to be conditional on IFUNC support
> (ifneq (no,$(multi-arch)), as already used in elf/Makefile) to avoid
> problems on architectures lacking such support.
Fixed. Thanks!
On 10 Jul 2017, nix@esperi.org.uk verbalised:
> On 10 Jul 2017, Tulio Magno Quites Machado Filho stated:
>> By the way, I noticed 2 unrelated failures:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=21744
>
> I'll have a look at this one soon, if nobody else gets to it. I wonder
> if it's spotting an actual overflow? (You'd expect more output in that
> case. So it's probably something else that needs stack-protection
> disabling.)
... so as I reported in the bug, this does not happen for me with
today's master on x86_64, even with tunables explicitly enabled. A
backtrace would be nice (among other things, it might help me reproduce
it, as well as fix it).
@@ -188,12 +188,15 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
ARCH_INIT_CPU_FEATURES ();
- /* Perform IREL{,A} relocations. */
- apply_irel ();
-
/* The stack guard goes into the TCB, so initialize it early. */
__libc_setup_tls ();
+ /* Perform IREL{,A} relocations.
+ Note: the relocations must happen after TLS initialization so that
+ IFUNC resolvers can benefit from thread-local storage, e.g. powerpc's
+ hwcap and platform fields available in the TCB. */
+ apply_irel ();
+
/* Set up the stack checker's canary. */
uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
# ifdef THREAD_SET_STACK_GUARD
@@ -224,7 +227,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
__pointer_chk_guard_local = pointer_chk_guard;
# endif
-#endif
+#endif /* !SHARED */
/* Register the destructor of the dynamic linker if there is any. */
if (__glibc_likely (rtld_fini != NULL))
@@ -101,6 +101,8 @@ init_static_tls (size_t memsz, size_t align)
GL(dl_tls_static_nelem) = GL(dl_tls_max_dtv_idx);
}
+/* Note: IREL{,A} relocations happen after TLS setup. __libc_setup_tls has
+ to guarantee that it won't use STT_GNU_IFUNC. */
void
__libc_setup_tls (void)
{
@@ -147,12 +147,13 @@ tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \
tst-tlsalign-static tst-tlsalign-extern-static \
tst-linkall-static tst-env-setuid tst-env-setuid-tunables
tests-static-internal := tst-tls1-static tst-tls2-static \
- tst-ptrguard1-static tst-stackguard1-static
+ tst-ptrguard1-static tst-stackguard1-static \
+ tst-tlsifunc-static
tests := tst-tls9 tst-leaks1 \
tst-array1 tst-array2 tst-array3 tst-array4 tst-array5 \
tst-auxv
-tests-internal := tst-tls1 tst-tls2 $(tests-static-internal)
+tests-internal := tst-tls1 tst-tls2 tst-tlsifunc $(tests-static-internal)
tests-static := $(tests-static-normal) $(tests-static-internal)
ifeq (yes,$(build-shared))
new file mode 100644
@@ -0,0 +1,19 @@
+/* Test if an executable can read from the TLS from an STT_GNU_IFUNC resolver.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include "tst-tlsifunc.c"
new file mode 100644
@@ -0,0 +1,66 @@
+/* Test if an executable can read from the TLS from an STT_GNU_IFUNC resolver.
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <libc-symbols.h>
+#include "tls-macros.h"
+
+__thread int bar;
+static int * bar_ptr = NULL;
+
+int foo (void);
+
+void
+init_foo (void)
+{
+ bar_ptr = TLS_GD (bar);
+}
+
+int
+my_foo (void)
+{
+ printf ("&bar = %p and bar_ptr = %p.\n", &bar, bar_ptr);
+ return bar_ptr != NULL;
+}
+
+__ifunc (foo, foo, my_foo, void, init_foo);
+
+static int
+do_test (void)
+{
+ int ret = 0;
+
+ if (foo ())
+ printf ("PASS: IFUNC resolver called once.\n");
+ else {
+ printf ("FAIL: IFUNC resolver not called once.\n");
+ ret = 1;
+ }
+
+ if (&bar == bar_ptr)
+ printf ("PASS: Address read from IFUNC resolver is correct.\n");
+ else {
+ printf ("FAIL: Address read from IFUNC resolver is incorrect.\n");
+ ret = 1;
+ }
+
+ return ret;
+}
+
+#include <support/test-driver.c>
@@ -1,6 +1,11 @@
# When building float128 we need to ensure -mfloat128 is
# passed to all such object files.
+# libgcc requires __tcb_parse_hwcap_and_convert_at_platform when built with
+# a binary128 type. That symbol is provided by the loader on dynamically
+# linked executables, forcing to link the loader after libgcc link.
+f128-loader-link = $(as-needed) $(elf-objpfx)ld.so $(no-as-needed)
+
ifeq ($(subdir),math)
# sqrtf128 requires emulation before POWER9.
CPPFLAGS += -I../soft-fp
@@ -11,6 +16,8 @@ $(foreach suf,$(all-object-suffixes),%f128_r$(suf)): CFLAGS += -mfloat128
$(objpfx)test-float128%.o $(objpfx)test-float128%.os: CFLAGS += -mfloat128
$(objpfx)test-ifloat128%.o $(objpfx)test-ifloat128%.os: CFLAGS += -mfloat128
CFLAGS-libm-test-support-float128.c += -mfloat128
+$(objpfx)test-float128% $(objpfx)test-ifloat128%: \
+ gnulib-tests += $(f128-loader-link)
endif
# Append flags to string <-> _Float128 routines.
@@ -24,6 +31,9 @@ CFLAGS-tst-strtod6.c += -mfloat128
CFLAGS-tst-strfrom.c += -mfloat128
CFLAGS-tst-strfrom-locale.c += -mfloat128
CFLAGS-strfrom-skeleton.c += -mfloat128
+$(foreach test,bug-strtod bug-strtod2 bug-strtod2 tst-strtod-round \
+tst-wcstod-round tst-strtod6 tst-strrom tst-strfrom-locale \
+strfrom-skeleton,$(objpfx)$(test)): gnulib-tests += $(f128-loader-link)
# When building glibc with support for _Float128, the powers of ten tables in
# fpioconst.c and in the string conversion functions must be extended.