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 5205c05..60d7941 100644 (file)
--- 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 e012d08..8699daf 100644 (file)
--- 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 4333914..b8217f6 100644 (file)
--- 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 */