From: claudio <claudio@openbsd.org>
Date: Wed, 10 Apr 2024 10:05:26 +0000 (+0000)
Subject: Unlock dosigsuspend() and with that some aspects of ppoll and pselect
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=a2f40c7d565e0cf83267ba9164c73040b47a3cc5;p=openbsd

Unlock dosigsuspend() and with that some aspects of ppoll and pselect

Change p_sigmask from atomic back to non-atomic updates. All changes to
p_sigmask are only allowed by curproc (the owner). There is no need for
atomic instructions here.

p_sigmask is mostly accessed by curproc with the exception of ptsignal().
In ptsignal() p_sigmask is now only read once unless a SSLEEP proc gets
the signal. In that case recheck the p_sigmask before wakeup to ensure
that no unnecessary wakeup happens.

Add some KASSERT(p == curproc) to ensure this precondition.
sigabort() is special since it is also called by ddb but apart from that
only works for curproc.

With and OK mvs@ OK mpi@
---

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 5205c05e890..60d7941ece2 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_sig.c,v 1.323 2024/03/30 13:33:20 mpi Exp $	*/
+/*	$OpenBSD: kern_sig.c,v 1.324 2024/04/10 10:05:26 claudio Exp $	*/
 /*	$NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $	*/
 
 /*
@@ -472,10 +472,10 @@ sys_sigprocmask(struct proc *p, void *v, register_t *retval)
 
 	switch (SCARG(uap, how)) {
 	case SIG_BLOCK:
-		atomic_setbits_int(&p->p_sigmask, mask);
+		SET(p->p_sigmask, mask);
 		break;
 	case SIG_UNBLOCK:
-		atomic_clearbits_int(&p->p_sigmask, mask);
+		CLR(p->p_sigmask, mask);
 		break;
 	case SIG_SETMASK:
 		p->p_sigmask = mask;
@@ -504,8 +504,8 @@ dosigsuspend(struct proc *p, sigset_t newmask)
 	KASSERT(p == curproc);
 
 	p->p_oldmask = p->p_sigmask;
-	atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
 	p->p_sigmask = newmask;
+	atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
 }
 
 /*
@@ -770,7 +770,7 @@ void
 postsig_done(struct proc *p, int signum, sigset_t catchmask, int reset)
 {
 	p->p_ru.ru_nsignals++;
-	atomic_setbits_int(&p->p_sigmask, catchmask);
+	SET(p->p_sigmask, catchmask);
 	if (reset != 0) {
 		sigset_t mask = sigmask(signum);
 		struct sigacts *ps = p->p_p->ps_sigacts;
@@ -922,7 +922,7 @@ ptsignal(struct proc *p, int signum, enum signal_type type)
 {
 	int s, prop;
 	sig_t action;
-	int mask;
+	sigset_t mask, sigmask;
 	int *siglist;
 	struct process *pr = p->p_p;
 	struct proc *q;
@@ -940,8 +940,11 @@ ptsignal(struct proc *p, int signum, enum signal_type type)
 		return;
 
 	mask = sigmask(signum);
+	sigmask = READ_ONCE(p->p_sigmask);
 
 	if (type == SPROCESS) {
+		sigset_t tmpmask;
+
 		/* Accept SIGKILL to coredumping processes */
 		if (pr->ps_flags & PS_COREDUMP && signum == SIGKILL) {
 			atomic_setbits_int(&pr->ps_siglist, mask);
@@ -953,10 +956,12 @@ ptsignal(struct proc *p, int signum, enum signal_type type)
 		 * immediately (it's unblocked) then have it take it.
 		 */
 		q = curproc;
-		if (q != NULL && q->p_p == pr && (q->p_flag & P_WEXIT) == 0 &&
-		    (q->p_sigmask & mask) == 0)
+		tmpmask = READ_ONCE(q->p_sigmask);
+		if (q->p_p == pr && (q->p_flag & P_WEXIT) == 0 &&
+		    (tmpmask & mask) == 0) {
 			p = q;
-		else {
+			sigmask = tmpmask;
+		} else {
 			/*
 			 * A process-wide signal can be diverted to a
 			 * different thread that's in sigwait() for this
@@ -967,16 +972,19 @@ ptsignal(struct proc *p, int signum, enum signal_type type)
 			 * main thread.
 			 */
 			TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
+
 				/* ignore exiting threads */
 				if (q->p_flag & P_WEXIT)
 					continue;
 
 				/* skip threads that have the signal blocked */
-				if ((q->p_sigmask & mask) != 0)
+				tmpmask = READ_ONCE(q->p_sigmask);
+				if ((tmpmask & mask) != 0)
 					continue;
 
 				/* okay, could send to this thread */
 				p = q;
+				sigmask = tmpmask;
 
 				/*
 				 * sigsuspend, sigwait, ppoll/pselect, etc?
@@ -1016,7 +1024,7 @@ ptsignal(struct proc *p, int signum, enum signal_type type)
 
 		if (sigignore & mask)
 			return;
-		if (p->p_sigmask & mask) {
+		if (sigmask & mask) {
 			action = SIG_HOLD;
 		} else if (sigcatch & mask) {
 			action = SIG_CATCH;
@@ -1090,6 +1098,16 @@ ptsignal(struct proc *p, int signum, enum signal_type type)
 		 */
 		if (pr->ps_flags & PS_TRACED)
 			goto run;
+
+		/*
+		 * Recheck sigmask before waking up the process,
+		 * there is a chance that while sending the signal
+		 * the process changed sigmask and went to sleep.
+		 */
+		sigmask = READ_ONCE(p->p_sigmask);
+		if (sigmask & mask)
+			goto out;
+
 		/*
 		 * If SIGCONT is default (or ignored) and process is
 		 * asleep, we are finished; the process should not
@@ -1582,10 +1600,12 @@ sigabort(struct proc *p)
 {
 	struct sigaction sa;
 
+	KASSERT(p == curproc || panicstr || db_active);
+
 	memset(&sa, 0, sizeof sa);
 	sa.sa_handler = SIG_DFL;
 	setsigvec(p, SIGABRT, &sa);
-	atomic_clearbits_int(&p->p_sigmask, sigmask(SIGABRT));
+	CLR(p->p_sigmask, sigmask(SIGABRT));
 	psignal(p, SIGABRT);
 }
 
@@ -1599,6 +1619,8 @@ sigismasked(struct proc *p, int sig)
 	struct process *pr = p->p_p;
 	int rv;
 
+	KASSERT(p == curproc);
+
 	mtx_enter(&pr->ps_mtx);
 	rv = (pr->ps_sigacts->ps_sigignore & sigmask(sig)) ||
 	    (p->p_sigmask & sigmask(sig));
@@ -2000,8 +2022,8 @@ userret(struct proc *p)
 	 * time for signals to post.
 	 */
 	if (p->p_flag & P_SIGSUSPEND) {
-		atomic_clearbits_int(&p->p_flag, P_SIGSUSPEND);
 		p->p_sigmask = p->p_oldmask;
+		atomic_clearbits_int(&p->p_flag, P_SIGSUSPEND);
 
 		while ((signum = cursig(p, &ctx)) != 0)
 			postsig(p, signum, &ctx);
diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index e012d086ba6..8699daff522 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: sys_generic.c,v 1.156 2023/05/09 14:22:17 visa Exp $	*/
+/*	$OpenBSD: sys_generic.c,v 1.157 2024/04/10 10:05:26 claudio Exp $	*/
 /*	$NetBSD: sys_generic.c,v 1.24 1996/03/29 00:25:32 cgd Exp $	*/
 
 /*
@@ -644,11 +644,8 @@ dopselect(struct proc *p, int nd, fd_set *in, fd_set *ou, fd_set *ex,
 	}
 #endif
 
-	if (sigmask) {
-		KERNEL_LOCK();
+	if (sigmask)
 		dosigsuspend(p, *sigmask &~ sigcantmask);
-		KERNEL_UNLOCK();
-	}
 
 	/* Register kqueue events */
 	error = pselregister(p, pibits, pobits, nd, &nevents, &ncollected);
@@ -946,11 +943,8 @@ doppoll(struct proc *p, struct pollfd *fds, u_int nfds,
 	if ((error = copyin(fds, pl, sz)) != 0)
 		goto bad;
 
-	if (sigmask) {
-		KERNEL_LOCK();
+	if (sigmask)
 		dosigsuspend(p, *sigmask &~ sigcantmask);
-		KERNEL_UNLOCK();
-	}
 
 	/* Register kqueue events */
 	ppollregister(p, pl, nfds, &nevents, &ncollected);
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 4333914bf6e..b8217f6b948 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.h,v 1.358 2024/04/02 08:39:16 deraadt Exp $	*/
+/*	$OpenBSD: proc.h,v 1.359 2024/04/10 10:05:26 claudio Exp $	*/
 /*	$NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $	*/
 
 /*-
@@ -321,7 +321,7 @@ struct p_inentry {
  *	S	scheduler lock
  *	U	uidinfolk
  *	l	read only reference, see lim_read_enter()
- *	o	owned (read/modified only) by this thread
+ *	o	owned (modified only) by this thread
  */
 struct proc {
 	TAILQ_ENTRY(proc) p_runq;	/* [S] current run/sleep queue */
@@ -379,7 +379,7 @@ struct proc {
 
 /* The following fields are all copied upon creation in fork. */
 #define	p_startcopy	p_sigmask
-	sigset_t p_sigmask;		/* [a] Current signal mask */
+	sigset_t p_sigmask;		/* [o] Current signal mask */
 
 	char	p_name[_MAXCOMLEN];	/* thread name, incl NUL */
 	u_char	p_slppri;		/* [S] Sleeping priority */
@@ -398,7 +398,7 @@ struct proc {
 	struct	user *p_addr;	/* Kernel virtual addr of u-area */
 	struct	mdproc p_md;	/* Any machine-dependent fields. */
 
-	sigset_t p_oldmask;	/* Saved mask from before sigpause */
+	sigset_t p_oldmask;	/* [o] Saved mask from before sigpause */
 	int	p_sisig;	/* For core dump/debugger XXX */
 	union sigval p_sigval;	/* For core dump/debugger XXX */
 	long	p_sitrapno;	/* For core dump/debugger XXX */