* [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
@ 2020-12-28 14:11 H.J. Lu
2021-01-04 18:47 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2020-12-28 14:11 UTC (permalink / raw)
To: libc-alpha
Calling an IFUNC function defined in unrelocated executable may also
lead to segfault. Issue an error message when calling IFUNC function
defined in the unrelocated executable from a shared library.
---
sysdeps/i386/dl-machine.h | 15 ++++++++++-----
sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index fea9e579ec..dedda484ba 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
{
# ifndef RTLD_BOOTSTRAP
if (sym_map != map
- && sym_map->l_type != lt_executable
&& !sym_map->l_relocated)
{
const char *strtab
= (const char *) D_PTR (map, l_info[DT_STRTAB]);
- _dl_error_printf ("\
+ if (sym_map->l_type == lt_executable)
+ _dl_error_printf ("\
+%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
+ RTLD_PROGNAME, strtab + refsym->st_name,
+ map->l_name);
+ else
+ _dl_error_printf ("\
%s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
- RTLD_PROGNAME, map->l_name,
- sym_map->l_name,
- strtab + refsym->st_name);
+ RTLD_PROGNAME, map->l_name,
+ sym_map->l_name,
+ strtab + refsym->st_name);
}
# endif
value = ((Elf32_Addr (*) (void)) value) ();
diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index bb93c7c6ab..fc847f4bc2 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
{
# ifndef RTLD_BOOTSTRAP
if (sym_map != map
- && sym_map->l_type != lt_executable
&& !sym_map->l_relocated)
{
const char *strtab
= (const char *) D_PTR (map, l_info[DT_STRTAB]);
- _dl_error_printf ("\
+ if (sym_map->l_type == lt_executable)
+ _dl_error_printf ("\
+%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
+ RTLD_PROGNAME, strtab + refsym->st_name,
+ map->l_name);
+ else
+ _dl_error_printf ("\
%s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
- RTLD_PROGNAME, map->l_name,
- sym_map->l_name,
- strtab + refsym->st_name);
+ RTLD_PROGNAME, map->l_name,
+ sym_map->l_name,
+ strtab + refsym->st_name);
}
# endif
value = ((ElfW(Addr) (*) (void)) value) ();
--
2.29.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2020-12-28 14:11 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019] H.J. Lu
@ 2021-01-04 18:47 ` Carlos O'Donell
2021-01-04 19:34 ` V2 " H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2021-01-04 18:47 UTC (permalink / raw)
To: H.J. Lu, libc-alpha
On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
> Calling an IFUNC function defined in unrelocated executable may also
> lead to segfault. Issue an error message when calling IFUNC function
> defined in the unrelocated executable from a shared library.
The logic here makes sense, but we need a stronger error message.
Please review my understanding and suggested error message.
Looking forward to v2.
> ---
> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
> 2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> index fea9e579ec..dedda484ba 100644
> --- a/sysdeps/i386/dl-machine.h
> +++ b/sysdeps/i386/dl-machine.h
> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> {
> # ifndef RTLD_BOOTSTRAP
OK. Logic is in the correct place in dl-machine.h for i386.
> if (sym_map != map
> - && sym_map->l_type != lt_executable
> && !sym_map->l_relocated)
> {
> const char *strtab
> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> - _dl_error_printf ("\
> + if (sym_map->l_type == lt_executable)
> + _dl_error_printf ("\
> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> + RTLD_PROGNAME, strtab + refsym->st_name,
> + map->l_name);
> + else
> + _dl_error_printf ("\
> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> - RTLD_PROGNAME, map->l_name,
> - sym_map->l_name,
> - strtab + refsym->st_name);
> + RTLD_PROGNAME, map->l_name,
> + sym_map->l_name,
> + strtab + refsym->st_name);
> }
> # endif
> value = ((Elf32_Addr (*) (void)) value) ();
> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> index bb93c7c6ab..fc847f4bc2 100644
> --- a/sysdeps/x86_64/dl-machine.h
> +++ b/sysdeps/x86_64/dl-machine.h
> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> {
> # ifndef RTLD_BOOTSTRAP
OK. Logic is in the correct place in dl-machine.h for x86_64.
> if (sym_map != map
> - && sym_map->l_type != lt_executable
> && !sym_map->l_relocated)
> {
> const char *strtab
> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> - _dl_error_printf ("\
> + if (sym_map->l_type == lt_executable)
> + _dl_error_printf ("\
> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
The message should explain the error
e.g. "Such and such *must not* reference such and such."
Or the message should explain how to fix the error (as the other does)
e.g. "Such and such must be relinked with such and such."
We have made this a hard error. An executable with immediate binding
may not define an IFUNC resolver and implementation that is used from
a shared library since it creates an ordering issue with the dependent
libraries that use the resolution of the symbol i.e. you must initialize
the executable but to do that you must initialize the libraries, but to
do that you must initialize the executable etc. etc.
In which case the error message could be:
"%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
and creates an unsatisfiable circular dependency."
Note: Use '' quotes not `' since the GNU Coding standards have changed.
https://www.gnu.org/prep/standards/standards.html#Quote-Characters
> + RTLD_PROGNAME, strtab + refsym->st_name,
> + map->l_name);
> + else
> + _dl_error_printf ("\
> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> - RTLD_PROGNAME, map->l_name,
> - sym_map->l_name,
> - strtab + refsym->st_name);
> + RTLD_PROGNAME, map->l_name,
> + sym_map->l_name,
> + strtab + refsym->st_name);
> }
> # endif
> value = ((ElfW(Addr) (*) (void)) value) ();
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 18:47 ` Carlos O'Donell
@ 2021-01-04 19:34 ` H.J. Lu
2021-01-04 19:50 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2021-01-04 19:34 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
[-- Attachment #1: Type: text/plain, Size: 4899 bytes --]
On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
> > Calling an IFUNC function defined in unrelocated executable may also
> > lead to segfault. Issue an error message when calling IFUNC function
> > defined in the unrelocated executable from a shared library.
>
> The logic here makes sense, but we need a stronger error message.
>
> Please review my understanding and suggested error message.
>
> Looking forward to v2.
>
> > ---
> > sysdeps/i386/dl-machine.h | 15 ++++++++++-----
> > sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> > index fea9e579ec..dedda484ba 100644
> > --- a/sysdeps/i386/dl-machine.h
> > +++ b/sysdeps/i386/dl-machine.h
> > @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> > {
> > # ifndef RTLD_BOOTSTRAP
>
> OK. Logic is in the correct place in dl-machine.h for i386.
>
> > if (sym_map != map
> > - && sym_map->l_type != lt_executable
> > && !sym_map->l_relocated)
> > {
> > const char *strtab
> > = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> > - _dl_error_printf ("\
> > + if (sym_map->l_type == lt_executable)
> > + _dl_error_printf ("\
> > +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> > + RTLD_PROGNAME, strtab + refsym->st_name,
> > + map->l_name);
> > + else
> > + _dl_error_printf ("\
> > %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> > - RTLD_PROGNAME, map->l_name,
> > - sym_map->l_name,
> > - strtab + refsym->st_name);
> > + RTLD_PROGNAME, map->l_name,
> > + sym_map->l_name,
> > + strtab + refsym->st_name);
> > }
> > # endif
> > value = ((Elf32_Addr (*) (void)) value) ();
> > diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> > index bb93c7c6ab..fc847f4bc2 100644
> > --- a/sysdeps/x86_64/dl-machine.h
> > +++ b/sysdeps/x86_64/dl-machine.h
> > @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> > {
> > # ifndef RTLD_BOOTSTRAP
>
> OK. Logic is in the correct place in dl-machine.h for x86_64.
>
> > if (sym_map != map
> > - && sym_map->l_type != lt_executable
> > && !sym_map->l_relocated)
> > {
> > const char *strtab
> > = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> > - _dl_error_printf ("\
> > + if (sym_map->l_type == lt_executable)
> > + _dl_error_printf ("\
> > +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>
> The message should explain the error
> e.g. "Such and such *must not* reference such and such."
>
> Or the message should explain how to fix the error (as the other does)
> e.g. "Such and such must be relinked with such and such."
>
> We have made this a hard error. An executable with immediate binding
> may not define an IFUNC resolver and implementation that is used from
> a shared library since it creates an ordering issue with the dependent
> libraries that use the resolution of the symbol i.e. you must initialize
> the executable but to do that you must initialize the libraries, but to
> do that you must initialize the executable etc. etc.
>
> In which case the error message could be:
>
> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
> and creates an unsatisfiable circular dependency."
Fixed.
> Note: Use '' quotes not `' since the GNU Coding standards have changed.
> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
>
> > + RTLD_PROGNAME, strtab + refsym->st_name,
> > + map->l_name);
> > + else
> > + _dl_error_printf ("\
> > %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> > - RTLD_PROGNAME, map->l_name,
> > - sym_map->l_name,
> > - strtab + refsym->st_name);
> > + RTLD_PROGNAME, map->l_name,
> > + sym_map->l_name,
> > + strtab + refsym->st_name);
> > }
> > # endif
> > value = ((ElfW(Addr) (*) (void)) value) ();
> >
>
>
Here is the updated patch. Changes from V1:
1. Update the error message based on feedback from Carlos.
2. Make the error fatal instead of segfault later.
OK for master?
Thanks.
--
H.J.
[-- Attachment #2: 0001-x86-Check-IFUNC-definition-in-unrelocated-executable.patch --]
[-- Type: text/x-patch, Size: 2815 bytes --]
From 85fd4f35471038f734532ee902fd0b99a9aa16ba Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 28 Dec 2020 05:28:49 -0800
Subject: [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ
#20019]
Calling an IFUNC function defined in unrelocated executable also leads to
segfault. Issue a fatal error message when calling IFUNC function defined
in the unrelocated executable from a shared library.
---
sysdeps/i386/dl-machine.h | 16 +++++++++++-----
sysdeps/x86_64/dl-machine.h | 16 +++++++++++-----
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 50960605e6..23e9cc3bfb 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -337,16 +337,22 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
{
# ifndef RTLD_BOOTSTRAP
if (sym_map != map
- && sym_map->l_type != lt_executable
&& !sym_map->l_relocated)
{
const char *strtab
= (const char *) D_PTR (map, l_info[DT_STRTAB]);
- _dl_error_printf ("\
+ if (sym_map->l_type == lt_executable)
+ _dl_fatal_printf ("\
+%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
+and creates an unsatisfiable circular dependency.\n",
+ RTLD_PROGNAME, strtab + refsym->st_name,
+ map->l_name);
+ else
+ _dl_error_printf ("\
%s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
- RTLD_PROGNAME, map->l_name,
- sym_map->l_name,
- strtab + refsym->st_name);
+ RTLD_PROGNAME, map->l_name,
+ sym_map->l_name,
+ strtab + refsym->st_name);
}
# endif
value = ((Elf32_Addr (*) (void)) value) ();
diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index f582be5320..103eee6c3f 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -314,16 +314,22 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
{
# ifndef RTLD_BOOTSTRAP
if (sym_map != map
- && sym_map->l_type != lt_executable
&& !sym_map->l_relocated)
{
const char *strtab
= (const char *) D_PTR (map, l_info[DT_STRTAB]);
- _dl_error_printf ("\
+ if (sym_map->l_type == lt_executable)
+ _dl_fatal_printf ("\
+%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
+and creates an unsatisfiable circular dependency.\n",
+ RTLD_PROGNAME, strtab + refsym->st_name,
+ map->l_name);
+ else
+ _dl_error_printf ("\
%s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
- RTLD_PROGNAME, map->l_name,
- sym_map->l_name,
- strtab + refsym->st_name);
+ RTLD_PROGNAME, map->l_name,
+ sym_map->l_name,
+ strtab + refsym->st_name);
}
# endif
value = ((ElfW(Addr) (*) (void)) value) ();
--
2.29.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 19:34 ` V2 " H.J. Lu
@ 2021-01-04 19:50 ` Carlos O'Donell
2021-01-04 19:59 ` H.J. Lu
2021-01-04 20:44 ` H.J. Lu
0 siblings, 2 replies; 14+ messages in thread
From: Carlos O'Donell @ 2021-01-04 19:50 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
On 1/4/21 2:34 PM, H.J. Lu wrote:
> On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
>>> Calling an IFUNC function defined in unrelocated executable may also
>>> lead to segfault. Issue an error message when calling IFUNC function
>>> defined in the unrelocated executable from a shared library.
>>
>> The logic here makes sense, but we need a stronger error message.
>>
>> Please review my understanding and suggested error message.
>>
>> Looking forward to v2.
>>
>>> ---
>>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
>>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
>>> 2 files changed, 20 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
>>> index fea9e579ec..dedda484ba 100644
>>> --- a/sysdeps/i386/dl-machine.h
>>> +++ b/sysdeps/i386/dl-machine.h
>>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
>>> {
>>> # ifndef RTLD_BOOTSTRAP
>>
>> OK. Logic is in the correct place in dl-machine.h for i386.
>>
>>> if (sym_map != map
>>> - && sym_map->l_type != lt_executable
>>> && !sym_map->l_relocated)
>>> {
>>> const char *strtab
>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>> - _dl_error_printf ("\
>>> + if (sym_map->l_type == lt_executable)
>>> + _dl_error_printf ("\
>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>> + RTLD_PROGNAME, strtab + refsym->st_name,
>>> + map->l_name);
>>> + else
>>> + _dl_error_printf ("\
>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>> - RTLD_PROGNAME, map->l_name,
>>> - sym_map->l_name,
>>> - strtab + refsym->st_name);
>>> + RTLD_PROGNAME, map->l_name,
>>> + sym_map->l_name,
>>> + strtab + refsym->st_name);
>>> }
>>> # endif
>>> value = ((Elf32_Addr (*) (void)) value) ();
>>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
>>> index bb93c7c6ab..fc847f4bc2 100644
>>> --- a/sysdeps/x86_64/dl-machine.h
>>> +++ b/sysdeps/x86_64/dl-machine.h
>>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>>> {
>>> # ifndef RTLD_BOOTSTRAP
>>
>> OK. Logic is in the correct place in dl-machine.h for x86_64.
>>
>>> if (sym_map != map
>>> - && sym_map->l_type != lt_executable
>>> && !sym_map->l_relocated)
>>> {
>>> const char *strtab
>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>> - _dl_error_printf ("\
>>> + if (sym_map->l_type == lt_executable)
>>> + _dl_error_printf ("\
>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>
>> The message should explain the error
>> e.g. "Such and such *must not* reference such and such."
>>
>> Or the message should explain how to fix the error (as the other does)
>> e.g. "Such and such must be relinked with such and such."
>>
>> We have made this a hard error. An executable with immediate binding
>> may not define an IFUNC resolver and implementation that is used from
>> a shared library since it creates an ordering issue with the dependent
>> libraries that use the resolution of the symbol i.e. you must initialize
>> the executable but to do that you must initialize the libraries, but to
>> do that you must initialize the executable etc. etc.
>>
>> In which case the error message could be:
>>
>> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
>> and creates an unsatisfiable circular dependency."
>
> Fixed.
>
>> Note: Use '' quotes not `' since the GNU Coding standards have changed.
>> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
>>
>>> + RTLD_PROGNAME, strtab + refsym->st_name,
>>> + map->l_name);
>>> + else
>>> + _dl_error_printf ("\
>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>> - RTLD_PROGNAME, map->l_name,
>>> - sym_map->l_name,
>>> - strtab + refsym->st_name);
>>> + RTLD_PROGNAME, map->l_name,
>>> + sym_map->l_name,
>>> + strtab + refsym->st_name);
>>> }
>>> # endif
>>> value = ((ElfW(Addr) (*) (void)) value) ();
>>>
>>
>>
>
> Here is the updated patch. Changes from V1:
>
> 1. Update the error message based on feedback from Carlos.
> 2. Make the error fatal instead of segfault later.
>
> OK for master?
Could binutils have given the user a better warnings?
OK for master.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> From 85fd4f35471038f734532ee902fd0b99a9aa16ba Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Mon, 28 Dec 2020 05:28:49 -0800
> Subject: [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ
> #20019]
>
> Calling an IFUNC function defined in unrelocated executable also leads to
> segfault. Issue a fatal error message when calling IFUNC function defined
> in the unrelocated executable from a shared library.
> ---
> sysdeps/i386/dl-machine.h | 16 +++++++++++-----
> sysdeps/x86_64/dl-machine.h | 16 +++++++++++-----
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> index 50960605e6..23e9cc3bfb 100644
> --- a/sysdeps/i386/dl-machine.h
> +++ b/sysdeps/i386/dl-machine.h
> @@ -337,16 +337,22 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> {
> # ifndef RTLD_BOOTSTRAP
> if (sym_map != map
> - && sym_map->l_type != lt_executable
> && !sym_map->l_relocated)
> {
> const char *strtab
> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> - _dl_error_printf ("\
> + if (sym_map->l_type == lt_executable)
> + _dl_fatal_printf ("\
> +%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
> +and creates an unsatisfiable circular dependency.\n",
> + RTLD_PROGNAME, strtab + refsym->st_name,
> + map->l_name);
> + else
> + _dl_error_printf ("\
> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> - RTLD_PROGNAME, map->l_name,
> - sym_map->l_name,
> - strtab + refsym->st_name);
> + RTLD_PROGNAME, map->l_name,
> + sym_map->l_name,
> + strtab + refsym->st_name);
> }
> # endif
> value = ((Elf32_Addr (*) (void)) value) ();
> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> index f582be5320..103eee6c3f 100644
> --- a/sysdeps/x86_64/dl-machine.h
> +++ b/sysdeps/x86_64/dl-machine.h
> @@ -314,16 +314,22 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> {
> # ifndef RTLD_BOOTSTRAP
> if (sym_map != map
> - && sym_map->l_type != lt_executable
> && !sym_map->l_relocated)
> {
> const char *strtab
> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> - _dl_error_printf ("\
> + if (sym_map->l_type == lt_executable)
> + _dl_fatal_printf ("\
> +%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
> +and creates an unsatisfiable circular dependency.\n",
> + RTLD_PROGNAME, strtab + refsym->st_name,
> + map->l_name);
> + else
> + _dl_error_printf ("\
> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> - RTLD_PROGNAME, map->l_name,
> - sym_map->l_name,
> - strtab + refsym->st_name);
> + RTLD_PROGNAME, map->l_name,
> + sym_map->l_name,
> + strtab + refsym->st_name);
> }
> # endif
> value = ((ElfW(Addr) (*) (void)) value) ();
> --
> 2.29.2
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 19:50 ` Carlos O'Donell
@ 2021-01-04 19:59 ` H.J. Lu
2021-01-04 22:57 ` H.J. Lu
2021-01-04 20:44 ` H.J. Lu
1 sibling, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2021-01-04 19:59 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 1/4/21 2:34 PM, H.J. Lu wrote:
> > On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> >> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
> >>> Calling an IFUNC function defined in unrelocated executable may also
> >>> lead to segfault. Issue an error message when calling IFUNC function
> >>> defined in the unrelocated executable from a shared library.
> >>
> >> The logic here makes sense, but we need a stronger error message.
> >>
> >> Please review my understanding and suggested error message.
> >>
> >> Looking forward to v2.
> >>
> >>> ---
> >>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
> >>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
> >>> 2 files changed, 20 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> >>> index fea9e579ec..dedda484ba 100644
> >>> --- a/sysdeps/i386/dl-machine.h
> >>> +++ b/sysdeps/i386/dl-machine.h
> >>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> >>> {
> >>> # ifndef RTLD_BOOTSTRAP
> >>
> >> OK. Logic is in the correct place in dl-machine.h for i386.
> >>
> >>> if (sym_map != map
> >>> - && sym_map->l_type != lt_executable
> >>> && !sym_map->l_relocated)
> >>> {
> >>> const char *strtab
> >>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> >>> - _dl_error_printf ("\
> >>> + if (sym_map->l_type == lt_executable)
> >>> + _dl_error_printf ("\
> >>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> >>> + RTLD_PROGNAME, strtab + refsym->st_name,
> >>> + map->l_name);
> >>> + else
> >>> + _dl_error_printf ("\
> >>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> >>> - RTLD_PROGNAME, map->l_name,
> >>> - sym_map->l_name,
> >>> - strtab + refsym->st_name);
> >>> + RTLD_PROGNAME, map->l_name,
> >>> + sym_map->l_name,
> >>> + strtab + refsym->st_name);
> >>> }
> >>> # endif
> >>> value = ((Elf32_Addr (*) (void)) value) ();
> >>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> >>> index bb93c7c6ab..fc847f4bc2 100644
> >>> --- a/sysdeps/x86_64/dl-machine.h
> >>> +++ b/sysdeps/x86_64/dl-machine.h
> >>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> >>> {
> >>> # ifndef RTLD_BOOTSTRAP
> >>
> >> OK. Logic is in the correct place in dl-machine.h for x86_64.
> >>
> >>> if (sym_map != map
> >>> - && sym_map->l_type != lt_executable
> >>> && !sym_map->l_relocated)
> >>> {
> >>> const char *strtab
> >>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> >>> - _dl_error_printf ("\
> >>> + if (sym_map->l_type == lt_executable)
> >>> + _dl_error_printf ("\
> >>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> >>
> >> The message should explain the error
> >> e.g. "Such and such *must not* reference such and such."
> >>
> >> Or the message should explain how to fix the error (as the other does)
> >> e.g. "Such and such must be relinked with such and such."
> >>
> >> We have made this a hard error. An executable with immediate binding
> >> may not define an IFUNC resolver and implementation that is used from
> >> a shared library since it creates an ordering issue with the dependent
> >> libraries that use the resolution of the symbol i.e. you must initialize
> >> the executable but to do that you must initialize the libraries, but to
> >> do that you must initialize the executable etc. etc.
> >>
> >> In which case the error message could be:
> >>
> >> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
> >> and creates an unsatisfiable circular dependency."
> >
> > Fixed.
> >
> >> Note: Use '' quotes not `' since the GNU Coding standards have changed.
> >> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
> >>
> >>> + RTLD_PROGNAME, strtab + refsym->st_name,
> >>> + map->l_name);
> >>> + else
> >>> + _dl_error_printf ("\
> >>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> >>> - RTLD_PROGNAME, map->l_name,
> >>> - sym_map->l_name,
> >>> - strtab + refsym->st_name);
> >>> + RTLD_PROGNAME, map->l_name,
> >>> + sym_map->l_name,
> >>> + strtab + refsym->st_name);
> >>> }
> >>> # endif
> >>> value = ((ElfW(Addr) (*) (void)) value) ();
> >>>
> >>
> >>
> >
> > Here is the updated patch. Changes from V1:
> >
> > 1. Update the error message based on feedback from Carlos.
> > 2. Make the error fatal instead of segfault later.
> >
> > OK for master?
>
> Could binutils have given the user a better warnings?
I will take a look.
> OK for master.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > From 85fd4f35471038f734532ee902fd0b99a9aa16ba Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Mon, 28 Dec 2020 05:28:49 -0800
> > Subject: [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ
> > #20019]
> >
> > Calling an IFUNC function defined in unrelocated executable also leads to
> > segfault. Issue a fatal error message when calling IFUNC function defined
> > in the unrelocated executable from a shared library.
> > ---
> > sysdeps/i386/dl-machine.h | 16 +++++++++++-----
> > sysdeps/x86_64/dl-machine.h | 16 +++++++++++-----
> > 2 files changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> > index 50960605e6..23e9cc3bfb 100644
> > --- a/sysdeps/i386/dl-machine.h
> > +++ b/sysdeps/i386/dl-machine.h
> > @@ -337,16 +337,22 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> > {
> > # ifndef RTLD_BOOTSTRAP
> > if (sym_map != map
> > - && sym_map->l_type != lt_executable
> > && !sym_map->l_relocated)
> > {
> > const char *strtab
> > = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> > - _dl_error_printf ("\
> > + if (sym_map->l_type == lt_executable)
> > + _dl_fatal_printf ("\
> > +%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
> > +and creates an unsatisfiable circular dependency.\n",
> > + RTLD_PROGNAME, strtab + refsym->st_name,
> > + map->l_name);
> > + else
> > + _dl_error_printf ("\
> > %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> > - RTLD_PROGNAME, map->l_name,
> > - sym_map->l_name,
> > - strtab + refsym->st_name);
> > + RTLD_PROGNAME, map->l_name,
> > + sym_map->l_name,
> > + strtab + refsym->st_name);
> > }
> > # endif
> > value = ((Elf32_Addr (*) (void)) value) ();
> > diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> > index f582be5320..103eee6c3f 100644
> > --- a/sysdeps/x86_64/dl-machine.h
> > +++ b/sysdeps/x86_64/dl-machine.h
> > @@ -314,16 +314,22 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> > {
> > # ifndef RTLD_BOOTSTRAP
> > if (sym_map != map
> > - && sym_map->l_type != lt_executable
> > && !sym_map->l_relocated)
> > {
> > const char *strtab
> > = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> > - _dl_error_printf ("\
> > + if (sym_map->l_type == lt_executable)
> > + _dl_fatal_printf ("\
> > +%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable \
> > +and creates an unsatisfiable circular dependency.\n",
> > + RTLD_PROGNAME, strtab + refsym->st_name,
> > + map->l_name);
> > + else
> > + _dl_error_printf ("\
> > %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> > - RTLD_PROGNAME, map->l_name,
> > - sym_map->l_name,
> > - strtab + refsym->st_name);
> > + RTLD_PROGNAME, map->l_name,
> > + sym_map->l_name,
> > + strtab + refsym->st_name);
> > }
> > # endif
> > value = ((ElfW(Addr) (*) (void)) value) ();
> > --
> > 2.29.2
> >
>
> --
> Cheers,
> Carlos.
>
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 19:50 ` Carlos O'Donell
2021-01-04 19:59 ` H.J. Lu
@ 2021-01-04 20:44 ` H.J. Lu
2021-01-04 21:20 ` Carlos O'Donell
1 sibling, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2021-01-04 20:44 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 1/4/21 2:34 PM, H.J. Lu wrote:
> > On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> >> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
> >>> Calling an IFUNC function defined in unrelocated executable may also
> >>> lead to segfault. Issue an error message when calling IFUNC function
> >>> defined in the unrelocated executable from a shared library.
> >>
> >> The logic here makes sense, but we need a stronger error message.
> >>
> >> Please review my understanding and suggested error message.
> >>
> >> Looking forward to v2.
> >>
> >>> ---
> >>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
> >>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
> >>> 2 files changed, 20 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> >>> index fea9e579ec..dedda484ba 100644
> >>> --- a/sysdeps/i386/dl-machine.h
> >>> +++ b/sysdeps/i386/dl-machine.h
> >>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> >>> {
> >>> # ifndef RTLD_BOOTSTRAP
> >>
> >> OK. Logic is in the correct place in dl-machine.h for i386.
> >>
> >>> if (sym_map != map
> >>> - && sym_map->l_type != lt_executable
> >>> && !sym_map->l_relocated)
> >>> {
> >>> const char *strtab
> >>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> >>> - _dl_error_printf ("\
> >>> + if (sym_map->l_type == lt_executable)
> >>> + _dl_error_printf ("\
> >>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> >>> + RTLD_PROGNAME, strtab + refsym->st_name,
> >>> + map->l_name);
> >>> + else
> >>> + _dl_error_printf ("\
> >>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> >>> - RTLD_PROGNAME, map->l_name,
> >>> - sym_map->l_name,
> >>> - strtab + refsym->st_name);
> >>> + RTLD_PROGNAME, map->l_name,
> >>> + sym_map->l_name,
> >>> + strtab + refsym->st_name);
> >>> }
> >>> # endif
> >>> value = ((Elf32_Addr (*) (void)) value) ();
> >>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> >>> index bb93c7c6ab..fc847f4bc2 100644
> >>> --- a/sysdeps/x86_64/dl-machine.h
> >>> +++ b/sysdeps/x86_64/dl-machine.h
> >>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> >>> {
> >>> # ifndef RTLD_BOOTSTRAP
> >>
> >> OK. Logic is in the correct place in dl-machine.h for x86_64.
> >>
> >>> if (sym_map != map
> >>> - && sym_map->l_type != lt_executable
> >>> && !sym_map->l_relocated)
> >>> {
> >>> const char *strtab
> >>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> >>> - _dl_error_printf ("\
> >>> + if (sym_map->l_type == lt_executable)
> >>> + _dl_error_printf ("\
> >>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> >>
> >> The message should explain the error
> >> e.g. "Such and such *must not* reference such and such."
> >>
> >> Or the message should explain how to fix the error (as the other does)
> >> e.g. "Such and such must be relinked with such and such."
> >>
> >> We have made this a hard error. An executable with immediate binding
> >> may not define an IFUNC resolver and implementation that is used from
> >> a shared library since it creates an ordering issue with the dependent
> >> libraries that use the resolution of the symbol i.e. you must initialize
> >> the executable but to do that you must initialize the libraries, but to
> >> do that you must initialize the executable etc. etc.
> >>
> >> In which case the error message could be:
> >>
> >> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
> >> and creates an unsatisfiable circular dependency."
> >
> > Fixed.
> >
> >> Note: Use '' quotes not `' since the GNU Coding standards have changed.
> >> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
> >>
> >>> + RTLD_PROGNAME, strtab + refsym->st_name,
> >>> + map->l_name);
> >>> + else
> >>> + _dl_error_printf ("\
> >>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> >>> - RTLD_PROGNAME, map->l_name,
> >>> - sym_map->l_name,
> >>> - strtab + refsym->st_name);
> >>> + RTLD_PROGNAME, map->l_name,
> >>> + sym_map->l_name,
> >>> + strtab + refsym->st_name);
> >>> }
> >>> # endif
> >>> value = ((ElfW(Addr) (*) (void)) value) ();
> >>>
> >>
> >>
> >
> > Here is the updated patch. Changes from V1:
> >
> > 1. Update the error message based on feedback from Carlos.
> > 2. Make the error fatal instead of segfault later.
> >
> > OK for master?
>
> Could binutils have given the user a better warnings?
>
> OK for master.
>
Now I got
[hjl@gnu-cfl-2 build-x86_64-linux]$ ./elf/ifuncmain6pie --direct
./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in
'/export/build/gnu/tools-build/glibc/build-x86_64-linux/elf/ifuncmod6.so'
is defined in the executable and creates an unsatisfiable circular
dependency.
[hjl@gnu-cfl-2 build-x86_64-linux]$
The message is correct. Should we update the testcase to avoid it?
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 20:44 ` H.J. Lu
@ 2021-01-04 21:20 ` Carlos O'Donell
2021-01-04 22:38 ` [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency " H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2021-01-04 21:20 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
On 1/4/21 3:44 PM, H.J. Lu wrote:
> On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 1/4/21 2:34 PM, H.J. Lu wrote:
>>> On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>>>
>>>> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
>>>>> Calling an IFUNC function defined in unrelocated executable may also
>>>>> lead to segfault. Issue an error message when calling IFUNC function
>>>>> defined in the unrelocated executable from a shared library.
>>>>
>>>> The logic here makes sense, but we need a stronger error message.
>>>>
>>>> Please review my understanding and suggested error message.
>>>>
>>>> Looking forward to v2.
>>>>
>>>>> ---
>>>>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
>>>>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
>>>>> 2 files changed, 20 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
>>>>> index fea9e579ec..dedda484ba 100644
>>>>> --- a/sysdeps/i386/dl-machine.h
>>>>> +++ b/sysdeps/i386/dl-machine.h
>>>>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
>>>>> {
>>>>> # ifndef RTLD_BOOTSTRAP
>>>>
>>>> OK. Logic is in the correct place in dl-machine.h for i386.
>>>>
>>>>> if (sym_map != map
>>>>> - && sym_map->l_type != lt_executable
>>>>> && !sym_map->l_relocated)
>>>>> {
>>>>> const char *strtab
>>>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>>>> - _dl_error_printf ("\
>>>>> + if (sym_map->l_type == lt_executable)
>>>>> + _dl_error_printf ("\
>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>>>> + RTLD_PROGNAME, strtab + refsym->st_name,
>>>>> + map->l_name);
>>>>> + else
>>>>> + _dl_error_printf ("\
>>>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>>>> - RTLD_PROGNAME, map->l_name,
>>>>> - sym_map->l_name,
>>>>> - strtab + refsym->st_name);
>>>>> + RTLD_PROGNAME, map->l_name,
>>>>> + sym_map->l_name,
>>>>> + strtab + refsym->st_name);
>>>>> }
>>>>> # endif
>>>>> value = ((Elf32_Addr (*) (void)) value) ();
>>>>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
>>>>> index bb93c7c6ab..fc847f4bc2 100644
>>>>> --- a/sysdeps/x86_64/dl-machine.h
>>>>> +++ b/sysdeps/x86_64/dl-machine.h
>>>>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>>>>> {
>>>>> # ifndef RTLD_BOOTSTRAP
>>>>
>>>> OK. Logic is in the correct place in dl-machine.h for x86_64.
>>>>
>>>>> if (sym_map != map
>>>>> - && sym_map->l_type != lt_executable
>>>>> && !sym_map->l_relocated)
>>>>> {
>>>>> const char *strtab
>>>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>>>> - _dl_error_printf ("\
>>>>> + if (sym_map->l_type == lt_executable)
>>>>> + _dl_error_printf ("\
>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>>>
>>>> The message should explain the error
>>>> e.g. "Such and such *must not* reference such and such."
>>>>
>>>> Or the message should explain how to fix the error (as the other does)
>>>> e.g. "Such and such must be relinked with such and such."
>>>>
>>>> We have made this a hard error. An executable with immediate binding
>>>> may not define an IFUNC resolver and implementation that is used from
>>>> a shared library since it creates an ordering issue with the dependent
>>>> libraries that use the resolution of the symbol i.e. you must initialize
>>>> the executable but to do that you must initialize the libraries, but to
>>>> do that you must initialize the executable etc. etc.
>>>>
>>>> In which case the error message could be:
>>>>
>>>> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
>>>> and creates an unsatisfiable circular dependency."
>>>
>>> Fixed.
>>>
>>>> Note: Use '' quotes not `' since the GNU Coding standards have changed.
>>>> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
>>>>
>>>>> + RTLD_PROGNAME, strtab + refsym->st_name,
>>>>> + map->l_name);
>>>>> + else
>>>>> + _dl_error_printf ("\
>>>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>>>> - RTLD_PROGNAME, map->l_name,
>>>>> - sym_map->l_name,
>>>>> - strtab + refsym->st_name);
>>>>> + RTLD_PROGNAME, map->l_name,
>>>>> + sym_map->l_name,
>>>>> + strtab + refsym->st_name);
>>>>> }
>>>>> # endif
>>>>> value = ((ElfW(Addr) (*) (void)) value) ();
>>>>>
>>>>
>>>>
>>>
>>> Here is the updated patch. Changes from V1:
>>>
>>> 1. Update the error message based on feedback from Carlos.
>>> 2. Make the error fatal instead of segfault later.
>>>
>>> OK for master?
>>
>> Could binutils have given the user a better warnings?
>>
>> OK for master.
>>
>
> Now I got
>
> [hjl@gnu-cfl-2 build-x86_64-linux]$ ./elf/ifuncmain6pie --direct
> ./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in
> '/export/build/gnu/tools-build/glibc/build-x86_64-linux/elf/ifuncmod6.so'
> is defined in the executable and creates an unsatisfiable circular
> dependency.
> [hjl@gnu-cfl-2 build-x86_64-linux]$
>
> The message is correct. Should we update the testcase to avoid it?
Yes, but it is still possible to support this with lazy binding?
Should ifuncmain6pie be explicitly compiled with -Wl,-z,lazy to
bypass selection from the toolchain?
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency [BZ #20019]
2021-01-04 21:20 ` Carlos O'Donell
@ 2021-01-04 22:38 ` H.J. Lu
2021-01-13 19:43 ` Adhemerval Zanella
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2021-01-04 22:38 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
[-- Attachment #1: Type: text/plain, Size: 780 bytes --]
On Mon, Jan 4, 2021 at 1:20 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
...
> >
> > [hjl@gnu-cfl-2 build-x86_64-linux]$ ./elf/ifuncmain6pie --direct
> > ./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in
> > '/export/build/gnu/tools-build/glibc/build-x86_64-linux/elf/ifuncmod6.so'
> > is defined in the executable and creates an unsatisfiable circular
> > dependency.
> > [hjl@gnu-cfl-2 build-x86_64-linux]$
> >
> > The message is correct. Should we update the testcase to avoid it?
>
> Yes, but it is still possible to support this with lazy binding?
>
> Should ifuncmain6pie be explicitly compiled with -Wl,-z,lazy to
> bypass selection from the toolchain?
The problem is non-JUMP_SLOT relocations. Here is a patch to
remove them. OK for master?
Thanks.
--
H.J.
[-- Attachment #2: 0001-ifuncmain6pie-Remove-the-circular-IFUNC-dependency-B.patch --]
[-- Type: text/x-patch, Size: 2904 bytes --]
From fe3bd3b8d7e6401dc96e2aa59f341d41d1cb4723 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 4 Jan 2021 14:25:39 -0800
Subject: [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency [BZ
#20019]
On x86, ifuncmain6pie failed with:
[hjl@gnu-cfl-2 build-i686-linux]$ ./elf/ifuncmain6pie --direct
./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in '/export/build/gnu/tools-build/glibc-32bit/build-i686-linux/elf/ifuncmod6.so' is defined in the executable and creates an unsatisfiable circular dependency.
[hjl@gnu-cfl-2 build-i686-linux]$ readelf -rW elf/ifuncmod6.so | grep foo
00003ff4 00000706 R_386_GLOB_DAT 0000400c foo_ptr
00003ff8 00000406 R_386_GLOB_DAT 00000000 foo
0000400c 00000401 R_386_32 00000000 foo
[hjl@gnu-cfl-2 build-i686-linux]$
Remove non-JUMP_SLOT relocations against foo in ifuncmod6.so, which
trigger the circular IFUNC dependency, and build ifuncmain6pie with
-Wl,-z,lazy.
---
elf/Makefile | 2 ++
elf/ifuncmain6pie.c | 14 +++-----------
elf/ifuncmod6.c | 8 ++++----
3 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/elf/Makefile b/elf/Makefile
index 543800f4be..c41d11693b 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1403,6 +1403,8 @@ CFLAGS-ifuncmain7pie.c += $(pie-ccflag)
CFLAGS-ifuncmain9pie.c += $(pie-ccflag)
CFLAGS-tst-ifunc-textrel.c += $(pic-ccflag)
+LDFLAGS-ifuncmain6pie = -Wl,-z,lazy
+
$(objpfx)ifuncmain1pie: $(objpfx)ifuncmod1.so
$(objpfx)ifuncmain1staticpie: $(objpfx)ifuncdep1pic.o
$(objpfx)ifuncmain1vispie: $(objpfx)ifuncmod1.so
diff --git a/elf/ifuncmain6pie.c b/elf/ifuncmain6pie.c
index 04faeb86ef..4a01906836 100644
--- a/elf/ifuncmain6pie.c
+++ b/elf/ifuncmain6pie.c
@@ -9,7 +9,6 @@
#include "ifunc-sel.h"
typedef int (*foo_p) (void);
-extern foo_p foo_ptr;
static int
one (void)
@@ -28,20 +27,17 @@ foo_ifunc (void)
}
extern int foo (void);
-extern foo_p get_foo (void);
+extern int call_foo (void);
extern foo_p get_foo_p (void);
-foo_p my_foo_ptr = foo;
+foo_p foo_ptr = foo;
int
main (void)
{
foo_p p;
- p = get_foo ();
- if (p != foo)
- abort ();
- if ((*p) () != -30)
+ if (call_foo () != -30)
abort ();
p = get_foo_p ();
@@ -52,12 +48,8 @@ main (void)
if (foo_ptr != foo)
abort ();
- if (my_foo_ptr != foo)
- abort ();
if ((*foo_ptr) () != -30)
abort ();
- if ((*my_foo_ptr) () != -30)
- abort ();
if (foo () != -30)
abort ();
diff --git a/elf/ifuncmod6.c b/elf/ifuncmod6.c
index 2e16c1d06d..2f6d0715e6 100644
--- a/elf/ifuncmod6.c
+++ b/elf/ifuncmod6.c
@@ -4,7 +4,7 @@ extern int foo (void);
typedef int (*foo_p) (void);
-foo_p foo_ptr = foo;
+extern foo_p foo_ptr;
foo_p
get_foo_p (void)
@@ -12,8 +12,8 @@ get_foo_p (void)
return foo_ptr;
}
-foo_p
-get_foo (void)
+int
+call_foo (void)
{
- return foo;
+ return foo ();
}
--
2.29.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 19:59 ` H.J. Lu
@ 2021-01-04 22:57 ` H.J. Lu
2021-01-05 13:03 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2021-01-04 22:57 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
On Mon, Jan 4, 2021 at 11:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >
> > On 1/4/21 2:34 PM, H.J. Lu wrote:
> > > On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
> > >>
> > >> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
> > >>> Calling an IFUNC function defined in unrelocated executable may also
> > >>> lead to segfault. Issue an error message when calling IFUNC function
> > >>> defined in the unrelocated executable from a shared library.
> > >>
> > >> The logic here makes sense, but we need a stronger error message.
> > >>
> > >> Please review my understanding and suggested error message.
> > >>
> > >> Looking forward to v2.
> > >>
> > >>> ---
> > >>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
> > >>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
> > >>> 2 files changed, 20 insertions(+), 10 deletions(-)
> > >>>
> > >>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> > >>> index fea9e579ec..dedda484ba 100644
> > >>> --- a/sysdeps/i386/dl-machine.h
> > >>> +++ b/sysdeps/i386/dl-machine.h
> > >>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> > >>> {
> > >>> # ifndef RTLD_BOOTSTRAP
> > >>
> > >> OK. Logic is in the correct place in dl-machine.h for i386.
> > >>
> > >>> if (sym_map != map
> > >>> - && sym_map->l_type != lt_executable
> > >>> && !sym_map->l_relocated)
> > >>> {
> > >>> const char *strtab
> > >>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> > >>> - _dl_error_printf ("\
> > >>> + if (sym_map->l_type == lt_executable)
> > >>> + _dl_error_printf ("\
> > >>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> > >>> + RTLD_PROGNAME, strtab + refsym->st_name,
> > >>> + map->l_name);
> > >>> + else
> > >>> + _dl_error_printf ("\
> > >>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> > >>> - RTLD_PROGNAME, map->l_name,
> > >>> - sym_map->l_name,
> > >>> - strtab + refsym->st_name);
> > >>> + RTLD_PROGNAME, map->l_name,
> > >>> + sym_map->l_name,
> > >>> + strtab + refsym->st_name);
> > >>> }
> > >>> # endif
> > >>> value = ((Elf32_Addr (*) (void)) value) ();
> > >>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> > >>> index bb93c7c6ab..fc847f4bc2 100644
> > >>> --- a/sysdeps/x86_64/dl-machine.h
> > >>> +++ b/sysdeps/x86_64/dl-machine.h
> > >>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> > >>> {
> > >>> # ifndef RTLD_BOOTSTRAP
> > >>
> > >> OK. Logic is in the correct place in dl-machine.h for x86_64.
> > >>
> > >>> if (sym_map != map
> > >>> - && sym_map->l_type != lt_executable
> > >>> && !sym_map->l_relocated)
> > >>> {
> > >>> const char *strtab
> > >>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> > >>> - _dl_error_printf ("\
> > >>> + if (sym_map->l_type == lt_executable)
> > >>> + _dl_error_printf ("\
> > >>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> > >>
> > >> The message should explain the error
> > >> e.g. "Such and such *must not* reference such and such."
> > >>
> > >> Or the message should explain how to fix the error (as the other does)
> > >> e.g. "Such and such must be relinked with such and such."
> > >>
> > >> We have made this a hard error. An executable with immediate binding
> > >> may not define an IFUNC resolver and implementation that is used from
> > >> a shared library since it creates an ordering issue with the dependent
> > >> libraries that use the resolution of the symbol i.e. you must initialize
> > >> the executable but to do that you must initialize the libraries, but to
> > >> do that you must initialize the executable etc. etc.
> > >>
> > >> In which case the error message could be:
> > >>
> > >> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
> > >> and creates an unsatisfiable circular dependency."
> > >
> > > Fixed.
> > >
> > >> Note: Use '' quotes not `' since the GNU Coding standards have changed.
> > >> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
> > >>
> > >>> + RTLD_PROGNAME, strtab + refsym->st_name,
> > >>> + map->l_name);
> > >>> + else
> > >>> + _dl_error_printf ("\
> > >>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> > >>> - RTLD_PROGNAME, map->l_name,
> > >>> - sym_map->l_name,
> > >>> - strtab + refsym->st_name);
> > >>> + RTLD_PROGNAME, map->l_name,
> > >>> + sym_map->l_name,
> > >>> + strtab + refsym->st_name);
> > >>> }
> > >>> # endif
> > >>> value = ((ElfW(Addr) (*) (void)) value) ();
> > >>>
> > >>
> > >>
> > >
> > > Here is the updated patch. Changes from V1:
> > >
> > > 1. Update the error message based on feedback from Carlos.
> > > 2. Make the error fatal instead of segfault later.
> > >
> > > OK for master?
> >
> > Could binutils have given the user a better warnings?
>
> I will take a look.
>
The problem is
0000000000003fe8 0000000300000006 R_X86_64_GLOB_DAT
0000000000000000 foo + 0
0000000000004018 0000000300000001 R_X86_64_64
0000000000000000 foo + 0
in ifuncmod6.so. When linker creates ifuncmod6.so, linker doesn't
know that foo will
be an IFUNC symbol defined in executable at run-time. When linker
creates ifuncmain6pie,
linker doesn't check dynamic relocations in ifuncmod6.so and
ifuncmod6.so used in link-time
can be different from run-time.
So there is not much linker can do.
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-04 22:57 ` H.J. Lu
@ 2021-01-05 13:03 ` Carlos O'Donell
2021-01-05 15:14 ` H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2021-01-05 13:03 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library
On 1/4/21 5:57 PM, H.J. Lu wrote:
> On Mon, Jan 4, 2021 at 11:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>>
>>> On 1/4/21 2:34 PM, H.J. Lu wrote:
>>>> On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>>>>
>>>>> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
>>>>>> Calling an IFUNC function defined in unrelocated executable may also
>>>>>> lead to segfault. Issue an error message when calling IFUNC function
>>>>>> defined in the unrelocated executable from a shared library.
>>>>>
>>>>> The logic here makes sense, but we need a stronger error message.
>>>>>
>>>>> Please review my understanding and suggested error message.
>>>>>
>>>>> Looking forward to v2.
>>>>>
>>>>>> ---
>>>>>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
>>>>>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
>>>>>> 2 files changed, 20 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
>>>>>> index fea9e579ec..dedda484ba 100644
>>>>>> --- a/sysdeps/i386/dl-machine.h
>>>>>> +++ b/sysdeps/i386/dl-machine.h
>>>>>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
>>>>>> {
>>>>>> # ifndef RTLD_BOOTSTRAP
>>>>>
>>>>> OK. Logic is in the correct place in dl-machine.h for i386.
>>>>>
>>>>>> if (sym_map != map
>>>>>> - && sym_map->l_type != lt_executable
>>>>>> && !sym_map->l_relocated)
>>>>>> {
>>>>>> const char *strtab
>>>>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>>>>> - _dl_error_printf ("\
>>>>>> + if (sym_map->l_type == lt_executable)
>>>>>> + _dl_error_printf ("\
>>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>>>>> + RTLD_PROGNAME, strtab + refsym->st_name,
>>>>>> + map->l_name);
>>>>>> + else
>>>>>> + _dl_error_printf ("\
>>>>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>>>>> - RTLD_PROGNAME, map->l_name,
>>>>>> - sym_map->l_name,
>>>>>> - strtab + refsym->st_name);
>>>>>> + RTLD_PROGNAME, map->l_name,
>>>>>> + sym_map->l_name,
>>>>>> + strtab + refsym->st_name);
>>>>>> }
>>>>>> # endif
>>>>>> value = ((Elf32_Addr (*) (void)) value) ();
>>>>>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
>>>>>> index bb93c7c6ab..fc847f4bc2 100644
>>>>>> --- a/sysdeps/x86_64/dl-machine.h
>>>>>> +++ b/sysdeps/x86_64/dl-machine.h
>>>>>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
>>>>>> {
>>>>>> # ifndef RTLD_BOOTSTRAP
>>>>>
>>>>> OK. Logic is in the correct place in dl-machine.h for x86_64.
>>>>>
>>>>>> if (sym_map != map
>>>>>> - && sym_map->l_type != lt_executable
>>>>>> && !sym_map->l_relocated)
>>>>>> {
>>>>>> const char *strtab
>>>>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
>>>>>> - _dl_error_printf ("\
>>>>>> + if (sym_map->l_type == lt_executable)
>>>>>> + _dl_error_printf ("\
>>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
>>>>>
>>>>> The message should explain the error
>>>>> e.g. "Such and such *must not* reference such and such."
>>>>>
>>>>> Or the message should explain how to fix the error (as the other does)
>>>>> e.g. "Such and such must be relinked with such and such."
>>>>>
>>>>> We have made this a hard error. An executable with immediate binding
>>>>> may not define an IFUNC resolver and implementation that is used from
>>>>> a shared library since it creates an ordering issue with the dependent
>>>>> libraries that use the resolution of the symbol i.e. you must initialize
>>>>> the executable but to do that you must initialize the libraries, but to
>>>>> do that you must initialize the executable etc. etc.
>>>>>
>>>>> In which case the error message could be:
>>>>>
>>>>> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
>>>>> and creates an unsatisfiable circular dependency."
>>>>
>>>> Fixed.
>>>>
>>>>> Note: Use '' quotes not `' since the GNU Coding standards have changed.
>>>>> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
>>>>>
>>>>>> + RTLD_PROGNAME, strtab + refsym->st_name,
>>>>>> + map->l_name);
>>>>>> + else
>>>>>> + _dl_error_printf ("\
>>>>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
>>>>>> - RTLD_PROGNAME, map->l_name,
>>>>>> - sym_map->l_name,
>>>>>> - strtab + refsym->st_name);
>>>>>> + RTLD_PROGNAME, map->l_name,
>>>>>> + sym_map->l_name,
>>>>>> + strtab + refsym->st_name);
>>>>>> }
>>>>>> # endif
>>>>>> value = ((ElfW(Addr) (*) (void)) value) ();
>>>>>>
>>>>>
>>>>>
>>>>
>>>> Here is the updated patch. Changes from V1:
>>>>
>>>> 1. Update the error message based on feedback from Carlos.
>>>> 2. Make the error fatal instead of segfault later.
>>>>
>>>> OK for master?
>>>
>>> Could binutils have given the user a better warnings?
>>
>> I will take a look.
>>
>
> The problem is
>
> 0000000000003fe8 0000000300000006 R_X86_64_GLOB_DAT
> 0000000000000000 foo + 0
> 0000000000004018 0000000300000001 R_X86_64_64
> 0000000000000000 foo + 0
>
> in ifuncmod6.so. When linker creates ifuncmod6.so, linker doesn't
> know that foo will
> be an IFUNC symbol defined in executable at run-time. When linker
> creates ifuncmain6pie,
> linker doesn't check dynamic relocations in ifuncmod6.so and
> ifuncmod6.so used in link-time
> can be different from run-time
The executable has a DT_NEEDED on ifuncmod*.so.
The static linker must load both the executable and DSO to finish linking.
All the information you need is in theory present.
An when you run with -Wl,-Map you can see this:
~~~
Local IFUNC function `foo' in ./ifuncmain.o
~~~
So the static linker sees the definition and identifies it as an IFUNC.
Then:
~~~
LOAD ./ifuncmain.o
LOAD ./ifuncmod.so
~~~
So we know ifuncmod.so is dependent and we lay it out for linkage.
It is at this point that you would have to attempt a quick check of the
dependent DSOs to generate a link warning that given the present set of
DSOs that this will not work.
Obviously it *could* work at runtime where the DSO is different, where
an LD_PRELOAD loads a DSO with an interposing definition ahead of the
executable IFUNC etc. etc. You are subject to the entire host of dynamic
resolution rules.
However, the static linker could have given a warnings given the existing
set of objects used in the static link to assemble the executable.
This kind of warning is right on the line between the static and dynamic
linkers because it is something you can detect at static link time but
can't formally prove until dynamic link time.
> So there is not much linker can do.
There is.
It is similar to generating warnings from gnu attribute tags.
You issue a warning a static link time, but it won't really fail until
you attempt to run it e.g. mismatched floating point ABI.
If you wanted it could be expressed as a gnu attribute tag:
- Non-DSO object defines IFUNC
- DSO uses IFUNC
- During merge of the sections you look for the problematic scenario.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]
2021-01-05 13:03 ` Carlos O'Donell
@ 2021-01-05 15:14 ` H.J. Lu
0 siblings, 0 replies; 14+ messages in thread
From: H.J. Lu @ 2021-01-05 15:14 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
On Tue, Jan 5, 2021 at 5:03 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 1/4/21 5:57 PM, H.J. Lu wrote:
> > On Mon, Jan 4, 2021 at 11:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>>
> >>> On 1/4/21 2:34 PM, H.J. Lu wrote:
> >>>> On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>>>>
> >>>>> On 12/28/20 9:11 AM, H.J. Lu via Libc-alpha wrote:
> >>>>>> Calling an IFUNC function defined in unrelocated executable may also
> >>>>>> lead to segfault. Issue an error message when calling IFUNC function
> >>>>>> defined in the unrelocated executable from a shared library.
> >>>>>
> >>>>> The logic here makes sense, but we need a stronger error message.
> >>>>>
> >>>>> Please review my understanding and suggested error message.
> >>>>>
> >>>>> Looking forward to v2.
> >>>>>
> >>>>>> ---
> >>>>>> sysdeps/i386/dl-machine.h | 15 ++++++++++-----
> >>>>>> sysdeps/x86_64/dl-machine.h | 15 ++++++++++-----
> >>>>>> 2 files changed, 20 insertions(+), 10 deletions(-)
> >>>>>>
> >>>>>> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
> >>>>>> index fea9e579ec..dedda484ba 100644
> >>>>>> --- a/sysdeps/i386/dl-machine.h
> >>>>>> +++ b/sysdeps/i386/dl-machine.h
> >>>>>> @@ -337,16 +337,21 @@ elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
> >>>>>> {
> >>>>>> # ifndef RTLD_BOOTSTRAP
> >>>>>
> >>>>> OK. Logic is in the correct place in dl-machine.h for i386.
> >>>>>
> >>>>>> if (sym_map != map
> >>>>>> - && sym_map->l_type != lt_executable
> >>>>>> && !sym_map->l_relocated)
> >>>>>> {
> >>>>>> const char *strtab
> >>>>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> >>>>>> - _dl_error_printf ("\
> >>>>>> + if (sym_map->l_type == lt_executable)
> >>>>>> + _dl_error_printf ("\
> >>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> >>>>>> + RTLD_PROGNAME, strtab + refsym->st_name,
> >>>>>> + map->l_name);
> >>>>>> + else
> >>>>>> + _dl_error_printf ("\
> >>>>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> >>>>>> - RTLD_PROGNAME, map->l_name,
> >>>>>> - sym_map->l_name,
> >>>>>> - strtab + refsym->st_name);
> >>>>>> + RTLD_PROGNAME, map->l_name,
> >>>>>> + sym_map->l_name,
> >>>>>> + strtab + refsym->st_name);
> >>>>>> }
> >>>>>> # endif
> >>>>>> value = ((Elf32_Addr (*) (void)) value) ();
> >>>>>> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
> >>>>>> index bb93c7c6ab..fc847f4bc2 100644
> >>>>>> --- a/sysdeps/x86_64/dl-machine.h
> >>>>>> +++ b/sysdeps/x86_64/dl-machine.h
> >>>>>> @@ -314,16 +314,21 @@ elf_machine_rela (struct link_map *map, const ElfW(Rela) *reloc,
> >>>>>> {
> >>>>>> # ifndef RTLD_BOOTSTRAP
> >>>>>
> >>>>> OK. Logic is in the correct place in dl-machine.h for x86_64.
> >>>>>
> >>>>>> if (sym_map != map
> >>>>>> - && sym_map->l_type != lt_executable
> >>>>>> && !sym_map->l_relocated)
> >>>>>> {
> >>>>>> const char *strtab
> >>>>>> = (const char *) D_PTR (map, l_info[DT_STRTAB]);
> >>>>>> - _dl_error_printf ("\
> >>>>>> + if (sym_map->l_type == lt_executable)
> >>>>>> + _dl_error_printf ("\
> >>>>>> +%s: IFUNC symbol `%s' referenced in `%s' is defined in executable\n",
> >>>>>
> >>>>> The message should explain the error
> >>>>> e.g. "Such and such *must not* reference such and such."
> >>>>>
> >>>>> Or the message should explain how to fix the error (as the other does)
> >>>>> e.g. "Such and such must be relinked with such and such."
> >>>>>
> >>>>> We have made this a hard error. An executable with immediate binding
> >>>>> may not define an IFUNC resolver and implementation that is used from
> >>>>> a shared library since it creates an ordering issue with the dependent
> >>>>> libraries that use the resolution of the symbol i.e. you must initialize
> >>>>> the executable but to do that you must initialize the libraries, but to
> >>>>> do that you must initialize the executable etc. etc.
> >>>>>
> >>>>> In which case the error message could be:
> >>>>>
> >>>>> "%s: IFUNC symbol '%s' referenced in '%s' is defined in the executable
> >>>>> and creates an unsatisfiable circular dependency."
> >>>>
> >>>> Fixed.
> >>>>
> >>>>> Note: Use '' quotes not `' since the GNU Coding standards have changed.
> >>>>> https://www.gnu.org/prep/standards/standards.html#Quote-Characters
> >>>>>
> >>>>>> + RTLD_PROGNAME, strtab + refsym->st_name,
> >>>>>> + map->l_name);
> >>>>>> + else
> >>>>>> + _dl_error_printf ("\
> >>>>>> %s: Relink `%s' with `%s' for IFUNC symbol `%s'\n",
> >>>>>> - RTLD_PROGNAME, map->l_name,
> >>>>>> - sym_map->l_name,
> >>>>>> - strtab + refsym->st_name);
> >>>>>> + RTLD_PROGNAME, map->l_name,
> >>>>>> + sym_map->l_name,
> >>>>>> + strtab + refsym->st_name);
> >>>>>> }
> >>>>>> # endif
> >>>>>> value = ((ElfW(Addr) (*) (void)) value) ();
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>> Here is the updated patch. Changes from V1:
> >>>>
> >>>> 1. Update the error message based on feedback from Carlos.
> >>>> 2. Make the error fatal instead of segfault later.
> >>>>
> >>>> OK for master?
> >>>
> >>> Could binutils have given the user a better warnings?
> >>
> >> I will take a look.
> >>
> >
> > The problem is
> >
> > 0000000000003fe8 0000000300000006 R_X86_64_GLOB_DAT
> > 0000000000000000 foo + 0
> > 0000000000004018 0000000300000001 R_X86_64_64
> > 0000000000000000 foo + 0
> >
> > in ifuncmod6.so. When linker creates ifuncmod6.so, linker doesn't
> > know that foo will
> > be an IFUNC symbol defined in executable at run-time. When linker
> > creates ifuncmain6pie,
> > linker doesn't check dynamic relocations in ifuncmod6.so and
> > ifuncmod6.so used in link-time
> > can be different from run-time
> The executable has a DT_NEEDED on ifuncmod*.so.
>
> The static linker must load both the executable and DSO to finish linking.
>
> All the information you need is in theory present.
>
> An when you run with -Wl,-Map you can see this:
> ~~~
> Local IFUNC function `foo' in ./ifuncmain.o
> ~~~
> So the static linker sees the definition and identifies it as an IFUNC.
>
> Then:
> ~~~
> LOAD ./ifuncmain.o
> LOAD ./ifuncmod.so
Static linker doesn't use dynamic relocations in ifuncmod.so.
It only uses the dynamic symbol table. Some shared objects
used for linking don't even have dynamic relocations.
> ~~~
> So we know ifuncmod.so is dependent and we lay it out for linkage.
>
> It is at this point that you would have to attempt a quick check of the
> dependent DSOs to generate a link warning that given the present set of
> DSOs that this will not work.
>
> Obviously it *could* work at runtime where the DSO is different, where
> an LD_PRELOAD loads a DSO with an interposing definition ahead of the
> executable IFUNC etc. etc. You are subject to the entire host of dynamic
> resolution rules.
>
> However, the static linker could have given a warnings given the existing
> set of objects used in the static link to assemble the executable.
>
> This kind of warning is right on the line between the static and dynamic
> linkers because it is something you can detect at static link time but
> can't formally prove until dynamic link time.
>
> > So there is not much linker can do.
>
> There is.
>
> It is similar to generating warnings from gnu attribute tags.
>
> You issue a warning a static link time, but it won't really fail until
> you attempt to run it e.g. mismatched floating point ABI.
>
> If you wanted it could be expressed as a gnu attribute tag:
> - Non-DSO object defines IFUNC
> - DSO uses IFUNC
> - During merge of the sections you look for the problematic scenario.
>
> --
> Cheers,
> Carlos.
>
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency [BZ #20019]
2021-01-04 22:38 ` [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency " H.J. Lu
@ 2021-01-13 19:43 ` Adhemerval Zanella
2021-01-13 19:48 ` H.J. Lu
0 siblings, 1 reply; 14+ messages in thread
From: Adhemerval Zanella @ 2021-01-13 19:43 UTC (permalink / raw)
To: libc-alpha, H.J. Lu, Carlos O'Donell
On 04/01/2021 19:38, H.J. Lu via Libc-alpha wrote:
> On Mon, Jan 4, 2021 at 1:20 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>
> ...
>>>
>>> [hjl@gnu-cfl-2 build-x86_64-linux]$ ./elf/ifuncmain6pie --direct
>>> ./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in
>>> '/export/build/gnu/tools-build/glibc/build-x86_64-linux/elf/ifuncmod6.so'
>>> is defined in the executable and creates an unsatisfiable circular
>>> dependency.
>>> [hjl@gnu-cfl-2 build-x86_64-linux]$
>>>
>>> The message is correct. Should we update the testcase to avoid it?
>>
>> Yes, but it is still possible to support this with lazy binding?
>>
>> Should ifuncmain6pie be explicitly compiled with -Wl,-z,lazy to
>> bypass selection from the toolchain?
>
> The problem is non-JUMP_SLOT relocations. Here is a patch to
> remove them. OK for master?
>
> Thanks.
>
I am getting a failure for elf/ifuncmain6pie for a couple of days:
$ ./testrun.sh elf/ifuncmain6pie
elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in '/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/ifuncmod6.so' is defined in the executable and creates an unsatisfiable circular dependency.
The patch looks ok for 2.33.
However, I think it should be been added along with 6ea5b57afa5
fix in first place. Why hasn't it shown in your make check? Does the
failure depend on a binutils version?
> diff --git a/elf/Makefile b/elf/Makefile
> index 543800f4be..c41d11693b 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1403,6 +1403,8 @@ CFLAGS-ifuncmain7pie.c += $(pie-ccflag)
> CFLAGS-ifuncmain9pie.c += $(pie-ccflag)
> CFLAGS-tst-ifunc-textrel.c += $(pic-ccflag)
>
> +LDFLAGS-ifuncmain6pie = -Wl,-z,lazy
> +
> $(objpfx)ifuncmain1pie: $(objpfx)ifuncmod1.so
> $(objpfx)ifuncmain1staticpie: $(objpfx)ifuncdep1pic.o
> $(objpfx)ifuncmain1vispie: $(objpfx)ifuncmod1.so
> diff --git a/elf/ifuncmain6pie.c b/elf/ifuncmain6pie.c
> index 04faeb86ef..4a01906836 100644
> --- a/elf/ifuncmain6pie.c
> +++ b/elf/ifuncmain6pie.c
> @@ -9,7 +9,6 @@
> #include "ifunc-sel.h"
>
> typedef int (*foo_p) (void);
> -extern foo_p foo_ptr;
>
> static int
> one (void)
> @@ -28,20 +27,17 @@ foo_ifunc (void)
> }
>
> extern int foo (void);
> -extern foo_p get_foo (void);
> +extern int call_foo (void);
> extern foo_p get_foo_p (void);
>
> -foo_p my_foo_ptr = foo;
> +foo_p foo_ptr = foo;
>
> int
> main (void)
> {
> foo_p p;
>
> - p = get_foo ();
> - if (p != foo)
> - abort ();
> - if ((*p) () != -30)
> + if (call_foo () != -30)
> abort ();
>
> p = get_foo_p ();
> @@ -52,12 +48,8 @@ main (void)
>
> if (foo_ptr != foo)
> abort ();
> - if (my_foo_ptr != foo)
> - abort ();
> if ((*foo_ptr) () != -30)
> abort ();
> - if ((*my_foo_ptr) () != -30)
> - abort ();
> if (foo () != -30)
> abort ();
>
> diff --git a/elf/ifuncmod6.c b/elf/ifuncmod6.c
> index 2e16c1d06d..2f6d0715e6 100644
> --- a/elf/ifuncmod6.c
> +++ b/elf/ifuncmod6.c
> @@ -4,7 +4,7 @@ extern int foo (void);
>
> typedef int (*foo_p) (void);
>
> -foo_p foo_ptr = foo;
> +extern foo_p foo_ptr;
>
> foo_p
> get_foo_p (void)
> @@ -12,8 +12,8 @@ get_foo_p (void)
> return foo_ptr;
> }
>
> -foo_p
> -get_foo (void)
> +int
> +call_foo (void)
> {
> - return foo;
> + return foo ();
> }
> --
> 2.29.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency [BZ #20019]
2021-01-13 19:43 ` Adhemerval Zanella
@ 2021-01-13 19:48 ` H.J. Lu
2021-01-14 13:10 ` Adhemerval Zanella
0 siblings, 1 reply; 14+ messages in thread
From: H.J. Lu @ 2021-01-13 19:48 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: GNU C Library, Carlos O'Donell
On Wed, Jan 13, 2021 at 11:43 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 04/01/2021 19:38, H.J. Lu via Libc-alpha wrote:
> > On Mon, Jan 4, 2021 at 1:20 PM Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> > ...
> >>>
> >>> [hjl@gnu-cfl-2 build-x86_64-linux]$ ./elf/ifuncmain6pie --direct
> >>> ./elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in
> >>> '/export/build/gnu/tools-build/glibc/build-x86_64-linux/elf/ifuncmod6.so'
> >>> is defined in the executable and creates an unsatisfiable circular
> >>> dependency.
> >>> [hjl@gnu-cfl-2 build-x86_64-linux]$
> >>>
> >>> The message is correct. Should we update the testcase to avoid it?
> >>
> >> Yes, but it is still possible to support this with lazy binding?
> >>
> >> Should ifuncmain6pie be explicitly compiled with -Wl,-z,lazy to
> >> bypass selection from the toolchain?
> >
> > The problem is non-JUMP_SLOT relocations. Here is a patch to
> > remove them. OK for master?
> >
> > Thanks.
> >
>
> I am getting a failure for elf/ifuncmain6pie for a couple of days:
>
> $ ./testrun.sh elf/ifuncmain6pie
> elf/ifuncmain6pie: IFUNC symbol 'foo' referenced in '/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/ifuncmod6.so' is defined in the executable and creates an unsatisfiable circular dependency.
>
> The patch looks ok for 2.33.
I am checking it in. Thanks.
> However, I think it should be been added along with 6ea5b57afa5
I don't want the testcase issue to block the code fix.
> fix in first place. Why hasn't it shown in your make check? Does the
> failure depend on a binutils version?
No.
--
H.J.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency [BZ #20019]
2021-01-13 19:48 ` H.J. Lu
@ 2021-01-14 13:10 ` Adhemerval Zanella
0 siblings, 0 replies; 14+ messages in thread
From: Adhemerval Zanella @ 2021-01-14 13:10 UTC (permalink / raw)
To: H.J. Lu; +Cc: GNU C Library, Carlos O'Donell
On 13/01/2021 16:48, H.J. Lu wrote:
> On Wed, Jan 13, 2021 at 11:43 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 04/01/2021 19:38, H.J. Lu via Libc-alpha wrote:
>
>> However, I think it should be been added along with 6ea5b57afa5
>
> I don't want the testcase issue to block the code fix.
>
>> fix in first place. Why hasn't it shown in your make check? Does the
>> failure depend on a binutils version?
>
> No.
>
In this case I think the tests should have been disabled until we
add a proper fix. I saw that regression for a couple of days before I
had time to investigate and see you already posted a fix.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-01-14 13:10 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-28 14:11 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019] H.J. Lu
2021-01-04 18:47 ` Carlos O'Donell
2021-01-04 19:34 ` V2 " H.J. Lu
2021-01-04 19:50 ` Carlos O'Donell
2021-01-04 19:59 ` H.J. Lu
2021-01-04 22:57 ` H.J. Lu
2021-01-05 13:03 ` Carlos O'Donell
2021-01-05 15:14 ` H.J. Lu
2021-01-04 20:44 ` H.J. Lu
2021-01-04 21:20 ` Carlos O'Donell
2021-01-04 22:38 ` [PATCH] ifuncmain6pie: Remove the circular IFUNC dependency " H.J. Lu
2021-01-13 19:43 ` Adhemerval Zanella
2021-01-13 19:48 ` H.J. Lu
2021-01-14 13:10 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).