Date: Fri, 15 May 2015 07:54:32 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r282944 - in head/sys: kern sys Message-ID: <201505150754.t4F7sWsq077437@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: kib Date: Fri May 15 07:54:31 2015 New Revision: 282944 URL: https://svnweb.freebsd.org/changeset/base/282944 Log: Right now, the process' p_boundary_count counter is decremented by the suspended thread itself, on the return path from thread_suspend_check(). A consequence is that return from thread_single_end(SINGLE_BOUNDARY) may leave p_boundary_count non-zero, it might be even equal to the threads count. Now, assume that we have two threads in the process, both calling execve(2). Suppose that the first thread won the race to be the suspension thread, and that afterward its exec failed for any reason. After the first thread did thread_single_end(SINGLE_BOUNDARY), second thread becomes the process suspension thread and checks p_boundary_count. The non-zero value of the count allows the suspension loop to finish without actually suspending some threads. In other words, we enter exec code with some threads not suspended. Fix this by decrementing p_boundary_count in the thread_single_end()->thread_unsuspend_one() during marking the thread as runnable. This way, a return from thread_single_end() guarantees that the counter is cleared. We do not care whether the unsuspended thread has a chance to run. Add some asserts to ensure the state of the process when single boundary suspension is lifted. Also make thread_unuspend_one() static. In collaboration with: pho Sponsored by: The FreeBSD Foundation MFC after: 1 week Modified: head/sys/kern/kern_thread.c head/sys/sys/proc.h Modified: head/sys/kern/kern_thread.c ============================================================================== --- head/sys/kern/kern_thread.c Fri May 15 07:07:37 2015 (r282943) +++ head/sys/kern/kern_thread.c Fri May 15 07:54:31 2015 (r282944) @@ -74,6 +74,8 @@ static struct mtx zombie_lock; MTX_SYSINIT(zombie_lock, &zombie_lock, "zombie lock", MTX_SPIN); static void thread_zombie(struct thread *); +static int thread_unsuspend_one(struct thread *td, struct proc *p, + bool boundary); #define TID_BUFFER_SIZE 1024 @@ -445,7 +447,7 @@ thread_exit(void) if (p->p_numthreads == p->p_suspcount) { thread_lock(p->p_singlethread); wakeup_swapper = thread_unsuspend_one( - p->p_singlethread, p); + p->p_singlethread, p, false); thread_unlock(p->p_singlethread); if (wakeup_swapper) kick_proc0(); @@ -603,19 +605,19 @@ weed_inhib(int mode, struct thread *td2, switch (mode) { case SINGLE_EXIT: if (TD_IS_SUSPENDED(td2)) - wakeup_swapper |= thread_unsuspend_one(td2, p); + wakeup_swapper |= thread_unsuspend_one(td2, p, true); if (TD_ON_SLEEPQ(td2) && (td2->td_flags & TDF_SINTR) != 0) wakeup_swapper |= sleepq_abort(td2, EINTR); break; case SINGLE_BOUNDARY: if (TD_IS_SUSPENDED(td2) && (td2->td_flags & TDF_BOUNDARY) == 0) - wakeup_swapper |= thread_unsuspend_one(td2, p); + wakeup_swapper |= thread_unsuspend_one(td2, p, false); if (TD_ON_SLEEPQ(td2) && (td2->td_flags & TDF_SINTR) != 0) wakeup_swapper |= sleepq_abort(td2, ERESTART); break; case SINGLE_NO_EXIT: if (TD_IS_SUSPENDED(td2) && (td2->td_flags & TDF_BOUNDARY) == 0) - wakeup_swapper |= thread_unsuspend_one(td2, p); + wakeup_swapper |= thread_unsuspend_one(td2, p, false); if (TD_ON_SLEEPQ(td2) && (td2->td_flags & TDF_SINTR) != 0) wakeup_swapper |= sleepq_abort(td2, ERESTART); break; @@ -630,7 +632,7 @@ weed_inhib(int mode, struct thread *td2, */ if (TD_IS_SUSPENDED(td2) && (td2->td_flags & (TDF_BOUNDARY | TDF_ALLPROCSUSP)) == 0) - wakeup_swapper |= thread_unsuspend_one(td2, p); + wakeup_swapper |= thread_unsuspend_one(td2, p, false); if (TD_ON_SLEEPQ(td2) && (td2->td_flags & TDF_SINTR) != 0) { if ((td2->td_flags & TDF_SBDRY) == 0) { thread_suspend_one(td2); @@ -898,8 +900,8 @@ thread_suspend_check(int return_instead) if (P_SHOULDSTOP(p) == P_STOPPED_SINGLE) { if (p->p_numthreads == p->p_suspcount + 1) { thread_lock(p->p_singlethread); - wakeup_swapper = - thread_unsuspend_one(p->p_singlethread, p); + wakeup_swapper = thread_unsuspend_one( + p->p_singlethread, p, false); thread_unlock(p->p_singlethread); if (wakeup_swapper) kick_proc0(); @@ -918,15 +920,8 @@ thread_suspend_check(int return_instead) } PROC_SUNLOCK(p); mi_switch(SW_INVOL | SWT_SUSPEND, NULL); - if (return_instead == 0) - td->td_flags &= ~TDF_BOUNDARY; thread_unlock(td); PROC_LOCK(p); - if (return_instead == 0) { - PROC_SLOCK(p); - p->p_boundary_count--; - PROC_SUNLOCK(p); - } } return (0); } @@ -975,8 +970,8 @@ thread_suspend_one(struct thread *td) sched_sleep(td, 0); } -int -thread_unsuspend_one(struct thread *td, struct proc *p) +static int +thread_unsuspend_one(struct thread *td, struct proc *p, bool boundary) { THREAD_LOCK_ASSERT(td, MA_OWNED); @@ -986,6 +981,10 @@ thread_unsuspend_one(struct thread *td, if (td->td_proc == p) { PROC_SLOCK_ASSERT(p, MA_OWNED); p->p_suspcount--; + if (boundary && (td->td_flags & TDF_BOUNDARY) != 0) { + td->td_flags &= ~TDF_BOUNDARY; + p->p_boundary_count--; + } } return (setrunnable(td)); } @@ -1006,12 +1005,13 @@ thread_unsuspend(struct proc *p) FOREACH_THREAD_IN_PROC(p, td) { thread_lock(td); if (TD_IS_SUSPENDED(td)) { - wakeup_swapper |= thread_unsuspend_one(td, p); + wakeup_swapper |= thread_unsuspend_one(td, p, + true); } thread_unlock(td); } - } else if ((P_SHOULDSTOP(p) == P_STOPPED_SINGLE) && - (p->p_numthreads == p->p_suspcount)) { + } else if (P_SHOULDSTOP(p) == P_STOPPED_SINGLE && + p->p_numthreads == p->p_suspcount) { /* * Stopping everything also did the job for the single * threading request. Now we've downgraded to single-threaded, @@ -1020,7 +1020,7 @@ thread_unsuspend(struct proc *p) if (p->p_singlethread->td_proc == p) { thread_lock(p->p_singlethread); wakeup_swapper = thread_unsuspend_one( - p->p_singlethread, p); + p->p_singlethread, p, false); thread_unlock(p->p_singlethread); } } @@ -1044,6 +1044,12 @@ thread_single_end(struct proc *p, int mo KASSERT((mode == SINGLE_ALLPROC && (p->p_flag & P_TOTAL_STOP) != 0) || (mode != SINGLE_ALLPROC && (p->p_flag & P_TOTAL_STOP) == 0), ("mode %d does not match P_TOTAL_STOP", mode)); + KASSERT(mode == SINGLE_ALLPROC || p->p_singlethread == curthread, + ("thread_single_end from other thread %p %p", + curthread, p->p_singlethread)); + KASSERT(mode != SINGLE_BOUNDARY || + (p->p_flag & P_SINGLE_BOUNDARY) != 0, + ("mis-matched SINGLE_BOUNDARY flags %x", p->p_flag)); p->p_flag &= ~(P_STOPPED_SINGLE | P_SINGLE_EXIT | P_SINGLE_BOUNDARY | P_TOTAL_STOP); PROC_SLOCK(p); @@ -1059,11 +1065,14 @@ thread_single_end(struct proc *p, int mo FOREACH_THREAD_IN_PROC(p, td) { thread_lock(td); if (TD_IS_SUSPENDED(td)) { - wakeup_swapper |= thread_unsuspend_one(td, p); + wakeup_swapper |= thread_unsuspend_one(td, p, + mode == SINGLE_BOUNDARY); } thread_unlock(td); } } + KASSERT(mode != SINGLE_BOUNDARY || p->p_boundary_count == 0, + ("inconsistent boundary count %d", p->p_boundary_count)); PROC_SUNLOCK(p); if (wakeup_swapper) kick_proc0(); Modified: head/sys/sys/proc.h ============================================================================== --- head/sys/sys/proc.h Fri May 15 07:07:37 2015 (r282943) +++ head/sys/sys/proc.h Fri May 15 07:54:31 2015 (r282944) @@ -991,7 +991,6 @@ void thread_suspend_switch(struct thread void thread_suspend_one(struct thread *td); void thread_unlink(struct thread *td); void thread_unsuspend(struct proc *p); -int thread_unsuspend_one(struct thread *td, struct proc *p); void thread_wait(struct proc *p); struct thread *thread_find(struct proc *p, lwpid_t tid);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201505150754.t4F7sWsq077437>