diff mbox series

V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019]

Message ID CAMe9rOoNA-LBHDiThPftaSto1x2ss1o6fZvBUV_5XXgEr6M7qQ@mail.gmail.com
State Committed
Delegated to: Carlos O'Donell
Headers
Series V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019] |

Commit Message

H.J. Lu Jan. 4, 2021, 7:34 p.m. UTC 
  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.

Comments

Carlos O'Donell Jan. 4, 2021, 7:50 p.m. UTC | #1 
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
>
H.J. Lu Jan. 4, 2021, 7:59 p.m. UTC | #2 
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. Lu Jan. 4, 2021, 8:44 p.m. UTC | #3 
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?
Carlos O'Donell Jan. 4, 2021, 9:20 p.m. UTC | #4 
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?
H.J. Lu Jan. 4, 2021, 10:57 p.m. UTC | #5 
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.
Carlos O'Donell Jan. 5, 2021, 1:03 p.m. UTC | #6 
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.
H.J. Lu Jan. 5, 2021, 3:14 p.m. UTC | #7 
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.
>
diff mbox series

Patch

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