From owner-freebsd-current@FreeBSD.ORG Sat Oct 10 14:36:50 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3CF031065672 for ; Sat, 10 Oct 2009 14:36:50 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (skuns.zoral.com.ua [91.193.166.194]) by mx1.freebsd.org (Postfix) with ESMTP id 1BB368FC21 for ; Sat, 10 Oct 2009 14:36:48 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id n9AEahbL091470 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 10 Oct 2009 17:36:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3) with ESMTP id n9AEahRQ082599; Sat, 10 Oct 2009 17:36:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.3/8.14.3/Submit) id n9AEahKq082598; Sat, 10 Oct 2009 17:36:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 10 Oct 2009 17:36:43 +0300 From: Kostik Belousov To: David Xu Message-ID: <20091010143643.GA2259@deviant.kiev.zoral.com.ua> References: <20091001120730.GR3130@deviant.kiev.zoral.com.ua> <20091002201213.GA16633@stack.nl> <20091005192144.GA2259@deviant.kiev.zoral.com.ua> <4AD01ABC.50901@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="jvmCgFKx2lledmPK" Content-Disposition: inline In-Reply-To: <4AD01ABC.50901@freebsd.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-current@freebsd.org, Jilles Tjoelker , Justin Teller Subject: Re: Signals and an exiting thread X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Oct 2009 14:36:50 -0000 --jvmCgFKx2lledmPK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Oct 10, 2009 at 01:25:16PM +0800, David Xu wrote: > Kostik Belousov wrote: >=20 > >I agree that postponing assignment of the thread for signal delivery > >till the actual delivery occurs is the proper fix. I tried to cheat > >in my previous patch. Below is an experimental change that did very > >minimal testing. > > >=20 > Even if the signal is put into process's signal queue, it is still > possible that signal notification is lost because selected thread > exits before seeing it, if other threads are sleeping, they are > not notified, this leaves signal in process queue long time before > it can be delivered to userland. >=20 Agreed. Actually, there is one more race. Namely, when thread enters kernel for executing sigprocmask syscall, but still did not acquired process mutex, some signal may be scheduled for the thread which will block it later while still in kernel, so wakeup is lost again. I did fixed that in later version of the patch, by waking up possible target threads that have newly blocked signals unblocked. Resulting code seems to be relevant for exiting thread as well, where we also shall set sigmask to indicate that thread is not willing (cannot) handle any further signals. Updated patch below. diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 0386fc4..9e61ea8 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -220,6 +220,8 @@ static int sigproptbl[NSIG] =3D { SA_KILL|SA_PROC, /* SIGUSR2 */ }; =20 +static void reschedule_signals(struct proc *p, sigset_t block); + static void sigqueue_start(void) { @@ -581,20 +583,11 @@ void signotify(struct thread *td) { struct proc *p; - sigset_t set; =20 p =3D td->td_proc; =20 PROC_LOCK_ASSERT(p, MA_OWNED); =20 - /* - * If our mask changed we may have to move signal that were - * previously masked by all threads to our sigqueue. - */ - set =3D p->p_sigqueue.sq_signals; - SIGSETNAND(set, td->td_sigmask); - if (! SIGISEMPTY(set)) - sigqueue_move_set(&p->p_sigqueue, &td->td_sigqueue, &set); if (SIGPENDING(td)) { thread_lock(td); td->td_flags |=3D TDF_NEEDSIGCHK | TDF_ASTPENDING; @@ -976,24 +969,28 @@ execsigs(struct proc *p) * Manipulate signal mask. */ int -kern_sigprocmask(td, how, set, oset, old) - struct thread *td; - int how; - sigset_t *set, *oset; - int old; +kern_sigprocmask(struct thread *td, int how, sigset_t *set, sigset_t *oset, + int old) { + sigset_t new_block, oset1; + struct proc *p; int error; =20 - PROC_LOCK(td->td_proc); + p =3D td->td_proc; + PROC_LOCK(p); if (oset !=3D NULL) *oset =3D td->td_sigmask; =20 error =3D 0; + SIGEMPTYSET(new_block); if (set !=3D NULL) { switch (how) { case SIG_BLOCK: SIG_CANTMASK(*set); + oset1 =3D td->td_sigmask; SIGSETOR(td->td_sigmask, *set); + new_block =3D td->td_sigmask; + SIGSETNAND(new_block, oset1); break; case SIG_UNBLOCK: SIGSETNAND(td->td_sigmask, *set); @@ -1001,10 +998,13 @@ kern_sigprocmask(td, how, set, oset, old) break; case SIG_SETMASK: SIG_CANTMASK(*set); + oset1 =3D td->td_sigmask; if (old) SIGSETLO(td->td_sigmask, *set); else td->td_sigmask =3D *set; + new_block =3D td->td_sigmask; + SIGSETNAND(new_block, oset1); signotify(td); break; default: @@ -1012,7 +1012,20 @@ kern_sigprocmask(td, how, set, oset, old) break; } } - PROC_UNLOCK(td->td_proc); + + /* + * The new_block set contains signals that were not previosly + * blocked, but are blocked now. + * + * In case we block any signal that was not previously blocked + * for td, and process has the signal pending, try to schedule + * signal delivery to some thread that does not block the signal, + * possibly waking it up. + */ + if (p->p_numthreads !=3D 1) + reschedule_signals(p, new_block); + + PROC_UNLOCK(p); return (error); } =20 @@ -1985,17 +1998,9 @@ tdsignal(struct proc *p, struct thread *td, int sig,= ksiginfo_t *ksi) KNOTE_LOCKED(&p->p_klist, NOTE_SIGNAL | sig); prop =3D sigprop(sig); =20 - /* - * If the signal is blocked and not destined for this thread, then - * assign it to the process so that we can find it later in the first - * thread that unblocks it. Otherwise, assign it to this thread now. - */ if (td =3D=3D NULL) { td =3D sigtd(p, sig, prop); - if (SIGISMEMBER(td->td_sigmask, sig)) - sigqueue =3D &p->p_sigqueue; - else - sigqueue =3D &td->td_sigqueue; + sigqueue =3D &p->p_sigqueue; } else { KASSERT(td->td_proc =3D=3D p, ("invalid thread")); sigqueue =3D &td->td_sigqueue; @@ -2392,6 +2397,62 @@ stopme: return (td->td_xsig); } =20 +static void +reschedule_signals(struct proc *p, sigset_t block) +{ + struct sigacts *ps; + struct thread *td; + int i; + + PROC_LOCK_ASSERT(p, MA_OWNED); + + ps =3D p->p_sigacts; + for (i =3D 0; !SIGISEMPTY(block); i++) { + if (!SIGISMEMBER(block, i)) + continue; + SIGDELSET(block, i); + if (!SIGISMEMBER(p->p_siglist, i)) + continue; + + td =3D sigtd(p, i, 0); + signotify(td); + mtx_lock(&ps->ps_mtx); + if (p->p_flag & P_TRACED || SIGISMEMBER(ps->ps_sigcatch, i)) + tdsigwakeup(td, i, SIG_CATCH, + (SIGISMEMBER(ps->ps_sigintr, i) ? EINTR : + ERESTART)); + mtx_unlock(&ps->ps_mtx); + } +} + +void +tdsigcleanup(struct thread *td) +{ + struct proc *p; + sigset_t unblocked; + + p =3D td->td_proc; + PROC_LOCK_ASSERT(p, MA_OWNED); + + sigqueue_flush(&td->td_sigqueue); + if (p->p_numthreads =3D=3D 1) + return; + + /* + * Since we cannot handle signals, notify signal post code + * about this by filling the sigmask. + * + * Also, if needed, wake up thread(s) that do not block the + * same signals as the exiting thread, since the thread might + * have been selected for delivery and woken up. + */ + SIGFILLSET(unblocked); + SIGSETNAND(unblocked, td->td_sigmask); + SIGFILLSET(td->td_sigmask); + reschedule_signals(p, unblocked); + +} + /* * If the current process has received a signal (should be caught or cause * termination, should interrupt current syscall), return the signal numbe= r. @@ -2409,8 +2470,9 @@ issignal(struct thread *td, int stop_allowed) { struct proc *p; struct sigacts *ps; + struct sigqueue *queue; sigset_t sigpending; - int sig, prop, newsig; + int sig, prop, newsig, signo; =20 p =3D td->td_proc; ps =3D p->p_sigacts; @@ -2420,6 +2482,7 @@ issignal(struct thread *td, int stop_allowed) int traced =3D (p->p_flag & P_TRACED) || (p->p_stops & S_SIG); =20 sigpending =3D td->td_sigqueue.sq_signals; + SIGSETOR(sigpending, p->p_sigqueue.sq_signals); SIGSETNAND(sigpending, td->td_sigmask); =20 if (p->p_flag & P_PPWAIT) @@ -2440,6 +2503,7 @@ issignal(struct thread *td, int stop_allowed) */ if (SIGISMEMBER(ps->ps_sigignore, sig) && (traced =3D=3D 0)) { sigqueue_delete(&td->td_sigqueue, sig); + sigqueue_delete(&p->p_sigqueue, sig); continue; } if (p->p_flag & P_TRACED && (p->p_flag & P_PPWAIT) =3D=3D 0) { @@ -2452,12 +2516,18 @@ issignal(struct thread *td, int stop_allowed) =20 if (sig !=3D newsig) { ksiginfo_t ksi; + + queue =3D &td->td_sigqueue; /* * clear old signal. * XXX shrug off debugger, it causes siginfo to * be thrown away. */ - sigqueue_get(&td->td_sigqueue, sig, &ksi); + if (sigqueue_get(queue, sig, &ksi) =3D=3D 0) { + queue =3D &p->p_sigqueue; + signo =3D sigqueue_get(queue, sig, &ksi); + KASSERT(signo =3D=3D sig, ("signo !=3D sig")); + } =20 /* * If parent wants us to take the signal, @@ -2472,7 +2542,7 @@ issignal(struct thread *td, int stop_allowed) * Put the new signal into td_sigqueue. If the * signal is being masked, look for other signals. */ - SIGADDSET(td->td_sigqueue.sq_signals, sig); + SIGADDSET(queue->sq_signals, sig); if (SIGISMEMBER(td->td_sigmask, sig)) continue; signotify(td); @@ -2567,6 +2637,7 @@ issignal(struct thread *td, int stop_allowed) return (sig); } sigqueue_delete(&td->td_sigqueue, sig); /* take the signal! */ + sigqueue_delete(&p->p_sigqueue, sig); } /* NOTREACHED */ } @@ -2613,7 +2684,9 @@ postsig(sig) ps =3D p->p_sigacts; mtx_assert(&ps->ps_mtx, MA_OWNED); ksiginfo_init(&ksi); - sigqueue_get(&td->td_sigqueue, sig, &ksi); + if (sigqueue_get(&td->td_sigqueue, sig, &ksi) =3D=3D 0 && + sigqueue_get(&p->p_sigqueue, sig, &ksi) =3D=3D 0) + return; ksi.ksi_signo =3D sig; if (ksi.ksi_code =3D=3D SI_TIMER) itimer_accept(p, ksi.ksi_timerid, &ksi); diff --git a/sys/kern/kern_thr.c b/sys/kern/kern_thr.c index 630069b..3159a91 100644 --- a/sys/kern/kern_thr.c +++ b/sys/kern/kern_thr.c @@ -282,7 +282,7 @@ thr_exit(struct thread *td, struct thr_exit_args *uap) } =20 PROC_LOCK(p); - sigqueue_flush(&td->td_sigqueue); + tdsigcleanup(td); PROC_SLOCK(p); =20 /* diff --git a/sys/kern/subr_trap.c b/sys/kern/subr_trap.c index 6d60ddb..ccf6479 100644 --- a/sys/kern/subr_trap.c +++ b/sys/kern/subr_trap.c @@ -90,6 +90,7 @@ userret(struct thread *td, struct trapframe *frame) =20 CTR3(KTR_SYSC, "userret: thread %p (pid %d, %s)", td, p->p_pid, td->td_name); +#if 0 #ifdef DIAGNOSTIC /* Check that we called signotify() enough. */ PROC_LOCK(p); @@ -100,6 +101,7 @@ userret(struct thread *td, struct trapframe *frame) thread_unlock(td); PROC_UNLOCK(p); #endif +#endif #ifdef KTRACE KTRUSERRET(td); #endif @@ -218,7 +220,14 @@ ast(struct trapframe *framep) ktrcsw(0, 1); #endif } - if (flags & TDF_NEEDSIGCHK) { + + /* + * Check for signals. Unlocked reads of p_pendingcnt or + * p_siglist might cause process-directed signal to be handled + * later. + */ + if (flags & TDF_NEEDSIGCHK || p->p_pendingcnt > 0 || + !SIGISEMPTY(p->p_siglist)) { PROC_LOCK(p); mtx_lock(&p->p_sigacts->ps_mtx); while ((sig =3D cursig(td, SIG_STOP_ALLOWED)) !=3D 0) diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h index 89b40f0..b39f2bf 100644 --- a/sys/sys/signalvar.h +++ b/sys/sys/signalvar.h @@ -252,9 +252,10 @@ typedef struct sigqueue { =20 /* Return nonzero if process p has an unmasked pending signal. */ #define SIGPENDING(td) \ - (!SIGISEMPTY((td)->td_siglist) && \ - !sigsetmasked(&(td)->td_siglist, &(td)->td_sigmask)) - + ((!SIGISEMPTY((td)->td_siglist) && \ + !sigsetmasked(&(td)->td_siglist, &(td)->td_sigmask)) || \ + (!SIGISEMPTY((td)->td_proc->p_siglist) && \ + !sigsetmasked(&(td)->td_proc->p_siglist, &(td)->td_sigmask))) /* * Return the value of the pseudo-expression ((*set & ~*mask) !=3D 0). Th= is * is an optimized version of SIGISEMPTY() on a temporary variable @@ -336,6 +337,7 @@ void sigexit(struct thread *td, int signum) __dead2; int sig_ffs(sigset_t *set); void siginit(struct proc *p); void signotify(struct thread *td); +void tdsigcleanup(struct thread *td); int tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi); void trapsignal(struct thread *td, ksiginfo_t *); diff --git a/tools/regression/sigqueue/sigqtest1/sigqtest1.c b/tools/regres= sion/sigqueue/sigqtest1/sigqtest1.c index 0f40021..f0201c3 100644 --- a/tools/regression/sigqueue/sigqtest1/sigqtest1.c +++ b/tools/regression/sigqueue/sigqtest1/sigqtest1.c @@ -1,12 +1,14 @@ /* $FreeBSD$ */ -#include -#include #include #include +#include +#include +#include =20 int received; =20 -void handler(int sig, siginfo_t *si, void *ctx) +void +handler(int sig, siginfo_t *si, void *ctx) { if (si->si_code !=3D SI_QUEUE) errx(1, "si_code !=3D SI_QUEUE"); @@ -15,7 +17,8 @@ void handler(int sig, siginfo_t *si, void *ctx) received++; } =20 -int main() +int +main() { struct sigaction sa; union sigval val; diff --git a/tools/regression/sigqueue/sigqtest2/sigqtest2.c b/tools/regres= sion/sigqueue/sigqtest2/sigqtest2.c index 078ea81..50b579d 100644 --- a/tools/regression/sigqueue/sigqtest2/sigqtest2.c +++ b/tools/regression/sigqueue/sigqtest2/sigqtest2.c @@ -1,24 +1,29 @@ /* $FreeBSD$ */ -#include -#include -#include -#include #include #include +#include +#include +#include +#include +#include +#include =20 int stop_received; int exit_received; int cont_received; =20 -void job_handler(int sig, siginfo_t *si, void *ctx) +void +job_handler(int sig, siginfo_t *si, void *ctx) { int status; int ret; =20 if (si->si_code =3D=3D CLD_STOPPED) { + printf("%d: stop received\n", si->si_pid); stop_received =3D 1; kill(si->si_pid, SIGCONT); } else if (si->si_code =3D=3D CLD_EXITED) { + printf("%d: exit received\n", si->si_pid); ret =3D waitpid(si->si_pid, &status, 0); if (ret =3D=3D -1) errx(1, "waitpid"); @@ -26,11 +31,13 @@ void job_handler(int sig, siginfo_t *si, void *ctx) errx(1, "!WIFEXITED(status)"); exit_received =3D 1; } else if (si->si_code =3D=3D CLD_CONTINUED) { + printf("%d: cont received\n", si->si_pid); cont_received =3D 1; } } =20 -void job_control_test() +void +job_control_test(void) { struct sigaction sa; pid_t pid; @@ -43,9 +50,12 @@ void job_control_test() stop_received =3D 0; cont_received =3D 0; exit_received =3D 0; + fflush(stdout); pid =3D fork(); if (pid =3D=3D 0) { + printf("child %d\n", getpid()); kill(getpid(), SIGSTOP); + sleep(2); exit(1); } =20 @@ -60,11 +70,13 @@ void job_control_test() printf("job control test OK.\n"); } =20 -void rtsig_handler(int sig, siginfo_t *si, void *ctx) +void +rtsig_handler(int sig, siginfo_t *si, void *ctx) { } =20 -int main() +int +main() { struct sigaction sa; sigset_t set; --jvmCgFKx2lledmPK Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAkrQm/sACgkQC3+MBN1Mb4hTygCfVN6ia2OYpxHSXQS1jZWht6wc 8iIAoIq4qtkfEEQzINCI5h9zqifPXsI+ =pV7V -----END PGP SIGNATURE----- --jvmCgFKx2lledmPK--