From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42c.google.com (mail-pf1-x42c.google.com [IPv6:2607:f8b0:4864:20::42c]) by sourceware.org (Postfix) with ESMTPS id EF394385829F for ; Wed, 15 Jun 2022 04:17:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF394385829F Received: by mail-pf1-x42c.google.com with SMTP id y6so4689944pfr.13 for ; Tue, 14 Jun 2022 21:17:58 -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:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=2UcDHV+7Ra+7x9OIv12lGrhV2Eug+Ka/Tgoujs8k1W4=; b=DikIK490+8lpsvqnQPxaGfq3r0ve5ETWxZdBpBqGG3hjJ2xvJnWE8hMD6pQqRv9/WF ix7XqRsJsca4sYBttFTCSQzC/osYmKMPpw+OKcqkmSnKunkQfQafk6ucn5nbxrrE5mfe 9CTeoa2GqZIUWmI0gqrjiVSBrYXxd+1B0uWcZAz/NivCP/7c31r+y4VeL4B5DTOPpiBz qlgvyw/Ai9JLRh88IzN0jdxid0rZlpclPoD7SRok4nPwG3xKUbgw5aPoQau8yvyM9ktc u51E7BBikyftp8l6X5ZLToZ6f91GvdIPpbjDym4b1uUZglL6YDtrcl89sL2uzhxbXEKP Rtwg== X-Gm-Message-State: AOAM533kegMzON/alvuLnbo4632H/wUXnDObmo7ekmTUUJnonnKA/NDV cfO1pPwV3ok6ccoObDczo7ru0GvClrWSvw== X-Google-Smtp-Source: ABdhPJxFsklpnQNGVwq3sWU6urF34a/FdC03MwGgywJJ05MUowlxvGEcT46ZnNuuWkQJrL+DJ7PjGw== X-Received: by 2002:a05:6a00:1253:b0:51c:e1e:9990 with SMTP id u19-20020a056a00125300b0051c0e1e9990mr7990466pfi.5.1655266677429; Tue, 14 Jun 2022 21:17:57 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:73fa:a66c:4a7a:d736]) by smtp.gmail.com with ESMTPSA id v5-20020a170902e8c500b001675d843332sm8112762plg.63.2022.06.14.21.17.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jun 2022 21:17:56 -0700 (PDT) Date: Tue, 14 Jun 2022 21:17:53 -0700 From: Fangrui Song To: libc-alpha@sourceware.org, Szabolcs Nagy Subject: Re: [PATCH v4] elf: Remove ELF_RTYPE_CLASS_EXTERN_PROTECTED_DATA Message-ID: <20220615041753.pbmebchzmyyc7f7w@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: <20220608171021.3150916-1-maskray@google.com> 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 04:18:02 -0000 On 2022-06-08, 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. > >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 Does this look good now? We now always report the diagnostic for a copy relocation/canonical PLT entry on a protected symbol. https://sourceware.org/pipermail/libc-alpha/2022-June/139733.html (https://sourceware.org/git/?p=glibc.git;a=commit;h=7374c02b683b7110b853a32496a619410364d70b)