From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpout.efficios.com (smtpout.efficios.com [IPv6:2607:5300:203:b2ee::31e5]) by sourceware.org (Postfix) with ESMTPS id 3B7A53858D33 for ; Tue, 6 Feb 2024 16:41:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3B7A53858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=efficios.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=efficios.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 3B7A53858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:5300:203:b2ee::31e5 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707237680; cv=none; b=EkK7TToQoecn59VaBBtsx+WTmd8zBkUzFZp8j8bvTSDJPRq9lp8FFwV3aHYeysmm0ZTc9UW6f0wJ3pI/vxmtpYxlp1omgeeCw0a9EfdrlYh16ZxRlZ2IWgFn8vzHd8ML0Lt3AIR0oUzkpmXCY/CDdSRQbJCnszpcyGoNsUHGNVc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1707237680; c=relaxed/simple; bh=NwMPDjaTcflQCwtZHFRGij3CPzFVG7Nr53aiGCik5e0=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=oMXbUCViyJDuSYIWBBl39XxepYSoRLSKU+orEFxeSKgjFT1XqIj9SXtOe2O2kB6SdSBR3Kjxp2hYqDN+tvhCw9i69G7fHFBIw4bsYu13Ov8ajUPNPMP93lODX8tdMi/eu7xfih545HGv2vx1TGrA8BY3VVzk1d844aTTl1O5r40= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=efficios.com; s=smtpout1; t=1707237677; bh=NwMPDjaTcflQCwtZHFRGij3CPzFVG7Nr53aiGCik5e0=; h=Date:Subject:To:References:From:Cc:In-Reply-To:From; b=eejOQnYyJOudJQCBESpd1bV1JiiqOgrd103YR/ZHvH9FIoaU892g3bxX2utICcefk re86+xctFIvHyxsZ2VNsoI4+/ApBqrVgJIEKhEpXFywGfzpUjLPP9yoABrngGlkhp9 ckBxD83E5jiB43sDnppfyOwh7N1Jyzi8YgyW1KzoOjyifQ6iQJE14S41ijWtOhVRrk WYMctPgHaD0VOHTJB8xdfZDFAHjjR6OC/5GplHYRWEvyfw9no9U1jPVAlew8br/j4Z s9i5NjQ9lS44s5Hwir+hhFm3qtywXWPf8Hm8k6oMvzvCVSreUv2VEQSpfTn1fCTf6U ujXiRFTsi1zzg== Received: from [172.16.0.134] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) by smtpout.efficios.com (Postfix) with ESMTPSA id 4TTpsn5fZ7zXGf; Tue, 6 Feb 2024 11:41:17 -0500 (EST) Message-ID: <687999b5-d7e1-4b2b-a9fa-f967ebf174e9@efficios.com> Date: Tue, 6 Feb 2024 11:41:23 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 1/8] nptl: fix potential merge of __rseq_* relro symbols Content-Language: en-US To: Florian Weimer , "carlos@redhat.com" References: <20240206162801.882585-1-mjeanson@efficios.com> <20240206162801.882585-2-mjeanson@efficios.com> From: Mathieu Desnoyers Cc: Michael Jeanson , libc-alpha@sourceware.org In-Reply-To: <20240206162801.882585-2-mjeanson@efficios.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2024-02-06 11:27, Michael Jeanson wrote: > While working on a patch to add support for the extensible rseq ABI, we > came across an issue where a new 'const' variable would be merged with > the existing '__rseq_size' variable. We tracked this to the use of > '-fmerge-all-constants' which allows the compiler to merge identical > constant variables. This means that all 'const' variables in a compile > unit that are of the same size and are initialized to the same value can > be merged. > > In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t' > are both 4 bytes and initialized to 0 which should trigger the merge. > However for reasons we haven't delved into when the attribute 'section > (".data.rel.ro")' is added to the mix, only variables of the same exact > types are merged. As far as we know this behavior is not specified > anywhere and could change with a new compiler version, hence this patch. > > Move the definitions of these variables into an assembler file and add > hidden writable aliases for internal use. This has the added bonus of > removing the asm workaround to set the values on rseq registration. > > Tested on Debian 12 with GCC 12.2. Hi Florian, Just to make sure you don't duplicate this effort: we ended up implementing this fix, even though we originally said we did not intend to. Feedback is welcome! Thanks, Mathieu > > Signed-off-by: Michael Jeanson > Reviewed-by: Mathieu Desnoyers > --- > csu/Makefile | 2 +- > csu/rseq-sizes.sym | 8 +++++ > elf/Makefile | 1 + > elf/dl-rseq-symbols.S | 55 +++++++++++++++++++++++++++++++++++ > sysdeps/nptl/dl-tls_init_tp.c | 14 ++++----- > 5 files changed, 71 insertions(+), 9 deletions(-) > create mode 100644 csu/rseq-sizes.sym > create mode 100644 elf/dl-rseq-symbols.S > > diff --git a/csu/Makefile b/csu/Makefile > index ac05ab24d5..0bf51a0e48 100644 > --- a/csu/Makefile > +++ b/csu/Makefile > @@ -99,7 +99,7 @@ before-compile += $(objpfx)abi-tag.h > generated += abi-tag.h > > # Put it here to generate it earlier. > -gen-as-const-headers += rtld-sizes.sym > +gen-as-const-headers += rtld-sizes.sym rseq-sizes.sym > > # These are the special initializer/finalizer files. They are always the > # first and last file in the link. crti.o ... crtn.o define the global > diff --git a/csu/rseq-sizes.sym b/csu/rseq-sizes.sym > new file mode 100644 > index 0000000000..c959758ff0 > --- /dev/null > +++ b/csu/rseq-sizes.sym > @@ -0,0 +1,8 @@ > +#include > + > +-- > +RSEQ_SIZE_SIZE sizeof (unsigned int) > +RSEQ_SIZE_ALIGN __alignof (unsigned int) > + > +RSEQ_OFFSET_SIZE sizeof (ptrdiff_t) > +RSEQ_OFFSET_ALIGN __alignof (ptrdiff_t) > diff --git a/elf/Makefile b/elf/Makefile > index 5d78b659ce..7d711aedf0 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -73,6 +73,7 @@ dl-routines = \ > dl-origin \ > dl-printf \ > dl-reloc \ > + dl-rseq-symbols \ > dl-runtime \ > dl-scope \ > dl-setup_hash \ > diff --git a/elf/dl-rseq-symbols.S b/elf/dl-rseq-symbols.S > new file mode 100644 > index 0000000000..2d8e88367f > --- /dev/null > +++ b/elf/dl-rseq-symbols.S > @@ -0,0 +1,55 @@ > +/* Define symbols used by rseq. > + Copyright (C) 2024 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 > +#include > + > +/* Some targets define a macro to denote the zero register. */ > +#undef zero > + > +/* Define 2 symbols, __rseq_size is public const and _rseq_size, which is an > + alias of __rseq_size, but hidden and writable for internal use. */ > + > + .globl __rseq_size > + .type __rseq_size, %object > + .size __rseq_size, RSEQ_SIZE_SIZE > + .hidden _rseq_size > + .globl _rseq_size > + .type _rseq_size, %object > + .size _rseq_size, RSEQ_SIZE_SIZE > + .section .data.rel.ro > + .balign RSEQ_SIZE_ALIGN > +__rseq_size: > +_rseq_size: > + .zero RSEQ_SIZE_SIZE > + > +/* Define 2 symbols, __rseq_offset is public const and _rseq_offset, which is an > + alias of __rseq_offset, but hidden and writable for internal use. */ > + > + .globl __rseq_offset > + .type __rseq_offset, %object > + .size __rseq_offset, RSEQ_OFFSET_SIZE > + .hidden _rseq_offset > + .globl _rseq_offset > + .type _rseq_offset, %object > + .size _rseq_offset, RSEQ_OFFSET_SIZE > + .section .data.rel.ro > + .balign RSEQ_OFFSET_ALIGN > +__rseq_offset: > +_rseq_offset: > + .zero RSEQ_OFFSET_SIZE > diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c > index 092c274f36..80eb0107b5 100644 > --- a/sysdeps/nptl/dl-tls_init_tp.c > +++ b/sysdeps/nptl/dl-tls_init_tp.c > @@ -45,8 +45,10 @@ rtld_mutex_dummy (pthread_mutex_t *lock) > #endif > > const unsigned int __rseq_flags; > -const unsigned int __rseq_size attribute_relro; > -const ptrdiff_t __rseq_offset attribute_relro; > + > +/* The variables are in .data.relro but are not yet write-protected. */ > +extern unsigned int _rseq_size attribute_relro attribute_hidden; > +extern ptrdiff_t _rseq_offset attribute_relro attribute_hidden; > > void > __tls_pre_init_tp (void) > @@ -105,10 +107,7 @@ __tls_init_tp (void) > do_rseq = TUNABLE_GET (rseq, int, NULL); > if (rseq_register_current_thread (pd, do_rseq)) > { > - /* We need a writable view of the variables. They are in > - .data.relro and are not yet write-protected. */ > - extern unsigned int size __asm__ ("__rseq_size"); > - size = sizeof (pd->rseq_area); > + _rseq_size = sizeof (pd->rseq_area); > } > > #ifdef RSEQ_SIG > @@ -117,8 +116,7 @@ __tls_init_tp (void) > all targets support __thread_pointer, so set __rseq_offset only > if the rseq registration may have happened because RSEQ_SIG is > defined. */ > - extern ptrdiff_t offset __asm__ ("__rseq_offset"); > - offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); > + _rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); > #endif > } > -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com