On Thu, May 09, 2019 at 11:01:13PM -0400, Alexander Bluhm wrote:
> When killing a process, the signal is handled by any thread that
> does not block the signal. If all threads block the signal, we
> currently deliver it to the main thread. This does not conform to
> POSIX. If any thread unblocks the signal, it should be delivered
> immediately to this thread. We block it until the main thread
> unblocks.
>
> I see the problem as some tests of posixtestsuite hang. Various
> people reported similar observations with multi threaded programs
> to me.
>
> Solution is to mark such signals at the process instead of one
> thread. Then any thread can handle it later. There is a XXX comment
> that suggests this behavior, NetBSD also does this.
>
> This diff survived a full regress run on i386.
I have written a regress test that has showed a bug I introdced.
In exit1() I have to clear pr->ps_siglist only when exiting the
main thread.
new diff, ok?
bluhm
Index: kern/exec_elf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.149
diff -u -p -r1.149 exec_elf.c
--- kern/exec_elf.c 9 May 2019 22:25:42 -0000 1.149
+++ kern/exec_elf.c 10 May 2019 02:39:07 -0000
@@ -1224,7 +1224,7 @@ coredump_notes_elf(struct proc *p, void
cpi.cpi_signo = p->p_sisig;
cpi.cpi_sigcode = p->p_sicode;
- cpi.cpi_sigpend = p->p_siglist;
+ cpi.cpi_sigpend = p->p_siglist | pr->ps_siglist;
cpi.cpi_sigmask = p->p_sigmask;
cpi.cpi_sigignore = pr->ps_sigacts->ps_sigignore;
cpi.cpi_sigcatch = pr->ps_sigacts->ps_sigcatch;
Index: kern/kern_exit.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_exit.c,v
retrieving revision 1.173
diff -u -p -r1.173 kern_exit.c
--- kern/kern_exit.c 23 Jan 2019 22:39:47 -0000 1.173
+++ kern/kern_exit.c 11 May 2019 04:48:49 -0000
@@ -180,6 +180,8 @@ exit1(struct proc *p, int rv, int flags)
}
}
p->p_siglist = 0;
+ if ((p->p_flag & P_THREAD) == 0)
+ pr->ps_siglist = 0;
#if NKCOV > 0
kcov_exit(p);
Index: kern/kern_sig.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.229
diff -u -p -r1.229 kern_sig.c
--- kern/kern_sig.c 1 May 2019 06:26:42 -0000 1.229
+++ kern/kern_sig.c 11 May 2019 04:17:30 -0000
@@ -365,6 +365,7 @@ setsigvec(struct proc *p, int signum, st
if (sa->sa_handler == SIG_IGN ||
(sigprop[signum] & SA_IGNORE && sa->sa_handler == SIG_DFL)) {
atomic_clearbits_int(&p->p_siglist, bit);
+ atomic_clearbits_int(&p->p_p->ps_siglist, bit);
if (signum != SIGCONT)
ps->ps_sigignore |= bit; /* easier in psignal */
ps->ps_sigcatch &= ~bit;
@@ -419,6 +420,7 @@ execsigs(struct proc *p)
if (nc != SIGCONT)
ps->ps_sigignore |= mask;
atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(&p->p_p->ps_siglist, mask);
}
ps->ps_sigact[nc] = SIG_DFL;
}
@@ -472,7 +474,7 @@ int
sys_sigpending(struct proc *p, void *v, register_t *retval)
{
- *retval = p->p_siglist;
+ *retval = p->p_siglist | p->p_p->ps_siglist;
return (0);
}
@@ -875,7 +877,6 @@ psignal(struct proc *p, int signum)
/*
* type = SPROCESS process signal, can be diverted (sigwait())
- * XXX if blocked in all threads, mark as pending in struct process
* type = STHREAD thread signal, but should be propagated if unhandled
* type = SPROPAGATED propagated to this thread, so don't propagate again
*/
@@ -885,6 +886,7 @@ ptsignal(struct proc *p, int signum, enu
int s, prop;
sig_t action;
int mask;
+ int *siglist;
struct process *pr = p->p_p;
struct proc *q;
int wakeparent = 0;
@@ -907,7 +909,7 @@ ptsignal(struct proc *p, int signum, enu
if (pr->ps_flags & PS_COREDUMP && signum == SIGKILL) {
if (pr->ps_single != NULL)
p = pr->ps_single;
- atomic_setbits_int(&p->p_siglist, mask);
+ atomic_setbits_int(&p->p_p->ps_siglist, mask);
return;
}
@@ -962,7 +964,6 @@ ptsignal(struct proc *p, int signum, enu
*/
if (pr->ps_flags & PS_TRACED) {
action = SIG_DFL;
- atomic_setbits_int(&p->p_siglist, mask);
} else {
/*
* If the signal is being ignored,
@@ -992,17 +993,23 @@ ptsignal(struct proc *p, int signum, enu
if (prop & SA_TTYSTOP && pr->ps_pgrp->pg_jobc == 0)
return;
}
-
- atomic_setbits_int(&p->p_siglist, mask);
}
-
- if (prop & SA_CONT)
- atomic_clearbits_int(&p->p_siglist, stopsigmask);
-
+ /*
+ * If delivered to process, mark as pending there. Continue and stop
+ * signals will be propagated to all threads. So they are always
+ * marked at thread level.
+ */
+ siglist = (type == SPROCESS) ? &pr->ps_siglist : &p->p_siglist;
+ if (prop & SA_CONT) {
+ siglist = &p->p_siglist;
+ atomic_clearbits_int(siglist, stopsigmask);
+ }
if (prop & SA_STOP) {
- atomic_clearbits_int(&p->p_siglist, contsigmask);
+ siglist = &p->p_siglist;
+ atomic_clearbits_int(siglist, contsigmask);
atomic_clearbits_int(&p->p_flag, P_CONTINUED);
}
+ atomic_setbits_int(siglist, mask);
/*
* XXX delay processing of SA_STOP signals unless action == SIG_DFL?
@@ -1045,7 +1052,7 @@ ptsignal(struct proc *p, int signum, enu
* be awakened.
*/
if ((prop & SA_CONT) && action == SIG_DFL) {
- atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(siglist, mask);
goto out;
}
/*
@@ -1059,7 +1066,7 @@ ptsignal(struct proc *p, int signum, enu
*/
if (pr->ps_flags & PS_PPWAIT)
goto out;
- atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(siglist, mask);
p->p_xstat = signum;
proc_stop(p, 0);
goto out;
@@ -1102,7 +1109,7 @@ ptsignal(struct proc *p, int signum, enu
atomic_clearbits_int(&p->p_flag, P_SUSPSIG);
wakeparent = 1;
if (action == SIG_DFL)
- atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(siglist, mask);
if (action == SIG_CATCH)
goto runfast;
if (p->p_wchan == 0)
@@ -1116,7 +1123,7 @@ ptsignal(struct proc *p, int signum, enu
* Already stopped, don't need to stop again.
* (If we did the shell could get confused.)
*/
- atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(siglist, mask);
goto out;
}
@@ -1181,7 +1188,7 @@ issignal(struct proc *p)
int s;
for (;;) {
- mask = p->p_siglist & ~p->p_sigmask;
+ mask = SIGPENDING(p);
if (pr->ps_flags & PS_PPWAIT)
mask &= ~stopsigmask;
if (mask == 0) /* no signal to send */
@@ -1189,6 +1196,7 @@ issignal(struct proc *p)
signum = ffs((long)mask);
mask = sigmask(signum);
atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(&p->p_p->ps_siglist, mask);
/*
* We should see pending but ignored signals
@@ -1243,6 +1251,7 @@ issignal(struct proc *p)
/* take the signal! */
atomic_clearbits_int(&p->p_siglist, mask);
+ atomic_clearbits_int(&p->p_p->ps_siglist, mask);
}
prop = sigprop[signum];
@@ -1638,7 +1647,8 @@ coredump_write(void *cookie, enum uio_se
csize = len;
do {
- if (io->io_proc->p_siglist & sigmask(SIGKILL))
+ if (sigmask(SIGKILL) &
+ (io->io_proc->p_siglist | io->io_proc->p_p->ps_siglist))
return (EINTR);
/* Rest of the loop sleeps with lock held, so... */
@@ -1740,8 +1750,8 @@ sys___thrsigdivert(struct proc *p, void
if (smask & mask) {
if (p->p_siglist & smask)
m = &p->p_siglist;
- else if (pr->ps_mainproc->p_siglist & smask)
- m = &pr->ps_mainproc->p_siglist;
+ else if (pr->ps_siglist & smask)
+ m = &pr->ps_siglist;
else {
/* signal got eaten by someone else? */
continue;
@@ -1868,7 +1878,7 @@ userret(struct proc *p)
KERNEL_UNLOCK();
}
- if (SIGPENDING(p)) {
+ if (SIGPENDING(p) != 0) {
KERNEL_LOCK();
while ((signum = CURSIG(p)) != 0)
postsig(p, signum);
Index: kern/tty.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/tty.c,v
retrieving revision 1.143
diff -u -p -r1.143 tty.c
--- kern/tty.c 6 Sep 2018 11:50:54 -0000 1.143
+++ kern/tty.c 10 May 2019 02:39:07 -0000
@@ -1676,11 +1676,11 @@ ttycheckoutq(struct tty *tp, int wait)
hiwat = tp->t_hiwat;
s = spltty();
- oldsig = wait ? curproc->p_siglist : 0;
+ oldsig = wait ? SIGPENDING(curproc) : 0;
if (tp->t_outq.c_cc > hiwat + TTHIWATMINSPACE)
while (tp->t_outq.c_cc > hiwat) {
ttstart(tp);
- if (wait == 0 || curproc->p_siglist != oldsig) {
+ if (wait == 0 || SIGPENDING(curproc) != oldsig) {
splx(s);
return (0);
}
Index: nfs/nfs_socket.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.132
diff -u -p -r1.132 nfs_socket.c
--- nfs/nfs_socket.c 9 Nov 2018 14:14:32 -0000 1.132
+++ nfs/nfs_socket.c 10 May 2019 02:39:07 -0000
@@ -1225,9 +1225,8 @@ nfs_sigintr(struct nfsmount *nmp, struct
return (EINTR);
if (!(nmp->nm_flag & NFSMNT_INT))
return (0);
- if (p && p->p_siglist &&
- (((p->p_siglist & ~p->p_sigmask) &
- ~p->p_p->ps_sigacts->ps_sigignore) & NFSINT_SIGMASK))
+ if (p && (SIGPENDING(p) & ~p->p_p->ps_sigacts->ps_sigignore &
+ NFSINT_SIGMASK))
return (EINTR);
return (0);
}
Index: sys/proc.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/proc.h,v
retrieving revision 1.263
diff -u -p -r1.263 proc.h
--- sys/proc.h 6 Jan 2019 12:59:45 -0000 1.263
+++ sys/proc.h 10 May 2019 02:39:07 -0000
@@ -186,6 +186,7 @@ struct process {
#define ps_startzero ps_klist
struct klist ps_klist; /* knotes attached to this process */
int ps_flags; /* PS_* flags. */
+ int ps_siglist; /* Signals pending for the process. */
struct proc *ps_single; /* Single threading to this thread. */
int ps_singlecount; /* Not yet suspended threads. */
Index: sys/signalvar.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/signalvar.h,v
retrieving revision 1.35
diff -u -p -r1.35 signalvar.h
--- sys/signalvar.h 17 Dec 2018 14:51:57 -0000 1.35
+++ sys/signalvar.h 10 May 2019 02:39:07 -0000
@@ -67,24 +67,30 @@ struct sigacts {
/*
* Check if process p has an unmasked signal pending.
+ * Return mask of pending signals.
*/
-#define SIGPENDING(p) (((p)->p_siglist & ~(p)->p_sigmask) != 0)
+#define SIGPENDING(p) \
+ (((p)->p_siglist | (p)->p_p->ps_siglist) & ~(p)->p_sigmask)
/*
* Determine signal that should be delivered to process p, the current
* process, 0 if none. If there is a pending stop signal with default
* action, the process stops in issignal().
*/
-#define CURSIG(p)
\
- (((p)->p_siglist == 0 || \
+#define CURSIG(p) \
+ ((((p)->p_siglist | (p)->p_p->ps_siglist) == 0 || \
(((p)->p_p->ps_flags & PS_TRACED) == 0 && \
- ((p)->p_siglist & ~(p)->p_sigmask) == 0)) ? \
+ SIGPENDING(p) == 0)) ? \
0 : issignal(p))
/*
* Clear a pending signal from a process.
*/
-#define CLRSIG(p, sig) atomic_clearbits_int(&(p)->p_siglist,
sigmask(sig))
+#define CLRSIG(p, sig) do { \
+ int _mask = sigmask(sig); \
+ atomic_clearbits_int(&(p)->p_siglist, _mask); \
+ atomic_clearbits_int(&(p)->p_p->ps_siglist, _mask); \
+} while (0)
/*
* Signal properties and actions.
Index: sys/sysctl.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.185
diff -u -p -r1.185 sysctl.h
--- sys/sysctl.h 9 May 2019 14:59:30 -0000 1.185
+++ sys/sysctl.h 10 May 2019 02:39:07 -0000
@@ -595,7 +595,7 @@ do {
\
(kp)->p_tracep = PTRTOINT64((pr)->ps_tracevp); \
(kp)->p_traceflag = (pr)->ps_traceflag; \
\
- (kp)->p_siglist = (p)->p_siglist; \
+ (kp)->p_siglist = (p)->p_siglist | (pr)->ps_siglist; \
(kp)->p_sigmask = (p)->p_sigmask; \
(kp)->p_sigignore = (sa) ? (sa)->ps_sigignore : 0; \
(kp)->p_sigcatch = (sa) ? (sa)->ps_sigcatch : 0; \
