From owner-freebsd-threads@FreeBSD.ORG Fri Nov 21 20:32:34 2014 Return-Path: Delivered-To: freebsd-threads@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 889C02F2; Fri, 21 Nov 2014 20:32:34 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 1047F874; Fri, 21 Nov 2014 20:32:33 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id sALKWSZj068737 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 21 Nov 2014 22:32:28 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua sALKWSZj068737 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id sALKWSDh068736; Fri, 21 Nov 2014 22:32:28 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 21 Nov 2014 22:32:27 +0200 From: Konstantin Belousov To: Eygene Ryabinkin Subject: Re: [CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case Message-ID: <20141121203227.GS17068@kib.kiev.ua> References: <20141120194925.GK17068@kib.kiev.ua> <20141121165658.GP17068@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: davidxu@FreeBSD.org, freebsd-threads@FreeBSD.org X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Nov 2014 20:32:34 -0000 On Fri, Nov 21, 2014 at 10:55:55PM +0300, Eygene Ryabinkin wrote: > Fri, Nov 21, 2014 at 06:56:58PM +0200, Konstantin Belousov wrote: > > On Fri, Nov 21, 2014 at 07:07:14PM +0300, Eygene Ryabinkin wrote: > > > So, it looks like a kernel bug (since if we request SA_SIGINFO, we > > > should get the proper handler to be called even for the SA_RESETHAND > > > case). I see two possibilities: > > > > > > - invoke SA_RESETHAND processing inside mach-dependent code; that's > > > a kind of ugly and makes mach-specific code to deal with the > > > generic signal handling logics; > > > > > > - pass information about SA_SIGINFO "out-of-band" (not in > > > ps->ps_siginfo). > > > > > > We can't postpone sigdflt() to be called after signal being delivered, > > > since spec requires it to be done before calling user-space handler. > > > > No, userspace is not called from sv_sendsig(), it simply cannot work > > because nested signals would cause nesting on the kernel stack frame. > > sv_sendsig() only prepares the usermode signal stack frame and arranges > > the saved thread CPU state so that on return from kernel to usermode, > > the signal handler is executed. > > So, if we rearrange calls to sv_sendsig() and sigdflt() there can't be > cases when the process being signalled will pick the signal before > sigdflt() will be called? Just because XSI wants that handler reset > and clearing of SIGINFO happen before handler execution, > http://pubs.opengroup.org/onlinepubs/009695399/functions/sigaction.html > So, I went into more trouble of not touching the order. We do not return into usermode in random places of the kernel. As I explained, signal delivery is done by arranging normal return to usermode to effectively return to new signal frame. > > > And, amusingly, the only thing which sv_sendsig() methods need and which > > is also touched by sigdflt(), is ps_siginfo. Simply rearranging the > > order of calls should be enough. I put the patch at the end of message, > > it worked for me. > > If there is no change in behaviour that will arise from rearranging > the order of calls to mach-dependent and mach-independent code, > I'd go a bit firther and unify some repeated code, > http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-SIGINFO-processing-with-RESETHAND-v2.diff > > Works for me too, just tested with the same Squid installation. This looks correct, but is much more delicate change. In particular, the signal mask copied to usermode as part of ucontext, for restoration at sigreturn(2), seems to be correct in both cases, but in the postsig() case, where sendsig_common() is put after sv_sendsig() call, it depends on the returnmask calculation. Can you test that signal mask after signal return is correct with your patch ? Two other notes about your patch, which should be changed before it is committable. First, the name sendsig_common() is not telling anything. Might be, rename the function to postsig_sigprop() or like. In the same vein, comment is too ambitious for small helper. This is not The common code, just a helper to handle thread signal mask and several other details of delivery. Just specifying that this is the thing to call after sysent->sv_sendsig() to arrange for proper bookkeeping, is enough. Second, function needs asserts that process lock and sigact mutex (ps_mtx) are locked.