From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x336.google.com (mail-ot1-x336.google.com [IPv6:2607:f8b0:4864:20::336]) by sourceware.org (Postfix) with ESMTPS id EABDE3854812 for ; Tue, 5 Jan 2021 15:14:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EABDE3854812 Received: by mail-ot1-x336.google.com with SMTP id j12so29508841ota.7 for ; Tue, 05 Jan 2021 07:14:50 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=w7EirP33141bVXd6ZHZUPVHkuLkFGYFhkkr/GXn12wg=; b=gticunPkqu0NoGUQxpQsVZp99BadKCL2E//G5kdhEzTKTvW9VrHQCmEmdsKVCwaZ22 QmzG9/Vntjlw//1SsV7tRdH+j0DTH2MLpWY8Zr4wDXBKpCABSZ7iVgY2XUJxKztwSbfd Y2/SYUfs+o/f9B6EjtBbskyBNBdBzDK2z7eNM4NLtvMljwCko7yx4E/d7bIhwrBjr9VD rucfzLtGcLbUVtnNeiStDTu4YyL90gPwaKqZnlSQynZIDh4zuNO00lMhtakHoQPyMzFt 2hia8N/gecoiiRQreqyN2K4ZQRBjVHGoi9WUVNMCoCkXxaOVXqmGFqhRxkzvNse830wX 1dYA== X-Gm-Message-State: AOAM532dwnBZ54BUbMUfct77q/4LmmWEFECw6QgVowZCW2Fz8s/iQlkQ HUl9A/Both9Op6GOmeYaHEmSgNRwAKcceGitFQkc/fOuwrk= X-Google-Smtp-Source: ABdhPJwst9VKz1mUjJ7zs0HseaG8n6xGNH2jh8B3YOYJe5wRD/BtRa46c3lsQYLL8F+JQXB+MG2tMrYZzhO+6L8J4zs= X-Received: by 2002:a05:6830:10d2:: with SMTP id z18mr56059689oto.90.1609859690318; Tue, 05 Jan 2021 07:14:50 -0800 (PST) MIME-Version: 1.0 References: <20201228141102.2562718-1-hjl.tools@gmail.com> <0d676184-2aee-6580-7281-dbbafbe671c4@redhat.com> <4058143e-9b84-f8f5-1361-420a5b3aa808@redhat.com> <75eadd2f-1d32-a887-6506-3fdde23a97db@redhat.com> In-Reply-To: <75eadd2f-1d32-a887-6506-3fdde23a97db@redhat.com> From: "H.J. Lu" Date: Tue, 5 Jan 2021 07:14:14 -0800 Message-ID: Subject: Re: V2 [PATCH] x86: Check IFUNC definition in unrelocated executable [BZ #20019] To: "Carlos O'Donell" Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3036.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Jan 2021 15:14:53 -0000 On Tue, Jan 5, 2021 at 5:03 AM Carlos O'Donell wrote: > > On 1/4/21 5:57 PM, H.J. Lu wrote: > > On Mon, Jan 4, 2021 at 11:59 AM H.J. Lu wrote: > >> > >> On Mon, Jan 4, 2021 at 11:50 AM Carlos O'Donell wrote: > >>> > >>> On 1/4/21 2:34 PM, H.J. Lu wrote: > >>>> On Mon, Jan 4, 2021 at 10:47 AM Carlos O'Donell 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.