Revert commitid: yfAefyNWibUyjkU2, ESyyH5EKxtrXGkS6 and itscfpFvJLOj8mHB;
authorclaudio <claudio@openbsd.org>
Wed, 13 Sep 2023 14:25:49 +0000 (14:25 +0000)
committerclaudio <claudio@openbsd.org>
Wed, 13 Sep 2023 14:25:49 +0000 (14:25 +0000)
The change to the single thread API results in crashes inside exit1()
as found by Syzkaller. There seems to be a race in the exit codepath.
What exactly fails is not really clear therefor revert for now.

This should fix the following Syzkaller reports:
Reported-by: syzbot+38efb425eada701ca8bb@syzkaller.appspotmail.com
Reported-by: syzbot+ecc0e8628b3db39b5b17@syzkaller.appspotmail.com
and maybe more.

Reverted commits:
----------------------------
Protect ps_single, ps_singlecnt and ps_threadcnt by the process mutex.

The single thread API needs to lock the process to enter single thread
mode and does not need to stop the scheduler.

This code changes ps_singlecount from a count down to zero to ps_singlecnt
which counts up until equal to ps_threadcnt (in which case all threads
are properly asleep).

Tested by phessler@, OK mpi@ cheloha@
----------------------------
Change how ps_threads and p_thr_link are locked away from using SCHED_LOCK.

The per process thread list can be traversed (read) by holding either
the KERNEL_LOCK or the per process ps_mtx (instead of SCHED_LOCK).
Abusing the SCHED_LOCK for this makes it impossible to split up the
scheduler lock into something more fine grained.

Tested by phessler@, ok mpi@
----------------------------
Fix SCHED_LOCK() leak in single_thread_set()

In the (q->p_flag & P_WEXIT) branch is a continue that did not release
the SCHED_LOCK. Refactor the code a bit to simplify the places SCHED_LOCK
is grabbed and released.

Reported-by: syzbot+ea26d351acfad3bb3f15@syzkaller.appspotmail.com
OK kettenis@

sys/kern/kern_exit.c
sys/kern/kern_fork.c
sys/kern/kern_resource.c
sys/kern/kern_sig.c
sys/kern/kern_synch.c
sys/sys/proc.h

index 460833a..4d7bfd5 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_exit.c,v 1.214 2023/09/08 09:06:31 claudio Exp $ */
+/*     $OpenBSD: kern_exit.c,v 1.215 2023/09/13 14:25:49 claudio Exp $ */
 /*     $NetBSD: kern_exit.c,v 1.39 1996/04/22 01:38:25 christos Exp $  */
 
 /*
@@ -119,7 +119,7 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
        struct process *pr, *qr, *nqr;
        struct rusage *rup;
        struct timespec ts;
-       int s, wake;
+       int s;
 
        atomic_setbits_int(&p->p_flag, P_WEXIT);
 
@@ -157,22 +157,14 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
        }
 
        /* unlink ourselves from the active threads */
-       mtx_enter(&pr->ps_mtx);
+       SCHED_LOCK(s);
        TAILQ_REMOVE(&pr->ps_threads, p, p_thr_link);
-       pr->ps_threadcnt--;
-
-       wake = (pr->ps_single && pr->ps_singlecnt == pr->ps_threadcnt);
-       mtx_leave(&pr->ps_mtx);
-       if (wake)
-               wakeup(&pr->ps_singlecnt);
+       SCHED_UNLOCK(s);
 
        if ((p->p_flag & P_THREAD) == 0) {
                /* main thread gotta wait because it has the pid, et al */
-               mtx_enter(&pr->ps_mtx);
-               while (pr->ps_threadcnt > 0)
-                       msleep_nsec(&pr->ps_threads, &pr->ps_mtx, PWAIT,
-                           "thrdeath", INFSLP);
-               mtx_leave(&pr->ps_mtx);
+               while (pr->ps_threadcnt > 1)
+                       tsleep_nsec(&pr->ps_threads, PWAIT, "thrdeath", INFSLP);
                if (pr->ps_flags & PS_PROFIL)
                        stopprofclock(pr);
        }
@@ -345,10 +337,9 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
        /* just a thread? detach it from its process */
        if (p->p_flag & P_THREAD) {
                /* scheduler_wait_hook(pr->ps_mainproc, p); XXX */
-               mtx_enter(&pr->ps_mtx);
-               if (pr->ps_threadcnt == 0)
+               if (--pr->ps_threadcnt == 1)
                        wakeup(&pr->ps_threads);
-               mtx_leave(&pr->ps_mtx);
+               KASSERT(pr->ps_threadcnt > 0);
        }
 
        /* Release the thread's read reference of resource limit structure. */
@@ -832,7 +823,7 @@ process_zap(struct process *pr)
        if (otvp)
                vrele(otvp);
 
-       KASSERT(pr->ps_threadcnt == 0);
+       KASSERT(pr->ps_threadcnt == 1);
        if (pr->ps_ptstat != NULL)
                free(pr->ps_ptstat, M_SUBPROC, sizeof(*pr->ps_ptstat));
        pool_put(&rusage_pool, pr->ps_ru);
index f3eca89..5c60194 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_fork.c,v 1.251 2023/09/08 09:06:31 claudio Exp $ */
+/*     $OpenBSD: kern_fork.c,v 1.252 2023/09/13 14:25:49 claudio Exp $ */
 /*     $NetBSD: kern_fork.c,v 1.29 1996/02/09 18:59:34 christos Exp $  */
 
 /*
@@ -519,7 +519,7 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
        struct proc *p;
        pid_t tid;
        vaddr_t uaddr;
-       int error;
+       int s, error;
 
        if (stack == NULL)
                return EINVAL;
@@ -543,6 +543,7 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 
        /* other links */
        p->p_p = pr;
+       pr->ps_threadcnt++;
 
        /* local copies */
        p->p_fd         = pr->ps_fd;
@@ -561,17 +562,18 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
        LIST_INSERT_HEAD(&allproc, p, p_list);
        LIST_INSERT_HEAD(TIDHASH(p->p_tid), p, p_hash);
 
-       mtx_enter(&pr->ps_mtx);
+       SCHED_LOCK(s);
        TAILQ_INSERT_TAIL(&pr->ps_threads, p, p_thr_link);
-       pr->ps_threadcnt++;
 
        /*
         * if somebody else wants to take us to single threaded mode,
         * count ourselves in.
         */
-       if (pr->ps_single)
+       if (pr->ps_single) {
+               atomic_inc_int(&pr->ps_singlecount);
                atomic_setbits_int(&p->p_flag, P_SUSPSINGLE);
-       mtx_leave(&pr->ps_mtx);
+       }
+       SCHED_UNLOCK(s);
 
        /*
         * Return tid to parent thread and copy it out to userspace
index 2237ed4..9885396 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_resource.c,v 1.79 2023/09/08 09:06:31 claudio Exp $      */
+/*     $OpenBSD: kern_resource.c,v 1.80 2023/09/13 14:25:49 claudio Exp $      */
 /*     $NetBSD: kern_resource.c,v 1.38 1996/10/23 07:19:38 matthias Exp $      */
 
 /*-
@@ -212,13 +212,11 @@ donice(struct proc *curp, struct process *chgpr, int n)
        if (n < chgpr->ps_nice && suser(curp))
                return (EACCES);
        chgpr->ps_nice = n;
-       mtx_enter(&chgpr->ps_mtx);
+       SCHED_LOCK(s);
        TAILQ_FOREACH(p, &chgpr->ps_threads, p_thr_link) {
-               SCHED_LOCK(s);
                setpriority(p, p->p_estcpu, n);
-               SCHED_UNLOCK(s);
        }
-       mtx_leave(&chgpr->ps_mtx);
+       SCHED_UNLOCK(s);
        return (0);
 }
 
@@ -478,9 +476,8 @@ dogetrusage(struct proc *p, int who, struct rusage *rup)
        struct process *pr = p->p_p;
        struct proc *q;
 
-       KERNEL_ASSERT_LOCKED();
-
        switch (who) {
+
        case RUSAGE_SELF:
                /* start with the sum of dead threads, if any */
                if (pr->ps_ru != NULL)
index c2a1530..06b6a9e 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_sig.c,v 1.316 2023/09/09 14:50:09 claudio Exp $  */
+/*     $OpenBSD: kern_sig.c,v 1.317 2023/09/13 14:25:49 claudio Exp $  */
 /*     $NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $   */
 
 /*
@@ -2008,12 +2008,11 @@ userret(struct proc *p)
 }
 
 int
-single_thread_check_locked(struct proc *p, int deep)
+single_thread_check_locked(struct proc *p, int deep, int s)
 {
        struct process *pr = p->p_p;
-       int s, wake;
 
-       MUTEX_ASSERT_LOCKED(&pr->ps_mtx);
+       SCHED_ASSERT_LOCKED();
 
        if (pr->ps_single == NULL || pr->ps_single == p)
                return (0);
@@ -2027,24 +2026,19 @@ single_thread_check_locked(struct proc *p, int deep)
                                return (EINTR);
                }
 
+               if (atomic_dec_int_nv(&pr->ps_singlecount) == 0)
+                       wakeup(&pr->ps_singlecount);
+
                if (pr->ps_flags & PS_SINGLEEXIT) {
-                       mtx_leave(&pr->ps_mtx);
+                       SCHED_UNLOCK(s);
                        KERNEL_LOCK();
                        exit1(p, 0, 0, EXIT_THREAD_NOCHECK);
                        /* NOTREACHED */
                }
 
                /* not exiting and don't need to unwind, so suspend */
-               wake = (++pr->ps_singlecnt == pr->ps_threadcnt);
-               mtx_leave(&pr->ps_mtx);
-               if (wake)
-                       wakeup(&pr->ps_singlecnt);
-
-               SCHED_LOCK(s);
                p->p_stat = SSTOP;
                mi_switch();
-               SCHED_UNLOCK(s);
-               mtx_enter(&pr->ps_mtx);
        } while (pr->ps_single != NULL);
 
        return (0);
@@ -2053,11 +2047,11 @@ single_thread_check_locked(struct proc *p, int deep)
 int
 single_thread_check(struct proc *p, int deep)
 {
-       int error;
+       int s, error;
 
-       mtx_enter(&p->p_p->ps_mtx);
-       error = single_thread_check_locked(p, deep);
-       mtx_leave(&p->p_p->ps_mtx);
+       SCHED_LOCK(s);
+       error = single_thread_check_locked(p, deep, s);
+       SCHED_UNLOCK(s);
 
        return error;
 }
@@ -2077,14 +2071,13 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
        struct process *pr = p->p_p;
        struct proc *q;
        int error, s;
-       u_int count = 0;
 
        KASSERT(curproc == p);
 
-       mtx_enter(&pr->ps_mtx);
-       error = single_thread_check_locked(p, (mode == SINGLE_UNWIND));
+       SCHED_LOCK(s);
+       error = single_thread_check_locked(p, (mode == SINGLE_UNWIND), s);
        if (error) {
-               mtx_leave(&pr->ps_mtx);
+               SCHED_UNLOCK(s);
                return error;
        }
 
@@ -2103,25 +2096,26 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
                panic("single_thread_mode = %d", mode);
 #endif
        }
-       pr->ps_singlecnt = 1; /* count ourselfs in already */
+       pr->ps_singlecount = 0;
+       membar_producer();
        pr->ps_single = p;
-
        TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
                if (q == p)
                        continue;
                if (q->p_flag & P_WEXIT) {
-                       SCHED_LOCK(s);
-                       if (mode == SINGLE_EXIT && q->p_stat == SSTOP)
-                               setrunnable(q);
-                       SCHED_UNLOCK(s);
+                       if (mode == SINGLE_EXIT) {
+                               if (q->p_stat == SSTOP) {
+                                       setrunnable(q);
+                                       atomic_inc_int(&pr->ps_singlecount);
+                               }
+                       }
                        continue;
                }
-               SCHED_LOCK(s);
                atomic_setbits_int(&q->p_flag, P_SUSPSINGLE);
                switch (q->p_stat) {
                case SIDL:
-               case SDEAD:
                case SRUN:
+                       atomic_inc_int(&pr->ps_singlecount);
                        break;
                case SSLEEP:
                        /* if it's not interruptible, then just have to wait */
@@ -2129,29 +2123,28 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
                                /* merely need to suspend?  just stop it */
                                if (mode == SINGLE_SUSPEND) {
                                        q->p_stat = SSTOP;
-                                       count++;
                                        break;
                                }
                                /* need to unwind or exit, so wake it */
                                setrunnable(q);
                        }
+                       atomic_inc_int(&pr->ps_singlecount);
                        break;
                case SSTOP:
                        if (mode == SINGLE_EXIT) {
                                setrunnable(q);
-                               break;
+                               atomic_inc_int(&pr->ps_singlecount);
                        }
-                       count++;
+                       break;
+               case SDEAD:
                        break;
                case SONPROC:
+                       atomic_inc_int(&pr->ps_singlecount);
                        signotify(q);
                        break;
                }
-               SCHED_UNLOCK(s);
        }
-
-       pr->ps_singlecnt += count;
-       mtx_leave(&pr->ps_mtx);
+       SCHED_UNLOCK(s);
 
        if (wait)
                single_thread_wait(pr, 1);
@@ -2170,16 +2163,14 @@ single_thread_wait(struct process *pr, int recheck)
        int wait;
 
        /* wait until they're all suspended */
-       mtx_enter(&pr->ps_mtx);
-       wait = pr->ps_singlecnt < pr->ps_threadcnt;
+       wait = pr->ps_singlecount > 0;
        while (wait) {
-               msleep_nsec(&pr->ps_singlecnt, &pr->ps_mtx, PWAIT, "suspend",
-                   INFSLP);
+               sleep_setup(&pr->ps_singlecount, PWAIT, "suspend");
+               wait = pr->ps_singlecount > 0;
+               sleep_finish(0, wait);
                if (!recheck)
                        break;
-               wait = pr->ps_singlecnt < pr->ps_threadcnt;
        }
-       mtx_leave(&pr->ps_mtx);
 
        return wait;
 }
@@ -2194,10 +2185,9 @@ single_thread_clear(struct proc *p, int flag)
        KASSERT(pr->ps_single == p);
        KASSERT(curproc == p);
 
-       mtx_enter(&pr->ps_mtx);
+       SCHED_LOCK(s);
        pr->ps_single = NULL;
        atomic_clearbits_int(&pr->ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
-
        TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
                if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
                        continue;
@@ -2208,7 +2198,6 @@ single_thread_clear(struct proc *p, int flag)
                 * then clearing that either makes it runnable or puts
                 * it back into some sleep queue
                 */
-               SCHED_LOCK(s);
                if (q->p_stat == SSTOP && (q->p_flag & flag) == 0) {
                        if (q->p_wchan == NULL)
                                setrunnable(q);
@@ -2217,9 +2206,8 @@ single_thread_clear(struct proc *p, int flag)
                                q->p_stat = SSLEEP;
                        }
                }
-               SCHED_UNLOCK(s);
        }
-       mtx_leave(&pr->ps_mtx);
+       SCHED_UNLOCK(s);
 }
 
 void
index 06db10f..b4f0c27 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_synch.c,v 1.199 2023/09/08 09:06:31 claudio Exp $        */
+/*     $OpenBSD: kern_synch.c,v 1.200 2023/09/13 14:25:49 claudio Exp $        */
 /*     $NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $ */
 
 /*
@@ -566,18 +566,15 @@ sys_sched_yield(struct proc *p, void *v, register_t *retval)
        uint8_t newprio;
        int s;
 
+       SCHED_LOCK(s);
        /*
         * If one of the threads of a multi-threaded process called
         * sched_yield(2), drop its priority to ensure its siblings
         * can make some progress.
         */
-       mtx_enter(&p->p_p->ps_mtx);
        newprio = p->p_usrpri;
        TAILQ_FOREACH(q, &p->p_p->ps_threads, p_thr_link)
                newprio = max(newprio, q->p_runpri);
-       mtx_leave(&p->p_p->ps_mtx);
-
-       SCHED_LOCK(s);
        setrunqueue(p->p_cpu, p, newprio);
        p->p_ru.ru_nvcsw++;
        mi_switch();
index 9484a8b..3410438 100644 (file)
@@ -1,4 +1,4 @@
-/*     $OpenBSD: proc.h,v 1.350 2023/09/08 09:06:31 claudio Exp $      */
+/*     $OpenBSD: proc.h,v 1.351 2023/09/13 14:25:49 claudio Exp $      */
 /*     $NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $       */
 
 /*-
@@ -138,7 +138,7 @@ struct process {
        struct  ucred *ps_ucred;        /* Process owner's identity. */
 
        LIST_ENTRY(process) ps_list;    /* List of all processes. */
-       TAILQ_HEAD(,proc) ps_threads;   /* [K|m] Threads in this process. */
+       TAILQ_HEAD(,proc) ps_threads;   /* [K|S] Threads in this process. */
 
        LIST_ENTRY(process) ps_pglist;  /* List of processes in pgrp. */
        struct  process *ps_pptr;       /* Pointer to parent process. */
@@ -173,8 +173,8 @@ struct process {
        u_int   ps_flags;               /* [a] PS_* flags. */
        int     ps_siglist;             /* Signals pending for the process. */
 
-       struct  proc *ps_single;        /* [m] Thread for single-threading. */
-       u_int   ps_singlecnt;           /* [m] Number of suspended threads. */
+       struct  proc *ps_single;        /* [S] Thread for single-threading. */
+       u_int   ps_singlecount;         /* [a] Not yet suspended threads. */
 
        int     ps_traceflag;           /* Kernel trace points. */
        struct  vnode *ps_tracevp;      /* Trace to vnode. */
@@ -242,7 +242,7 @@ struct process {
 
 /* End area that is copied on creation. */
 #define ps_endcopy     ps_threadcnt
-       u_int   ps_threadcnt;           /* [m] Number of threads. */
+       u_int   ps_threadcnt;           /* Number of threads. */
 
        struct  timespec ps_start;      /* starting uptime. */
        struct  timeout ps_realit_to;   /* [m] ITIMER_REAL timeout */
@@ -310,14 +310,13 @@ struct p_inentry {
  *     U       uidinfolk
  *     l       read only reference, see lim_read_enter()
  *     o       owned (read/modified only) by this thread
- *     m       this proc's' `p->p_p->ps_mtx'
  */
 struct proc {
        TAILQ_ENTRY(proc) p_runq;       /* [S] current run/sleep queue */
        LIST_ENTRY(proc) p_list;        /* List of all threads. */
 
        struct  process *p_p;           /* [I] The process of this thread. */
-       TAILQ_ENTRY(proc) p_thr_link;   /* [K|m] Threads in a process linkage. */
+       TAILQ_ENTRY(proc) p_thr_link;   /* Threads in a process linkage. */
 
        TAILQ_ENTRY(proc) p_fut_link;   /* Threads in a futex linkage. */
        struct  futex   *p_futex;       /* Current sleeping futex. */