From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from brightrain.aerifal.cx (brightrain.aerifal.cx [216.12.86.13]) by sourceware.org (Postfix) with ESMTPS id 8BF1A3858C74 for ; Tue, 1 Feb 2022 15:21:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8BF1A3858C74 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=libc.org Authentication-Results: sourceware.org; spf=none smtp.mailfrom=libc.org Date: Tue, 1 Feb 2022 10:21:12 -0500 From: Rich Felker To: Florian Weimer Cc: libc-alpha@sourceware.org Subject: Re: [PATCH v2 0/8] Extensible rseq integration Message-ID: <20220201152112.GL7074@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 01 Feb 2022 15:21:15 -0000 On Tue, Dec 07, 2021 at 01:59:26PM +0100, Florian Weimer via Libc-alpha wrote: > This series integrates the previous posted v2 for . > > It incorporates Mathieu's and Paul E. McKenney suggestion to use a > volatile read for rseq_abi.cpu_id access, using a new > THREAD_GETMEM_VOLATILE macro. > > The last patch in the series makes rseq registration consistent across > threads. > > Florian Weimer (8): > nptl: Add for defining __thread_pointer > nptl: Introduce for THREAD_* accessors > nptl: Introduce THREAD_GETMEM_VOLATILE > nptl: Add rseq registration > Linux: Use rseq to accelerate sched_getcpu > nptl: Add glibc.pthread.rseq tunable to control rseq registration > nptl: Add public rseq symbols and > nptl: rseq failure after registration on main thread is fatal I'm sorry for bringing this up so late; I wasn't aware that redesign of the rseq ABI was taking place. I wish this had been discussed in a cross-libc venue, since, in its current form, I don't think the ABI is suitable for inclusion in, or use as a third-party library with, musl. The most pressing issue I see is that it does not admit lazy registration, which precludes it being implemented outside of libc (because it has to hook into pthread_create) and imposes runtime cost on programs which do not use it. RSEQ_CPU_ID_UNINITIALIZED exists to inform the application about an uninitialized state, but the application has no way to request an attempt at registration upon seeing it. I think that would be easy to add. Basically it's just making the syscall, which a consumer of the ABI could in theory do itself, but it's probably best not to have it do that and instead have registration mediated through the ABI/through libc. Related to this, if rseq is implemented outside of libc, I'm not sure if there's a safe way to ensure it's unregistered prior to thread exit. It may already be possible but I haven't sufficiently convinced myself. On another issue, while this isn't entirely a show-stopper, I'm not a fan of requiring constant __rseq_offset. This comes across as an instance-specific hack to make up for GD TLS being slow, when we already have a fully general solution to that which isn't being deployed: TLSDESC. As it stands in the current ABI, whatever library is providing rseq must be present at application startup; it can't be dlopened. And due to the ABI this applies *even if* we just wanted to make rseq always-fail in that case. The ABI simply doesn't admit not having memory pre-reserved for every thread (note: the size is something like a +30% increase to musl's per-thread memory usage and will surely increase over time, which is a lot for something we don't expect the vast majority of applications to use). One minor and hopefully non-controversial declared-ABI issue I see is that the __rseq_offset etc. objects are declared const, with a pre-relro access hack used to modify them at runtime. This is incompatible with LTO and static linking. If protecting them is desired, they should be declared non-const but live in non-modifiable memory, like string literals do. Otherwise a static linking LTO compiler is free to copy the initial values directly into code. I'm not sure what the right thing to do on the verge of release is. If it were my choice, I would hold it back and wait until it was better reviewed and these issues worked out before making it public API/ABI, but I don't know what glibc's constraints here are and how to best weigh them against the ability to revise this ABI after release. Most of these things I think *are* of the sort that can be fixed in non-breaking ways, except that applications written to the current version might need to adjust before they can use a version of the API/ABI we'd be willing to adopt in musl. Rich