On Sat, Oct 10, 2009 at 01:25:16PM +0800, David Xu wrote: > Kostik Belousov wrote: > > >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. > ><snip> > > 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. > 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 _at__at_ -220,6 +220,8 _at__at_ static int sigproptbl[NSIG] = { SA_KILL|SA_PROC, /* SIGUSR2 */ }; +static void reschedule_signals(struct proc *p, sigset_t block); + static void sigqueue_start(void) { _at__at_ -581,20 +583,11 _at__at_ void signotify(struct thread *td) { struct proc *p; - sigset_t set; p = td->td_proc; PROC_LOCK_ASSERT(p, MA_OWNED); - /* - * If our mask changed we may have to move signal that were - * previously masked by all threads to our sigqueue. - */ - set = 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 |= TDF_NEEDSIGCHK | TDF_ASTPENDING; _at__at_ -976,24 +969,28 _at__at_ 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; - PROC_LOCK(td->td_proc); + p = td->td_proc; + PROC_LOCK(p); if (oset != NULL) *oset = td->td_sigmask; error = 0; + SIGEMPTYSET(new_block); if (set != NULL) { switch (how) { case SIG_BLOCK: SIG_CANTMASK(*set); + oset1 = td->td_sigmask; SIGSETOR(td->td_sigmask, *set); + new_block = td->td_sigmask; + SIGSETNAND(new_block, oset1); break; case SIG_UNBLOCK: SIGSETNAND(td->td_sigmask, *set); _at__at_ -1001,10 +998,13 _at__at_ kern_sigprocmask(td, how, set, oset, old) break; case SIG_SETMASK: SIG_CANTMASK(*set); + oset1 = td->td_sigmask; if (old) SIGSETLO(td->td_sigmask, *set); else td->td_sigmask = *set; + new_block = td->td_sigmask; + SIGSETNAND(new_block, oset1); signotify(td); break; default: _at__at_ -1012,7 +1012,20 _at__at_ 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 != 1) + reschedule_signals(p, new_block); + + PROC_UNLOCK(p); return (error); } _at__at_ -1985,17 +1998,9 _at__at_ tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi) KNOTE_LOCKED(&p->p_klist, NOTE_SIGNAL | sig); prop = sigprop(sig); - /* - * 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 == NULL) { td = sigtd(p, sig, prop); - if (SIGISMEMBER(td->td_sigmask, sig)) - sigqueue = &p->p_sigqueue; - else - sigqueue = &td->td_sigqueue; + sigqueue = &p->p_sigqueue; } else { KASSERT(td->td_proc == p, ("invalid thread")); sigqueue = &td->td_sigqueue; _at__at_ -2392,6 +2397,62 _at__at_ stopme: return (td->td_xsig); } +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 = p->p_sigacts; + for (i = 0; !SIGISEMPTY(block); i++) { + if (!SIGISMEMBER(block, i)) + continue; + SIGDELSET(block, i); + if (!SIGISMEMBER(p->p_siglist, i)) + continue; + + td = 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 = td->td_proc; + PROC_LOCK_ASSERT(p, MA_OWNED); + + sigqueue_flush(&td->td_sigqueue); + if (p->p_numthreads == 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 number. _at__at_ -2409,8 +2470,9 _at__at_ 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; p = td->td_proc; ps = p->p_sigacts; _at__at_ -2420,6 +2482,7 _at__at_ issignal(struct thread *td, int stop_allowed) int traced = (p->p_flag & P_TRACED) || (p->p_stops & S_SIG); sigpending = td->td_sigqueue.sq_signals; + SIGSETOR(sigpending, p->p_sigqueue.sq_signals); SIGSETNAND(sigpending, td->td_sigmask); if (p->p_flag & P_PPWAIT) _at__at_ -2440,6 +2503,7 _at__at_ issignal(struct thread *td, int stop_allowed) */ if (SIGISMEMBER(ps->ps_sigignore, sig) && (traced == 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) == 0) { _at__at_ -2452,12 +2516,18 _at__at_ issignal(struct thread *td, int stop_allowed) if (sig != newsig) { ksiginfo_t ksi; + + queue = &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) == 0) { + queue = &p->p_sigqueue; + signo = sigqueue_get(queue, sig, &ksi); + KASSERT(signo == sig, ("signo != sig")); + } /* * If parent wants us to take the signal, _at__at_ -2472,7 +2542,7 _at__at_ 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); _at__at_ -2567,6 +2637,7 _at__at_ 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 */ } _at__at_ -2613,7 +2684,9 _at__at_ postsig(sig) ps = 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) == 0 && + sigqueue_get(&p->p_sigqueue, sig, &ksi) == 0) + return; ksi.ksi_signo = sig; if (ksi.ksi_code == 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 _at__at_ -282,7 +282,7 _at__at_ thr_exit(struct thread *td, struct thr_exit_args *uap) } PROC_LOCK(p); - sigqueue_flush(&td->td_sigqueue); + tdsigcleanup(td); PROC_SLOCK(p); /* 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 _at__at_ -90,6 +90,7 _at__at_ userret(struct thread *td, struct trapframe *frame) 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); _at__at_ -100,6 +101,7 _at__at_ userret(struct thread *td, struct trapframe *frame) thread_unlock(td); PROC_UNLOCK(p); #endif +#endif #ifdef KTRACE KTRUSERRET(td); #endif _at__at_ -218,7 +220,14 _at__at_ 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 = cursig(td, SIG_STOP_ALLOWED)) != 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 _at__at_ -252,9 +252,10 _at__at_ typedef struct sigqueue { /* 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) != 0). This * is an optimized version of SIGISEMPTY() on a temporary variable _at__at_ -336,6 +337,7 _at__at_ 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/regression/sigqueue/sigqtest1/sigqtest1.c index 0f40021..f0201c3 100644 --- a/tools/regression/sigqueue/sigqtest1/sigqtest1.c +++ b/tools/regression/sigqueue/sigqtest1/sigqtest1.c _at__at_ -1,12 +1,14 _at__at_ /* $FreeBSD$ */ -#include <signal.h> -#include <stdio.h> #include <err.h> #include <errno.h> +#include <signal.h> +#include <stdio.h> +#include <unistd.h> int received; -void handler(int sig, siginfo_t *si, void *ctx) +void +handler(int sig, siginfo_t *si, void *ctx) { if (si->si_code != SI_QUEUE) errx(1, "si_code != SI_QUEUE"); _at__at_ -15,7 +17,8 _at__at_ void handler(int sig, siginfo_t *si, void *ctx) received++; } -int main() +int +main() { struct sigaction sa; union sigval val; diff --git a/tools/regression/sigqueue/sigqtest2/sigqtest2.c b/tools/regression/sigqueue/sigqtest2/sigqtest2.c index 078ea81..50b579d 100644 --- a/tools/regression/sigqueue/sigqtest2/sigqtest2.c +++ b/tools/regression/sigqueue/sigqtest2/sigqtest2.c _at__at_ -1,24 +1,29 _at__at_ /* $FreeBSD$ */ -#include <signal.h> -#include <stdio.h> -#include <err.h> -#include <errno.h> #include <sys/types.h> #include <sys/wait.h> +#include <err.h> +#include <errno.h> +#include <signal.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> int stop_received; int exit_received; int cont_received; -void job_handler(int sig, siginfo_t *si, void *ctx) +void +job_handler(int sig, siginfo_t *si, void *ctx) { int status; int ret; if (si->si_code == CLD_STOPPED) { + printf("%d: stop received\n", si->si_pid); stop_received = 1; kill(si->si_pid, SIGCONT); } else if (si->si_code == CLD_EXITED) { + printf("%d: exit received\n", si->si_pid); ret = waitpid(si->si_pid, &status, 0); if (ret == -1) errx(1, "waitpid"); _at__at_ -26,11 +31,13 _at__at_ void job_handler(int sig, siginfo_t *si, void *ctx) errx(1, "!WIFEXITED(status)"); exit_received = 1; } else if (si->si_code == CLD_CONTINUED) { + printf("%d: cont received\n", si->si_pid); cont_received = 1; } } -void job_control_test() +void +job_control_test(void) { struct sigaction sa; pid_t pid; _at__at_ -43,9 +50,12 _at__at_ void job_control_test() stop_received = 0; cont_received = 0; exit_received = 0; + fflush(stdout); pid = fork(); if (pid == 0) { + printf("child %d\n", getpid()); kill(getpid(), SIGSTOP); + sleep(2); exit(1); } _at__at_ -60,11 +70,13 _at__at_ void job_control_test() printf("job control test OK.\n"); } -void rtsig_handler(int sig, siginfo_t *si, void *ctx) +void +rtsig_handler(int sig, siginfo_t *si, void *ctx) { } -int main() +int +main() { struct sigaction sa; sigset_t set;
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:56 UTC