From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x231.google.com (mail-oi1-x231.google.com [IPv6:2607:f8b0:4864:20::231]) by sourceware.org (Postfix) with ESMTPS id 3E59E38708DD for ; Mon, 4 Jan 2021 22:58:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3E59E38708DD Received: by mail-oi1-x231.google.com with SMTP id s2so34015071oij.2 for ; Mon, 04 Jan 2021 14:58:01 -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=2T7ukb7WRj4GgUxKPBs8a9nTmRZBzRR5N9Sb86hNDac=; b=DWmsc8Wy86rFeKDkqVzIJMP3a2xLsszu6OGPnPcjIaByCvhsvdpJ/mpI8o7NBPLrXZ axDSJgaKAWou1mlsnrAGt5sMQMWB8ucJUPrNkY7NtvxznWN3aypSRT48NbzFIcI7nNax Its8a8HYIjvp5d+rskgbESzxYanqfgANz0KIyH4NqiJuxtMULlmOZNvhT4TUYvtdBKil 8Itp2bTIEZA3VGIfoTW7sWdLypL1fm76Pzb3oI98WaaOmo2KwfVsdDIJMimQjERkDC22 j86AvMoLb8jFoQmEgILH4uw1FvarLIyOtAEvQr2Zz5F12BWVqt6Pvl9ehPTuyb9zER0D hLmQ== X-Gm-Message-State: AOAM533rkA6uhpWC0VBcGpAo8WkP5FWpHBXZp4Q9G2XgRTCS9AQVMHBM 6SSyf8GZj5pZpJeXgdGu1LRwcmnf2k41DKxrlbg= X-Google-Smtp-Source: ABdhPJxfLsbjGiLk23QhPEr/6d4NnrmmghAygSODw9UHma73Qho3pAC6Yom2yzMi1a/96UBAlAhwL2SpY67i3sSd5Is= X-Received: by 2002:aca:4d8b:: with SMTP id a133mr820086oib.79.1609801080694; Mon, 04 Jan 2021 14:58:00 -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> In-Reply-To: From: "H.J. Lu" Date: Mon, 4 Jan 2021 14:57:24 -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=-3037.1 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: Mon, 04 Jan 2021 22:58:03 -0000 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. So there is not much linker can do. H.J.