From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62e.google.com (mail-pl1-x62e.google.com [IPv6:2607:f8b0:4864:20::62e]) by sourceware.org (Postfix) with ESMTPS id D43883856267 for ; Wed, 15 Jun 2022 18:13:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D43883856267 Received: by mail-pl1-x62e.google.com with SMTP id o6so11100183plg.2 for ; Wed, 15 Jun 2022 11:13:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=7PL20sYoU2U0XjBUJtyDbeuUZ/8hoxWEJC8AorEQ2sA=; b=SKHVbJnFkFYXUT9aj8Wyt9+5qtuG2OBpi6CcCR/dlXAHFugVsbU4nLeJe8hGHjnVLd FeejF+CisTEEGmcr2TqaQPcA5HbSnkGI1DfN16Qd++53jwsd9A5jqY1hInElNsFnvYfz 6Efx4gLTb372vrxdfOWkQLYwu6zkNIZorXcgglZ4T5mwRL6y/TurnPrjyKOr3lNi3RyE MfDUlAiP2JYa19RIql9rmKtFGChTzRAWa1+GcUFqOX88WwNLQQGyC7ip7OVG6eHivlSW v06F3rrbWhObC9OyxpGAdOBAf/Fn1VBXKlRNk4bM3Wcij+NPj3j8rVLkTLqiKZvEXiwv O91g== X-Gm-Message-State: AJIora9qqmpFzn+WNyKv+XanX43gear5LTUMQ4kDzIoncurjD419+Ggi lrUhnzn03+TzpwluChVcTgc7SQ== X-Google-Smtp-Source: AGRyM1thTDur63DHSulw2btPW3CEnodwCuqf/an8TPZlQ3R7L27uKMQppnD6zwRJRC2fMsM0V5Zh1A== X-Received: by 2002:a17:902:ec83:b0:168:e5ad:8071 with SMTP id x3-20020a170902ec8300b00168e5ad8071mr874269plg.102.1655316783544; Wed, 15 Jun 2022 11:13:03 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:73fa:a66c:4a7a:d736]) by smtp.gmail.com with ESMTPSA id jd13-20020a170903260d00b0016184e7b013sm9628034plb.36.2022.06.15.11.13.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jun 2022 11:13:03 -0700 (PDT) Date: Wed, 15 Jun 2022 11:12:59 -0700 From: Fangrui Song To: Szabolcs Nagy Cc: libc-alpha@sourceware.org Subject: Re: [PATCH v4] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA Message-ID: <20220615181259.ifhqknhtqrqyxucz@google.com> References: <20220608171021.3150916-1-maskray@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-27.4 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Wed, 15 Jun 2022 18:13:08 -0000 On 2022-06-15, Szabolcs Nagy wrote: >The 06/08/2022 10:10, Fangrui Song wrote: >> If an executable has copy relocations for extern protected data, that >> can only work if the library containing the definition is built with >> assumptions (a) the compiler emits GOT-generating relocations (b) the >> linker produces R_*_GLOB_DAT instead of R_*_RELATIVE. Otherwise the >> library uses its own definition directly and the executable accesses a >> stale copy. Note: the GOT relocations defeat the purpose of protected >> visibility as an optimization, but allow rtld to make the executable and >> library use the same copy when copy relocations are present, but it >> turns out this never worked perfectly. >> >> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has strange semantics when both >> a.so and b.so define protected var and the executable copy relocates >> var: b.so accesses its own copy even with GLOB_DAT. The behavior change >> is from commit 62da1e3b00b51383ffa7efc89d8addda0502e107 (x86) and then >> copied to nios2 (ae5eae7cfc9c4a8297ff82ec6b794faca1976ecc) and arc >> (0e7d930c4c11de896fe807f67fa1eb756c9c1e05). >> >> Without ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA, b.so accesses the copy >> relocated data like a.so. >> >> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA has another effect in the absence >> of copy relocations: when a protected data symbol is defined in multiple >> objects, the code tries to bind the relocation locally. Without >> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA, STV_PROTECTED is handled in the >> same way as STV_DEFAULT: if ld produces GLOB_DAT (some ports of GNU ld), >> the relocation will bind to the first definition; otherwise (e.g. >> ld.lld) ld does the binding locally and ld.so doesn't help. > >this text does not look right with the updated patch. > >i think you should just remove it, otherwise the patch looks ok. Thanks. I'll remove this stale paragraph and push it, given H.J's favor previously. >> >> It's extremely unlikely anyone relies on the >> ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA behavior, so let's remove it: this >> removes a check in the symbol lookup code. >> >> -- >> Changes from v1: >> * Reword commit message as suggested by Szabolcs Nagy >> >> Changes from v2: >> * Explain interposition behavior >> >> Changes from v3: >> * Keep `if (__glibc_unlikely (protected != 0))` for now to make this >> more focus on data symbols. I will still try removing the code in the future. >> --- >> elf/dl-lookup.c | 60 +------------------------------------ >> sysdeps/arc/dl-sysdep.h | 21 ------------- >> sysdeps/generic/ldsodefs.h | 12 +------- >> sysdeps/i386/dl-machine.h | 3 +- >> sysdeps/nios2/dl-sysdep.h | 21 ------------- >> sysdeps/x86/dl-lookupcfg.h | 4 --- >> sysdeps/x86_64/dl-machine.h | 8 ++--- >> 7 files changed, 5 insertions(+), 124 deletions(-) >> delete mode 100644 sysdeps/arc/dl-sysdep.h >> delete mode 100644 sysdeps/nios2/dl-sysdep.h >> >> diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c >> index a42f6d5390..8cb32321da 100644 >> --- a/elf/dl-lookup.c >> +++ b/elf/dl-lookup.c >> @@ -456,59 +456,6 @@ do_lookup_x (const char *undef_name, unsigned int new_hash, >> if (sym != NULL) >> { >> found_it: >> - /* When UNDEF_MAP is NULL, which indicates we are called from >> - do_lookup_x on relocation against protected data, we skip >> - the data definion in the executable from copy reloc. */ >> - if (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA >> - && undef_map == NULL >> - && map->l_type == lt_executable >> - && type_class == ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA) >> - { >> - const ElfW(Sym) *s; >> - unsigned int i; >> - >> -#if ! ELF_MACHINE_NO_RELA >> - if (map->l_info[DT_RELA] != NULL >> - && map->l_info[DT_RELASZ] != NULL >> - && map->l_info[DT_RELASZ]->d_un.d_val != 0) >> - { >> - const ElfW(Rela) *rela >> - = (const ElfW(Rela) *) D_PTR (map, l_info[DT_RELA]); >> - unsigned int rela_count >> - = map->l_info[DT_RELASZ]->d_un.d_val / sizeof (*rela); >> - >> - for (i = 0; i < rela_count; i++, rela++) >> - if (elf_machine_type_class (ELFW(R_TYPE) (rela->r_info)) >> - == ELF_RTYPE_CLASS_COPY) >> - { >> - s = &symtab[ELFW(R_SYM) (rela->r_info)]; >> - if (!strcmp (strtab + s->st_name, undef_name)) >> - goto skip; >> - } >> - } >> -#endif >> -#if ! ELF_MACHINE_NO_REL >> - if (map->l_info[DT_REL] != NULL >> - && map->l_info[DT_RELSZ] != NULL >> - && map->l_info[DT_RELSZ]->d_un.d_val != 0) >> - { >> - const ElfW(Rel) *rel >> - = (const ElfW(Rel) *) D_PTR (map, l_info[DT_REL]); >> - unsigned int rel_count >> - = map->l_info[DT_RELSZ]->d_un.d_val / sizeof (*rel); >> - >> - for (i = 0; i < rel_count; i++, rel++) >> - if (elf_machine_type_class (ELFW(R_TYPE) (rel->r_info)) >> - == ELF_RTYPE_CLASS_COPY) >> - { >> - s = &symtab[ELFW(R_SYM) (rel->r_info)]; >> - if (!strcmp (strtab + s->st_name, undef_name)) >> - goto skip; >> - } >> - } >> -#endif >> - } >> - >> /* Hidden and internal symbols are local, ignore them. */ >> if (__glibc_unlikely (dl_symbol_visibility_binds_local_p (sym))) >> goto skip; >> @@ -875,12 +822,7 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map, >> for (scope = symbol_scope; *scope != NULL; i = 0, ++scope) >> if (do_lookup_x (undef_name, new_hash, &old_hash, *ref, >> &protected_value, *scope, i, version, flags, >> - skip_map, >> - (ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA >> - && ELFW(ST_TYPE) ((*ref)->st_info) == STT_OBJECT >> - && type_class == ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA) >> - ? ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA >> - : ELF_RTYPE_CLASS_PLT, NULL) != 0) >> + skip_map, ELF_RTYPE_CLASS_PLT, NULL) != 0) >> break; >> >> if (protected_value.s != NULL && protected_value.m != undef_map) >> diff --git a/sysdeps/arc/dl-sysdep.h b/sysdeps/arc/dl-sysdep.h >> deleted file mode 100644 >> index cf4d160a73..0000000000 >> --- a/sysdeps/arc/dl-sysdep.h >> +++ /dev/null >> @@ -1,21 +0,0 @@ >> -/* System-specific settings for dynamic linker code. ARC version. >> - Copyright (C) 2020-2022 Free Software Foundation, Inc. >> - This file is part of the GNU C Library. >> - >> - The GNU C Library is free software; you can redistribute it and/or >> - modify it under the terms of the GNU Lesser General Public >> - License as published by the Free Software Foundation; either >> - version 2.1 of the License, or (at your option) any later version. >> - >> - The GNU C Library is distributed in the hope that it will be useful, >> - but WITHOUT ANY WARRANTY; without even the implied warranty of >> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> - Lesser General Public License for more details. >> - >> - You should have received a copy of the GNU Lesser General Public >> - License along with the GNU C Library. If not, see >> - . */ >> - >> -#include_next >> - >> -#define DL_EXTERN_PROTECTED_DATA >> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h >> index 446d12a68c..050a3032de 100644 >> --- a/sysdeps/generic/ldsodefs.h >> +++ b/sysdeps/generic/ldsodefs.h >> @@ -149,23 +149,13 @@ dl_symbol_visibility_binds_local_p (const ElfW(Sym) *sym) >> satisfied by any symbol in the executable. Some architectures do >> not support copy relocations. In this case we define the macro to >> zero so that the code for handling them gets automatically optimized >> - out. ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA means address of protected >> - data defined in the shared library may be external, i.e., due to copy >> - relocation. */ >> + out. */ >> #define ELF_RTYPE_CLASS_PLT 1 >> #ifndef DL_NO_COPY_RELOCS >> # define ELF_RTYPE_CLASS_COPY 2 >> #else >> # define ELF_RTYPE_CLASS_COPY 0 >> #endif >> -/* If DL_EXTERN_PROTECTED_DATA is defined, address of protected data >> - defined in the shared library may be external, i.e., due to copy >> - relocation. */ >> -#ifdef DL_EXTERN_PROTECTED_DATA >> -# define ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA 4 >> -#else >> -# define ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA 0 >> -#endif >> >> /* ELF uses the PF_x macros to specify the segment permissions, mmap >> uses PROT_xxx. In most cases the three macros have the values 1, 2, >> diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h >> index 8779983c8c..a8ee594ff4 100644 >> --- a/sysdeps/i386/dl-machine.h >> +++ b/sysdeps/i386/dl-machine.h >> @@ -194,8 +194,7 @@ _dl_start_user:\n\ >> || (type) == R_386_TLS_DTPOFF32 || (type) == R_386_TLS_TPOFF32 \ >> || (type) == R_386_TLS_TPOFF || (type) == R_386_TLS_DESC) \ >> * ELF_RTYPE_CLASS_PLT) \ >> - | (((type) == R_386_COPY) * ELF_RTYPE_CLASS_COPY) \ >> - | (((type) == R_386_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA)) >> + | (((type) == R_386_COPY) * ELF_RTYPE_CLASS_COPY)) >> >> /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries. */ >> #define ELF_MACHINE_JMP_SLOT R_386_JMP_SLOT >> diff --git a/sysdeps/nios2/dl-sysdep.h b/sysdeps/nios2/dl-sysdep.h >> deleted file mode 100644 >> index 257b37c258..0000000000 >> --- a/sysdeps/nios2/dl-sysdep.h >> +++ /dev/null >> @@ -1,21 +0,0 @@ >> -/* System-specific settings for dynamic linker code. Nios II version. >> - Copyright (C) 2009-2022 Free Software Foundation, Inc. >> - This file is part of the GNU C Library. >> - >> - The GNU C Library is free software; you can redistribute it and/or >> - modify it under the terms of the GNU Lesser General Public >> - License as published by the Free Software Foundation; either >> - version 2.1 of the License, or (at your option) any later version. >> - >> - The GNU C Library is distributed in the hope that it will be useful, >> - but WITHOUT ANY WARRANTY; without even the implied warranty of >> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> - Lesser General Public License for more details. >> - >> - You should have received a copy of the GNU Lesser General Public >> - License along with the GNU C Library. If not, see >> - . */ >> - >> -#include_next >> - >> -#define DL_EXTERN_PROTECTED_DATA >> diff --git a/sysdeps/x86/dl-lookupcfg.h b/sysdeps/x86/dl-lookupcfg.h >> index 18b3b49f6e..e136cc63af 100644 >> --- a/sysdeps/x86/dl-lookupcfg.h >> +++ b/sysdeps/x86/dl-lookupcfg.h >> @@ -20,10 +20,6 @@ >> >> #include_next >> >> -/* Address of protected data defined in the shared library may be >> - external due to copy relocation. */ >> -#define DL_EXTERN_PROTECTED_DATA >> - >> struct link_map; >> >> extern void _dl_unmap (struct link_map *map) attribute_hidden; >> diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h >> index b9122944b9..06c9a932b0 100644 >> --- a/sysdeps/x86_64/dl-machine.h >> +++ b/sysdeps/x86_64/dl-machine.h >> @@ -172,10 +172,7 @@ _dl_start_user:\n\ >> TLS variable, so undefined references should not be allowed to >> define the value. >> ELF_RTYPE_CLASS_COPY iff TYPE should not be allowed to resolve to one >> - of the main executable's symbols, as for a COPY reloc. >> - ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA iff TYPE describes relocation may >> - against protected data whose address be external due to copy relocation. >> - */ >> + of the main executable's symbols, as for a COPY reloc. */ >> #define elf_machine_type_class(type) \ >> ((((type) == R_X86_64_JUMP_SLOT \ >> || (type) == R_X86_64_DTPMOD64 \ >> @@ -183,8 +180,7 @@ _dl_start_user:\n\ >> || (type) == R_X86_64_TPOFF64 \ >> || (type) == R_X86_64_TLSDESC) \ >> * ELF_RTYPE_CLASS_PLT) \ >> - | (((type) == R_X86_64_COPY) * ELF_RTYPE_CLASS_COPY) \ >> - | (((type) == R_X86_64_GLOB_DAT) * ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA)) >> + | (((type) == R_X86_64_COPY) * ELF_RTYPE_CLASS_COPY)) >> >> /* A reloc type used for ld.so cmdline arg lookups to reject PLT entries. */ >> #define ELF_MACHINE_JMP_SLOT R_X86_64_JUMP_SLOT >> -- >> 2.36.1.255.ge46751e96f-goog >>