From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com [IPv6:2607:f8b0:4864:20::22f]) by sourceware.org (Postfix) with ESMTPS id 198913857830 for ; Tue, 13 Apr 2021 14:03:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 198913857830 Received: by mail-oi1-x22f.google.com with SMTP id a21so4009212oib.10 for ; Tue, 13 Apr 2021 07:03:10 -0700 (PDT) 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=m0mnJx01lKJrbYAdg9uNX6ax7prv9K4GNr4kVJr1THA=; b=dkpzb0bxWIc29xBSrd6k/zWyHgCEyYXYMaw/f0vkCwyLhxNsXt2YLBOgF8B4jPsR2p 5C42eaGnc+7uCygVdxRNjYghgGwPNO9ZRT9IXWcDny3h+mjfQZKCFQEju3IZn70VPmMA UYTEmvh1m1m0uzzv7SxGLBtpVBDIjdLiB1lkeEYFaOe5Yxig80UpVG/80yBc0Etx91T7 1FzYuw/TtN/q3BsbHBJUWXOL5p+ivDE7S00HFK8PdF1XC1KWUa/IY5Ff9HV8smiqsd3D upSW7h+W0Zbg9IA22w00iERwEDVaafsXHUFM757xmwaYmb35mfharjNmLFUq+6uysb+q LniQ== X-Gm-Message-State: AOAM531CXfi5PzQQfmc15SFRiBBhqr15brOVkNd+0v1xth3lpnTX8Inj 3U6iW46VkTKkctPM59lSrvfEFi/sTABdci8hw5E= X-Google-Smtp-Source: ABdhPJxQGUzvrtRwe8/SbfEKKhgSAUyFPUh3dqy52n12O6T+367a4CSuFAGJTDXwcw2mvayUcEpBpKBdh/MMrE/qPuY= X-Received: by 2002:aca:b208:: with SMTP id b8mr114469oif.79.1618322589401; Tue, 13 Apr 2021 07:03:09 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "H.J. Lu" Date: Tue, 13 Apr 2021 07:02:33 -0700 Message-ID: Subject: Re: [PATCH v2 10/14] i386: Avoid lazy relocation of tlsdesc [BZ #27137] To: Szabolcs Nagy Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3034.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, 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, 13 Apr 2021 14:03:11 -0000 On Tue, Apr 13, 2021 at 2:32 AM Szabolcs Nagy via Libc-alpha wrote: > > Lazy tlsdesc relocation is racy because the static tls optimization and > tlsdesc management operations are done without holding the dlopen lock. > > This similar to the commit b7cf203b5c17dd6d9878537d41e0c7cc3d270a67 > for aarch64, but it fixes a different race: bug 27137. > > On i386 the code is a bit more complicated than on x86_64 because both > rel and rela relocs are supported. > --- > sysdeps/i386/dl-machine.h | 76 ++++++++++++++++++--------------------- > 1 file changed, 34 insertions(+), 42 deletions(-) > > diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h > index 23e9cc3bfb..590b41d8d7 100644 > --- a/sysdeps/i386/dl-machine.h > +++ b/sysdeps/i386/dl-machine.h > @@ -688,50 +688,32 @@ elf_machine_lazy_rel (struct link_map *map, > } > else if (__glibc_likely (r_type == R_386_TLS_DESC)) > { > - struct tlsdesc volatile * __attribute__((__unused__)) td = > - (struct tlsdesc volatile *)reloc_addr; > - > - /* Handle relocations that reference the local *ABS* in a simple > - way, so as to preserve a potential addend. */ > - if (ELF32_R_SYM (reloc->r_info) == 0) > - td->entry = _dl_tlsdesc_resolve_abs_plus_addend; > - /* Given a known-zero addend, we can store a pointer to the > - reloc in the arg position. */ > - else if (td->arg == 0) > - { > - td->arg = (void*)reloc; > - td->entry = _dl_tlsdesc_resolve_rel; > - } > - else > - { > - /* We could handle non-*ABS* relocations with non-zero addends > - by allocating dynamically an arg to hold a pointer to the > - reloc, but that sounds pointless. */ > - const Elf32_Rel *const r = reloc; > - /* The code below was borrowed from elf_dynamic_do_rel(). */ > - const ElfW(Sym) *const symtab = > - (const void *) D_PTR (map, l_info[DT_SYMTAB]); > + const Elf32_Rel *const r = reloc; > + /* The code below was borrowed from elf_dynamic_do_rel(). */ > + const ElfW(Sym) *const symtab = > + (const void *) D_PTR (map, l_info[DT_SYMTAB]); > > + /* Always initialize TLS descriptors completely at load time, in > + case static TLS is allocated for it that requires locking. */ > # ifdef RTLD_BOOTSTRAP > - /* The dynamic linker always uses versioning. */ > - assert (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL); > + /* The dynamic linker always uses versioning. */ > + assert (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL); > # else > - if (map->l_info[VERSYMIDX (DT_VERSYM)]) > + if (map->l_info[VERSYMIDX (DT_VERSYM)]) > # endif > - { > - const ElfW(Half) *const version = > - (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); > - ElfW(Half) ndx = version[ELFW(R_SYM) (r->r_info)] & 0x7fff; > - elf_machine_rel (map, r, &symtab[ELFW(R_SYM) (r->r_info)], > - &map->l_versions[ndx], > - (void *) (l_addr + r->r_offset), skip_ifunc); > - } > + { > + const ElfW(Half) *const version = > + (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); > + ElfW(Half) ndx = version[ELFW(R_SYM) (r->r_info)] & 0x7fff; > + elf_machine_rel (map, r, &symtab[ELFW(R_SYM) (r->r_info)], > + &map->l_versions[ndx], > + (void *) (l_addr + r->r_offset), skip_ifunc); > + } > # ifndef RTLD_BOOTSTRAP > - else > - elf_machine_rel (map, r, &symtab[ELFW(R_SYM) (r->r_info)], NULL, > - (void *) (l_addr + r->r_offset), skip_ifunc); > + else > + elf_machine_rel (map, r, &symtab[ELFW(R_SYM) (r->r_info)], NULL, > + (void *) (l_addr + r->r_offset), skip_ifunc); > # endif > - } > } > else if (__glibc_unlikely (r_type == R_386_IRELATIVE)) > { > @@ -758,11 +740,21 @@ elf_machine_lazy_rela (struct link_map *map, > ; > else if (__glibc_likely (r_type == R_386_TLS_DESC)) > { > - struct tlsdesc volatile * __attribute__((__unused__)) td = > - (struct tlsdesc volatile *)reloc_addr; > + const Elf_Symndx symndx = ELFW (R_SYM) (reloc->r_info); > + const ElfW (Sym) *symtab = (const void *)D_PTR (map, l_info[DT_SYMTAB]); > + const ElfW (Sym) *sym = &symtab[symndx]; > + const struct r_found_version *version = NULL; > + > + if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL) > + { > + const ElfW (Half) *vernum = > + (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]); > + version = &map->l_versions[vernum[symndx] & 0x7fff]; > + } > > - td->arg = (void*)reloc; > - td->entry = _dl_tlsdesc_resolve_rela; > + /* Always initialize TLS descriptors completely at load time, in > + case static TLS is allocated for it that requires locking. */ > + elf_machine_rela (map, reloc, sym, version, reloc_addr, skip_ifunc); > } > else if (__glibc_unlikely (r_type == R_386_IRELATIVE)) > { > -- > 2.17.1 > LGTM. Thanks. -- H.J.