From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id CBDDA3858D20 for ; Fri, 14 Apr 2023 17:33:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CBDDA3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pnNIl-00040O-3u; Fri, 14 Apr 2023 13:33:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=In-Reply-To:MIME-Version:References:Subject:To:From: Date; bh=uIXlOPq4eDqumKuou6dapMBV/4V3DoBknUcATQ8pWCc=; b=hxwYbyu7zpqHy0Jf4GNX TvGDhfdjLO+7A3WnAhKW50udZwFr9z0eEtQ9xtRfQsrE2xzLapcy4LyIpd3HtfLpcBqFNd096Jb9p 3/JxG5vlrKwYKh+LzE5ZVOftWFIC1uz3vgpBcRf8KqeeiUy3QMqy+N54Za3ir29fJX3GgkIYDNkSn lIW6PNJAGhmQP/MUMELFHZGW//JbnfbGenna5hZqwf0nxqfTYprvWKuJmc+RC2cyLMpMR4vZJSJ5V jtpRX5uGNlQBN4T+cj4d/SFN2UpBqSK1qJhJfxHczTZjU/9cj6jHOH5b3PyvFxyOGe4LbnxEAFcBE s/j+8yecXwW8KA==; Received: from [2a01:cb19:4a:a400:de41:a9ff:fe47:ec49] (helo=begin) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pnNIk-0004g8-OK; Fri, 14 Apr 2023 13:33:34 -0400 Received: from samy by begin with local (Exim 4.96) (envelope-from ) id 1pnNIi-00AKjx-2O; Fri, 14 Apr 2023 19:33:32 +0200 Date: Fri, 14 Apr 2023 19:33:32 +0200 From: Samuel Thibault To: Sergey Bugaev Cc: libc-alpha@sourceware.org, bug-hurd@gnu.org Subject: Re: [RFC PATCH glibc v2 26/34] hurd: Remove __hurd_local_reply_port Message-ID: <20230414173332.afm47w6clabzklex@begin> Mail-Followup-To: Sergey Bugaev , libc-alpha@sourceware.org, bug-hurd@gnu.org References: <20230411201845.oias7lryrvm3cck7@begin> <20230413115812.267158-1-bugaevc@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230413115812.267158-1-bugaevc@gmail.com> Organization: I am not organized User-Agent: NeoMutt/20170609 (1.8.3) X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_PASS,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: Applied with fixing the __mig_dealloc_reply_port(NULL) cases, thanks! Sergey Bugaev, le jeu. 13 avril 2023 14:58:12 +0300, a ecrit: > Now that the signal code no longer accesses it, the only real user of it > was mig-reply.c, so move the logic for managing the port there. > > If we're in SHARED and outside of rtld, we know that __LIBC_NO_TLS () > always evaluates to 0, and a TLS reply port will always be used, not > __hurd_reply_port0. Still, the compiler does not see that > __hurd_reply_port0 is never used due to its address being taken. To deal > with this, explicitly compile out __hurd_reply_port0 when we know we > won't use it. > > Also, instead of accessing the port via THREAD_SELF->reply_port, this > uses THREAD_GETMEM and THREAD_SETMEM directly, avoiding possible > miscompilations. > > Signed-off-by: Sergey Bugaev > --- > Changes since v1: > * Only deallocate the port in __mig_dealloc_reply_port if it's the same > as the stored port, because of signals. > * Add a comment detailing the reasoning. > * Add two assertions: that ARG and PORT are exactly the same (unless PORT > has been invalidated), and that we succeed deallocating PORT. > > NOTE: Completely untested! Do your own testing! You have been warned! > > hurd/hurd/threadvar.h | 7 ---- > sysdeps/mach/hurd/mig-reply.c | 70 +++++++++++++++++++++++++++++------ > 2 files changed, 58 insertions(+), 19 deletions(-) > > diff --git a/hurd/hurd/threadvar.h b/hurd/hurd/threadvar.h > index f5c6a278..c476d988 100644 > --- a/hurd/hurd/threadvar.h > +++ b/hurd/hurd/threadvar.h > @@ -29,11 +29,4 @@ > extern unsigned long int __hurd_sigthread_stack_base; > extern unsigned long int __hurd_sigthread_stack_end; > > -/* Store the MiG reply port reply port until we enable TLS. */ > -extern mach_port_t __hurd_reply_port0; > - > -/* This returns either the TLS reply port variable, or a single-thread variable > - when TLS is not initialized yet. */ > -#define __hurd_local_reply_port (*(__LIBC_NO_TLS () ? &__hurd_reply_port0 : &THREAD_SELF->reply_port)) > - > #endif /* hurd/threadvar.h */ > diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c > index 33ff260e..3fdee80e 100644 > --- a/sysdeps/mach/hurd/mig-reply.c > +++ b/sysdeps/mach/hurd/mig-reply.c > @@ -17,22 +17,46 @@ > > #include > #include > -#include > +#include > > /* These functions are called by MiG-generated code. */ > > +#if !defined (SHARED) || IS_IN (rtld) > mach_port_t __hurd_reply_port0; > +#endif > + > +static mach_port_t > +get_reply_port (void) > +{ > +#if !defined (SHARED) || IS_IN (rtld) > + if (__LIBC_NO_TLS ()) > + return __hurd_reply_port0; > +#endif > + return THREAD_GETMEM (THREAD_SELF, reply_port); > +} > + > +static void > +set_reply_port (mach_port_t port) > +{ > +#if !defined (SHARED) || IS_IN (rtld) > + if (__LIBC_NO_TLS ()) > + __hurd_reply_port0 = port; > + else > +#endif > + THREAD_SETMEM (THREAD_SELF, reply_port, port); > +} > > /* Called by MiG to get a reply port. */ > mach_port_t > __mig_get_reply_port (void) > { > - if (__hurd_local_reply_port == MACH_PORT_NULL > - || (&__hurd_local_reply_port != &__hurd_reply_port0 > - && __hurd_local_reply_port == __hurd_reply_port0)) > - __hurd_local_reply_port = __mach_reply_port (); > - > - return __hurd_local_reply_port; > + mach_port_t port = get_reply_port (); > + if (__glibc_unlikely (port == MACH_PORT_NULL)) > + { > + port = __mach_reply_port (); > + set_reply_port (port); > + } > + return port; > } > weak_alias (__mig_get_reply_port, mig_get_reply_port) > libc_hidden_def (__mig_get_reply_port) > @@ -41,12 +65,34 @@ libc_hidden_def (__mig_get_reply_port) > void > __mig_dealloc_reply_port (mach_port_t arg) > { > - mach_port_t port = __hurd_local_reply_port; > - __hurd_local_reply_port = MACH_PORT_NULL; /* So the mod_refs RPC won't use it. */ > + error_t err; > + mach_port_t port = get_reply_port (); > + > + set_reply_port (MACH_PORT_NULL); /* So the mod_refs RPC won't use it. */ > + > + /* Normally, ARG should be the same as PORT that we store. However, if a > + signal has interrupted the RPC, the stored PORT has been deallocated and > + reset to MACH_PORT_NULL (or possibly MACH_PORT_DEAD). In this case the > + MIG routine still has the old name, which it passes to us here. We must > + not deallocate (or otherwise touch) it, since it may be already allocated > + to another port right. Fortunately MIG itself doesn't do anything with > + the reply port on errors either, other than immediately calling this > + function. > + > + And so: > + 1. Assert that things are sane, i.e. and PORT is either invalid or same > + as ARG. > + 2. Only deallocate the name if our stored PORT still names it. In that > + case we're sure the right has not been deallocated / the name reused. > + */ > + > + if (!MACH_PORT_VALID (port)) > + return; > + assert (port == arg); > > - if (MACH_PORT_VALID (port)) > - __mach_port_mod_refs (__mach_task_self (), port, > - MACH_PORT_RIGHT_RECEIVE, -1); > + err = __mach_port_mod_refs (__mach_task_self (), port, > + MACH_PORT_RIGHT_RECEIVE, -1); > + assert_perror (err); > } > weak_alias (__mig_dealloc_reply_port, mig_dealloc_reply_port) > libc_hidden_def (__mig_dealloc_reply_port) > -- > 2.39.2 > -- Samuel --- Pour une évaluation indépendante, transparente et rigoureuse ! Je soutiens la Commission d'Évaluation de l'Inria.