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

[Carlos O'Donell <carlos@redhat.com>]

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.