From c1102e9f24b16377f5fe3cbd55828be64335bded Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Fri, 2 Dec 2005 22:02:20 +0000 Subject: [PATCH] Fix a process exit/wait race. The wait*() code was making a faulty test to determine that the exiting process had completely exited and was no longer running. Testing the TDF_RUNNING flag is insufficient because an exiting process may block at various points after becoming a Zombie, but before it deschedules itself for the last time. Add a new flag, TDF_EXITING, which is set just prior to a thread descheduling itself for the last time. The reaper then checks that TDF_EXITING is set and TDF_RUNNING is clear. Fix a second faulty test in both the exit and the thread cpu migration code. If a thread gets preempted, TDF_RUNNING will be temporarily cleared, so testing TDF_RUNNING is not sufficient by itself. We must also test the TDF_PREEMPT_LOCK flag to be sure that it is also clear. So the grand result is that to really be sure the zombie process has been completely descheduled and is no longer running or will ever run again, the TDF_EXITING, TDF_RUNNING, *and* TDF_PREEMPT_LOCK flags must be tested and all must be clear except for TDF_EXITING. It should be noted that TDF_RUNNING on the previously scheduled process is always cleared AFTER we have context-switched into the next scheduled thread or the idle thread, so seeing a cleared TDF_RUNNING along with the appropriate state for the other flags does in fact guarentee that the thread in question is no longer using its stack in any way. Reported-by: Stefan Krueger --- sys/i386/i386/vm_machdep.c | 3 ++- sys/kern/kern_exit.c | 40 +++++++++++++++++------------ sys/kern/lwkt_thread.c | 6 ++--- sys/platform/pc32/i386/vm_machdep.c | 3 ++- sys/sys/thread.h | 7 +++-- 5 files changed, 36 insertions(+), 23 deletions(-) diff --git a/sys/i386/i386/vm_machdep.c b/sys/i386/i386/vm_machdep.c index cd86549866..e2ceae6361 100644 --- a/sys/i386/i386/vm_machdep.c +++ b/sys/i386/i386/vm_machdep.c @@ -39,7 +39,7 @@ * from: @(#)vm_machdep.c 7.3 (Berkeley) 5/13/91 * Utah $Hdr: vm_machdep.c 1.16.1.1 89/06/23$ * $FreeBSD: src/sys/i386/i386/vm_machdep.c,v 1.132.2.9 2003/01/25 19:02:23 dillon Exp $ - * $DragonFly: src/sys/i386/i386/Attic/vm_machdep.c,v 1.38 2005/11/04 08:57:27 dillon Exp $ + * $DragonFly: src/sys/i386/i386/Attic/vm_machdep.c,v 1.39 2005/12/02 22:02:16 dillon Exp $ */ #include "use_npx.h" @@ -294,6 +294,7 @@ void cpu_thread_exit(void) { curthread->td_switch = cpu_exit_switch; + curthread->td_flags |= TDF_EXITING; lwkt_switch(); panic("cpu_exit"); } diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c index 948b95b1e4..d3cdd9bd5b 100644 --- a/sys/kern/kern_exit.c +++ b/sys/kern/kern_exit.c @@ -37,7 +37,7 @@ * * @(#)kern_exit.c 8.7 (Berkeley) 2/12/94 * $FreeBSD: src/sys/kern/kern_exit.c,v 1.92.2.11 2003/01/13 22:51:16 dillon Exp $ - * $DragonFly: src/sys/kern/kern_exit.c,v 1.50 2005/12/01 18:30:08 dillon Exp $ + * $DragonFly: src/sys/kern/kern_exit.c,v 1.51 2005/12/02 22:02:17 dillon Exp $ */ #include "opt_compat.h" @@ -310,8 +310,10 @@ exit1(int rv) /* * Once we set SZOMB the process can get reaped. The wait1 code - * will also wait for TDF_RUNNING to be cleared in the thread's flags, - * indicating that it has been completely switched out. + * will also wait for TDF_EXITING to be set and for both TDF_RUNNING + * and TDF_PREEMPT_LOCK to be cleared in the thread's flags, + * indicating that it has been completely switched out for the last + * time. */ /* @@ -319,7 +321,9 @@ exit1(int rv) * Place onto zombproc. Unlink from parent's child list. * * Interlock the SZOMB state with a tsleep against p_lock - * (PHOLD/PRELE) so allproc loops don't get confused. + * (PHOLD/PRELE) so allproc loops don't get confused. Get + * our own ref on p_lock to prevent us from getting reaped + * too early. */ LIST_REMOVE(p, p_list); LIST_INSERT_HEAD(&zombproc, p, p_list); @@ -468,18 +472,6 @@ loop: nfound++; if (p->p_flag & P_ZOMBIE) { - /* - * The process's thread may still be in the middle - * of switching away, we can't rip its stack out from - * under it until TDF_RUNNING clears! - * - * YYY no wakeup occurs so we depend on the timeout. - */ - if ((p->p_thread->td_flags & TDF_RUNNING) != 0) { - tsleep(p->p_thread, 0, "reap2", 1); - goto loop; - } - /* * Other kernel threads may be in the middle of * accessing the proc. For example, kern/kern_proc.c @@ -493,6 +485,22 @@ loop: } lwkt_wait_free(p->p_thread); + /* + * The process's thread may still be in the middle + * of switching away, we can't rip its stack out from + * under it until TDF_EXITING is set and both + * TDF_RUNNING and TDF_PREEMPT_LOCK are clear. + * TDF_PREEMPT_LOCK must be checked because TDF_RUNNING + * will be cleared temporarily if a thread gets + * preempted. + * + * YYY no wakeup occurs so we depend on the timeout. + */ + if ((p->p_thread->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK|TDF_EXITING)) != TDF_EXITING) { + tsleep(p->p_thread, 0, "reap2", 1); + goto loop; + } + /* scheduling hook for heuristic */ p->p_usched->heuristic_exiting(td->td_lwp, &p->p_lwp); diff --git a/sys/kern/lwkt_thread.c b/sys/kern/lwkt_thread.c index 511412411a..fbdb8084aa 100644 --- a/sys/kern/lwkt_thread.c +++ b/sys/kern/lwkt_thread.c @@ -31,7 +31,7 @@ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. * - * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.88 2005/11/22 08:41:03 dillon Exp $ + * $DragonFly: src/sys/kern/lwkt_thread.c,v 1.89 2005/12/02 22:02:17 dillon Exp $ */ /* @@ -1072,7 +1072,7 @@ lwkt_acquire(thread_t td) mygd = mycpu; cpu_lfence(); KKASSERT((td->td_flags & TDF_RUNQ) == 0); - while (td->td_flags & TDF_RUNNING) /* XXX spin */ + while (td->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK)) /* XXX spin */ cpu_lfence(); if (gd != mygd) { crit_enter_gd(mygd); @@ -1232,7 +1232,7 @@ lwkt_setcpu_remote(void *arg) thread_t td = arg; globaldata_t gd = mycpu; - while (td->td_flags & TDF_RUNNING) + while (td->td_flags & (TDF_RUNNING|TDF_PREEMPT_LOCK)) cpu_lfence(); td->td_gd = gd; cpu_sfence(); diff --git a/sys/platform/pc32/i386/vm_machdep.c b/sys/platform/pc32/i386/vm_machdep.c index 97813300fd..9d3e1c2b49 100644 --- a/sys/platform/pc32/i386/vm_machdep.c +++ b/sys/platform/pc32/i386/vm_machdep.c @@ -39,7 +39,7 @@ * from: @(#)vm_machdep.c 7.3 (Berkeley) 5/13/91 * Utah $Hdr: vm_machdep.c 1.16.1.1 89/06/23$ * $FreeBSD: src/sys/i386/i386/vm_machdep.c,v 1.132.2.9 2003/01/25 19:02:23 dillon Exp $ - * $DragonFly: src/sys/platform/pc32/i386/vm_machdep.c,v 1.38 2005/11/04 08:57:27 dillon Exp $ + * $DragonFly: src/sys/platform/pc32/i386/vm_machdep.c,v 1.39 2005/12/02 22:02:16 dillon Exp $ */ #include "use_npx.h" @@ -294,6 +294,7 @@ void cpu_thread_exit(void) { curthread->td_switch = cpu_exit_switch; + curthread->td_flags |= TDF_EXITING; lwkt_switch(); panic("cpu_exit"); } diff --git a/sys/sys/thread.h b/sys/sys/thread.h index f1c2ab7f48..accd0ca438 100644 --- a/sys/sys/thread.h +++ b/sys/sys/thread.h @@ -7,7 +7,7 @@ * Types which must already be defined when this header is included by * userland: struct md_thread * - * $DragonFly: src/sys/sys/thread.h,v 1.75 2005/11/22 08:41:05 dillon Exp $ + * $DragonFly: src/sys/sys/thread.h,v 1.76 2005/12/02 22:02:20 dillon Exp $ */ #ifndef _SYS_THREAD_H_ @@ -274,7 +274,9 @@ struct thread { * Thread flags. Note that TDF_RUNNING is cleared on the old thread after * we switch to the new one, which is necessary because LWKTs don't need * to hold the BGL. This flag is used by the exit code and the managed - * thread migration code. + * thread migration code. Note in addition that preemption will cause + * TDF_RUNNING to be cleared temporarily, so any code checking TDF_RUNNING + * must also check TDF_PREEMPT_LOCK. * * LWKT threads stay on their (per-cpu) run queue while running, not to * be confused with user processes which are removed from the user scheduling @@ -308,6 +310,7 @@ struct thread { #define TDF_PANICWARN 0x00080000 /* panic warning in switch */ #define TDF_BLOCKQ 0x00100000 /* on block queue */ #define TDF_MPSAFE 0x00200000 /* (thread creation) */ +#define TDF_EXITING 0x00400000 /* thread exiting */ /* * Thread priorities. Typically only one thread from any given -- 2.41.0