Date: Thu, 27 Nov 2014 01:12:31 +0300 From: Eygene Ryabinkin <rea@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: davidxu@FreeBSD.org, freebsd-threads@FreeBSD.org Subject: Re: [CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case Message-ID: <jFEd1ENuyp8%2BKOBFhFLA4ucYsq0@btiPEv4nTmhOyKpuIO1ud%2BMiSV0> In-Reply-To: <20141121203227.GS17068@kib.kiev.ua> References: <KBP9w7ztz0PP7uy58G1ODYb/voI@1d%2BaJAniZP50FCDdGj54nd51%2Bks> <20141120194925.GK17068@kib.kiev.ua> <ngq2VsHPChtgNNL/z%2BlRaAXEeTI@1d%2BaJAniZP50FCDdGj54nd51%2Bks> <20141121165658.GP17068@kib.kiev.ua> <cxMeZKcIhV0OesPiyimV3oGOays@W98LlNNy5PXu%2BS3VxPCL6cVmtdM> <20141121203227.GS17068@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--DocE+STaALJfprDB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Konstantin, good day. Fri, Nov 21, 2014 at 10:32:27PM +0200, Konstantin Belousov wrote: > On Fri, Nov 21, 2014 at 10:55:55PM +0300, Eygene Ryabinkin wrote: > > 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-process= ing-with-RESETHAND-v2.diff > >=20 > > 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. Since in original (unmodified) code returnmask is calculated before the call to kern_sigprocmask(), {{{ if (td->td_pflags & TDP_OLDMASK) { returnmask =3D td->td_oldsigmask; td->td_pflags &=3D ~TDP_OLDMASK; } else returnmask =3D td->td_sigmask; mask =3D ps->ps_catchmask[_SIG_IDX(sig)]; if (!SIGISMEMBER(ps->ps_signodefer, sig)) SIGADDSET(mask, sig); kern_sigprocmask(td, SIG_BLOCK, &mask, NULL, SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED); if (SIGISMEMBER(ps->ps_sigreset, sig)) sigdflt(ps, sig); td->td_ru.ru_nsignals++; if (p->p_sig =3D=3D sig) { p->p_code =3D 0; p->p_sig =3D 0; } (*p->p_sysent->sv_sendsig)(action, &ksi, &returnmask); }}} and kern_sigprocmask() only touches td->td_sigmask, such change should be safe, because mach-dependent handlers do not use or manipulate td_sigmask. This looks logical, because we want to pass current original signal mask for the thread to sv_sendsig() to make it to be restored after signal handler's work. So returnmask is to be calculated before mangling and should not depend on it. > Can you test that signal mask after signal return is correct with your > patch? Tested with the following code: http://codelabs.ru/fbsd/patches/libthr/kern-sig-test-sigmask-restoration.c All tested masks are restored properly both for modified and unmodified kernels. > 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. >=20 > 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. Changed it to sendsig_adjust that should carry more sense. Also modified comment. > Second, function needs asserts that process lock and sigact mutex > (ps_mtx) are locked. Added them too. New patch is at http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-SIGINFO-processing-= with-RESETHAND-v3.diff Thanks! --=20 Eygene Ryabinkin ,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] --DocE+STaALJfprDB Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iL4EABEKAGYFAlR2UE9fFIAAAAAALgAoaXNzdWVyLWZwckBub3RhdGlvbnMub3Bl bnBncC5maWZ0aGhvcnNlbWFuLm5ldDgyRkUwNkJDRDQ5N0MwREU0OUVDNEZGMDE2 QUY5RUFFODE1MkVDRkIACgkQFq+eroFS7Pv3mgD/ewMG9CFOtosyKsUxdNk5ooZ1 UpypzuLV4ZBzyWT7ZQMA/1XU/oFd/jwkQYTSZnfBY4zXZeqwWyVhKuNNKOAX9GyZ =Dqzb -----END PGP SIGNATURE----- --DocE+STaALJfprDB--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?jFEd1ENuyp8%2BKOBFhFLA4ucYsq0>