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 BDDE93858D38 for ; Mon, 10 Apr 2023 22:03:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BDDE93858D38 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 1plzbW-0001zV-5K; Mon, 10 Apr 2023 18:03:14 -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=zhmpC/9YnZgMCKkm4jMDdO/hJIubarV+lKSWtLUrZkw=; b=VYBUS1yPv3yAqP5KMCoc 6jIiZetPBuI5tCWxlRvQ1OeaUQfO9u4l7VaCDd65rArtNLlbjf7jno3SUtjZVHvvtBlJxgraiVtqA E3o5QA7xRrz7IKiHNweJpRED5jHPQslf/2RV7anSBaC5XOyhZAN581QffX/nZeWcmVW13y3MzITHo 0SbEXflG9YtRLEVGjGnLyOZBFVb+lS5BN45O1+WY1n8cawGmdsi7ikQmA48EiAORe2xZ1Jqq9ZRTN TqjUwqv6ZvOnErTrlJOu8s5bsfB4M968aSgMcSUg0Y2PQ+POJaMPddjsRlqFmeu4jeLELhNs9pSQf LonEhRL1eTsiHQ==; 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 1plzbV-0004Ha-Bf; Mon, 10 Apr 2023 18:03:13 -0400 Received: from samy by begin with local (Exim 4.96) (envelope-from ) id 1plzbT-000qN2-1W; Tue, 11 Apr 2023 00:03:11 +0200 Date: Tue, 11 Apr 2023 00:03:11 +0200 From: Samuel Thibault To: Sergey Bugaev Cc: libc-alpha@sourceware.org, bug-hurd@gnu.org Subject: Re: [RFC PATCH glibc 25/34] hurd: Improve reply port handling when exiting signal handlers Message-ID: <20230410220311.fecuaj4gicx3hh5l@begin> Mail-Followup-To: Sergey Bugaev , libc-alpha@sourceware.org, bug-hurd@gnu.org References: <20230319151017.531737-1-bugaevc@gmail.com> <20230319151017.531737-26-bugaevc@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230319151017.531737-26-bugaevc@gmail.com> Organization: I am not organized User-Agent: NeoMutt/20170609 (1.8.3) X-Spam-Status: No, score=-12.5 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 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, thanks! Sergey Bugaev, le dim. 19 mars 2023 18:10:08 +0300, a ecrit: > NOTE: I don't really understand why sigunwind wants to destroy both its > current reply port and user's reply port. Prior to commit > fb304035c41c7ee2afede51e5e8568974549ba5e, it was *restoring* the user's > reply port, in which case it actually made sense to destroy the current > reply port. Post-fb304035c41c7ee2afede51e5e8568974549ba5e, wouldn't it > be better to just keep using the current reply port, destroying the > user's one? We could try that, yes. I tend to be very cautious with reply port reuse since it can confuse servers a lot when e.g. interrupted, so it's generally safer not to try to reuse them. > hurd/sigunwind.c | 24 +++++++++++------------- > sysdeps/mach/hurd/i386/sigreturn.c | 21 +++++---------------- > 2 files changed, 16 insertions(+), 29 deletions(-) > > diff --git a/hurd/sigunwind.c b/hurd/sigunwind.c > index edd40b14..399e6900 100644 > --- a/hurd/sigunwind.c > +++ b/hurd/sigunwind.c > @@ -18,7 +18,6 @@ > > #include > #include > -#include > #include > #include > #include > @@ -39,19 +38,18 @@ _hurdsig_longjmp_from_handler (void *data, jmp_buf env, int val) > { > /* Destroy the MiG reply port used by the signal handler, and restore > the reply port in use by the thread when interrupted. */ > - mach_port_t *reply_port = &__hurd_local_reply_port; > - if (*reply_port) > - { > - mach_port_t port = *reply_port; > - /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port > - not to get another reply port, but avoids mig_dealloc_reply_port > - trying to deallocate it after the receive fails (which it will, > - because the reply port will be bogus, regardless). */ > - *reply_port = MACH_PORT_DEAD; > - __mach_port_destroy (__mach_task_self (), port); > - } > + mach_port_t reply_port = THREAD_GETMEM (THREAD_SELF, reply_port); > + /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to > + get another reply port, but avoids mig_dealloc_reply_port trying to > + deallocate it after the receive fails (which it will, because the > + reply port will be bogus, regardless). */ > + THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD); > + if (MACH_PORT_VALID (reply_port)) > + __mach_port_mod_refs (__mach_task_self (), reply_port, > + MACH_PORT_RIGHT_RECEIVE, -1); > if (scp->sc_reply_port) > - __mach_port_destroy (__mach_task_self (), scp->sc_reply_port); > + __mach_port_mod_refs (__mach_task_self (), scp->sc_reply_port, > + MACH_PORT_RIGHT_RECEIVE, -1); > } > > __spin_lock (&ss->lock); > diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c > index db1a01f3..29c9629f 100644 > --- a/sysdeps/mach/hurd/i386/sigreturn.c > +++ b/sysdeps/mach/hurd/i386/sigreturn.c > @@ -19,7 +19,6 @@ register int *sp asm ("%esp"); > > #include > #include > -#include > #include > #include > #include > @@ -59,7 +58,7 @@ __sigreturn (struct sigcontext *scp) > { > struct hurd_sigstate *ss; > struct hurd_userlink *link = (void *) &scp[1]; > - mach_port_t *reply_port; > + mach_port_t reply_port; > > if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)) > { > @@ -101,20 +100,10 @@ __sigreturn (struct sigcontext *scp) > > /* Destroy the MiG reply port used by the signal handler, and restore the > reply port in use by the thread when interrupted. */ > - reply_port = &__hurd_local_reply_port; > - if (*reply_port) > - { > - mach_port_t port = *reply_port; > - > - /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to > - get another reply port, but avoids mig_dealloc_reply_port trying to > - deallocate it after the receive fails (which it will, because the > - reply port will be bogus, whether we do this or not). */ > - *reply_port = MACH_PORT_DEAD; > - > - __mach_port_destroy (__mach_task_self (), port); > - } > - *reply_port = scp->sc_reply_port; > + reply_port = THREAD_GETMEM (THREAD_SELF, reply_port); > + THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port); > + __mach_port_mod_refs (__mach_task_self (), reply_port, > + MACH_PORT_RIGHT_RECEIVE, -1); > > if (scp->sc_fpused) > /* Restore the FPU state. Mach conveniently stores the state > -- > 2.39.2 > -- Samuel --- Pour une évaluation indépendante, transparente et rigoureuse ! Je soutiens la Commission d'Évaluation de l'Inria.