Reimplement stack capture of running threads on i386 and amd64.

After r355784 the td_oncpu field is no longer synchronized by the thread lock, so the stack capture interrupt cannot be delievered precisely. Fix this using a loop which drops the thread lock and restarts if the wrong thread was sampled from the stack capture interrupt handler. Change the implementation to use a regular interrupt instead of an NMI. Now that we drop the thread lock, there is no advantage to the latter. Simplify the KPIs. Remove stack_save_td_running() and add a return value to stack_save_td(). On platforms that do not support stack capture of running threads, stack_save_td() returns EOPNOTSUPP. If the target thread is running in user mode, stack_save_td() returns EBUSY. Reviewed by: kib Reported by: mjg, pho Tested by: pho Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D23355
trociny · Jan 31, 2020 · 97176df · 97176df
1 parent 832e6ed
commit 97176df
Show file tree

Hide file tree

Showing 18 changed files with 147 additions and 183 deletions.
diff --git a/share/man/man9/stack.9 b/share/man/man9/stack.9
@@ -27,7 +27,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd October 6, 2017
+.Dd January 31, 2020
 .Dt STACK 9
 .Os
 .Sh NAME
@@ -65,10 +65,8 @@ In the kernel configuration file:
 .Fn stack_sbuf_print_ddb "struct sbuf sb*" "const struct stack *st"
 .Ft void
 .Fn stack_save "struct stack *st"
-.Ft void
-.Fn stack_save_td "struct stack *st" "struct thread *td"
 .Ft int
-.Fn stack_save_td_running "struct stack *st" "struct thread *td"
+.Fn stack_save_td "struct stack *st" "struct thread *td"
 .Sh DESCRIPTION
 The
 .Nm
@@ -93,18 +91,17 @@ argument is passed to
 Memory associated with a trace is freed by calling
 .Fn stack_destroy .
 .Pp
-A trace of the current kernel thread's call stack may be captured using
+A trace of the current thread's kernel call stack may be captured using
 .Fn stack_save .
 .Fn stack_save_td
-and
-.Fn stack_save_td_running
-can also be used to capture the stack of a caller-specified thread.
-Callers of these functions must own the thread lock of the specified thread.
+can be used to capture the kernel stack of a caller-specified thread.
+Callers of these functions must own the thread lock of the specified thread,
+and the thread's stack must not be swapped out.
 .Fn stack_save_td
-can capture the stack of a kernel thread that is not running or
-swapped out at the time of the call.
-.Fn stack_save_td_running
-can capture the stack of a running kernel thread.
+can capture the kernel stack of a running thread, though note that this is
+not implemented on all platforms.
+If the thread is running, the caller must also hold the process lock for the
+target thread.
 .Pp
 .Fn stack_print
 and
@@ -157,11 +154,11 @@ Otherwise the
 does not contain space to record additional frames, and a non-zero value is
 returned.
 .Pp
-.Fn stack_save_td_running
+.Fn stack_save_td
 returns 0 when the stack capture was successful and a non-zero error number
 otherwise.
 In particular,
-.Er EAGAIN
+.Er EBUSY
 is returned if the thread was running in user mode at the time that the
 capture was attempted, and
 .Er EOPNOTSUPP

diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
@@ -52,7 +52,6 @@ __FBSDID("$FreeBSD$");
 #include "opt_hwpmc_hooks.h"
 #include "opt_isa.h"
 #include "opt_kdb.h"
-#include "opt_stack.h"
 
 #include <sys/param.h>
 #include <sys/bus.h>
@@ -228,11 +227,6 @@ trap(struct trapframe *frame)
 		    (*pmc_intr)(frame) != 0)
 			return;
 #endif
-
-#ifdef STACK
-		if (stack_nmi_handler(frame) != 0)
-			return;
-#endif
 	}
 
 	if ((frame->tf_rflags & PSL_I) == 0) {

diff --git a/sys/arm/arm/stack_machdep.c b/sys/arm/arm/stack_machdep.c
@@ -30,8 +30,11 @@ __FBSDID("$FreeBSD$");
 
 #include <sys/types.h>
 #include <sys/systm.h>
+#include <sys/lock.h>
+#include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/stack.h>
+
 #include <machine/pcb.h>
 #include <machine/stack.h>
 
@@ -63,29 +66,23 @@ stack_save(struct stack *st)
 	stack_capture(st, &state);
 }
 
-void
+int
 stack_save_td(struct stack *st, struct thread *td)
 {
 	struct unwind_state state;
 
-	KASSERT(!TD_IS_SWAPPED(td), ("stack_save_td: swapped"));
-	KASSERT(!TD_IS_RUNNING(td), ("stack_save_td: running"));
+	THREAD_LOCK_ASSERT(td, MA_OWNED);
+	KASSERT(!TD_IS_SWAPPED(td),
+	    ("stack_save_td: thread %p is swapped", td));
+
+	if (TD_IS_RUNNING(td))
+		return (EOPNOTSUPP);
 
 	state.registers[FP] = td->td_pcb->pcb_regs.sf_r11;
 	state.registers[SP] = td->td_pcb->pcb_regs.sf_sp;
 	state.registers[LR] = td->td_pcb->pcb_regs.sf_lr;
 	state.registers[PC] = td->td_pcb->pcb_regs.sf_pc;
 
 	stack_capture(st, &state);
-}
-
-int
-stack_save_td_running(struct stack *st, struct thread *td)
-{
-
-	if (td == curthread) {
-		stack_save(st);
-		return (0);
-	}
-	return (EOPNOTSUPP);
+	return (0);
 }
diff --git a/sys/arm64/arm64/stack_machdep.c b/sys/arm64/arm64/stack_machdep.c
@@ -33,6 +33,8 @@ __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
 #include <sys/systm.h>
+#include <sys/lock.h>
+#include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/stack.h>
 
@@ -55,28 +57,24 @@ stack_capture(struct stack *st, struct unwind_state *frame)
 	}
 }
 
-void
+int
 stack_save_td(struct stack *st, struct thread *td)
 {
 	struct unwind_state frame;
 
-	if (TD_IS_SWAPPED(td))
-		panic("stack_save_td: swapped");
+	THREAD_LOCK_ASSERT(td, MA_OWNED);
+	KASSERT(!TD_IS_SWAPPED(td),
+	    ("stack_save_td: thread %p is swapped", td));
+
 	if (TD_IS_RUNNING(td))
-		panic("stack_save_td: running");
+		return (EOPNOTSUPP);
 
 	frame.sp = td->td_pcb->pcb_sp;
 	frame.fp = td->td_pcb->pcb_x[29];
 	frame.pc = td->td_pcb->pcb_x[30];
 
 	stack_capture(st, &frame);
-}
-
-int
-stack_save_td_running(struct stack *st, struct thread *td)
-{
-
-	return (EOPNOTSUPP);
+	return (0);
 }
 
 void

diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
@@ -52,7 +52,6 @@ __FBSDID("$FreeBSD$");
 #include "opt_hwpmc_hooks.h"
 #include "opt_isa.h"
 #include "opt_kdb.h"
-#include "opt_stack.h"
 #include "opt_trap.h"
 
 #include <sys/param.h>
@@ -248,11 +247,6 @@ trap(struct trapframe *frame)
 		    (*pmc_intr)(frame) != 0)
 			return;
 #endif
-
-#ifdef STACK
-		if (stack_nmi_handler(frame) != 0)
-			return;
-#endif
 	}
 
 	if (type == T_MCHK) {

diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
@@ -2669,17 +2669,12 @@ sysctl_kern_proc_kstack(SYSCTL_HANDLER_ARGS)
 		    sizeof(kkstp->kkst_trace), SBUF_FIXEDLEN);
 		thread_lock(td);
 		kkstp->kkst_tid = td->td_tid;
-		if (TD_IS_SWAPPED(td)) {
+		if (TD_IS_SWAPPED(td))
 			kkstp->kkst_state = KKST_STATE_SWAPPED;
-		} else if (TD_IS_RUNNING(td)) {
-			if (stack_save_td_running(st, td) == 0)
-				kkstp->kkst_state = KKST_STATE_STACKOK;
-			else
-				kkstp->kkst_state = KKST_STATE_RUNNING;
-		} else {
+		else if (stack_save_td(st, td) == 0)
 			kkstp->kkst_state = KKST_STATE_STACKOK;
-			stack_save_td(st, td);
-		}
+		else
+			kkstp->kkst_state = KKST_STATE_RUNNING;
 		thread_unlock(td);
 		PROC_UNLOCK(p);
 		stack_sbuf_print(&sb, st);

diff --git a/sys/kern/subr_kdb.c b/sys/kern/subr_kdb.c
@@ -432,9 +432,8 @@ kdb_backtrace_thread(struct thread *td)
 		struct stack st;
 
 		printf("KDB: stack backtrace of thread %d:\n", td->td_tid);
-		stack_zero(&st);
-		stack_save_td(&st, td);
-		stack_print_ddb(&st);
+		if (stack_save_td(&st, td) == 0)
+			stack_print_ddb(&st);
 	}
 #endif
 }

diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c
@@ -1245,7 +1245,7 @@ sleepq_sbuf_print_stacks(struct sbuf *sb, const void *wchan, int queue,
 				goto loop_end;
 
 			/* Note the td_lock is equal to the sleepq_lock here. */
-			stack_save_td(st[stack_idx], td);
+			(void)stack_save_td(st[stack_idx], td);
 
 			sbuf_printf(td_infos[stack_idx], "%d: %s %p",
 			    td->td_tid, td->td_name, td);

diff --git a/sys/kern/tty_info.c b/sys/kern/tty_info.c
@@ -340,12 +340,8 @@ tty_info(struct tty *tp)
 	if (tty_info_kstacks) {
 		if (TD_IS_SWAPPED(td))
 			sterr = ENOENT;
-		else if (TD_IS_RUNNING(td))
-			sterr = stack_save_td_running(&stack, td);
-		else {
-			stack_save_td(&stack, td);
-			sterr = 0;
-		}
+		else
+			sterr = stack_save_td(&stack, td);
 	}
 #endif
 	thread_unlock(td);

diff --git a/sys/mips/mips/stack_machdep.c b/sys/mips/mips/stack_machdep.c
@@ -29,9 +29,10 @@
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD$");
 
-#include <sys/types.h>
-#include <sys/systm.h>
 #include <sys/param.h>
+#include <sys/systm.h>
+#include <sys/lock.h>
+#include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/stack.h>
 
@@ -127,26 +128,22 @@ stack_capture(struct stack *st, u_register_t pc, u_register_t sp)
 	return;
 }
 
-void
+int
 stack_save_td(struct stack *st, struct thread *td)
 {
 	u_register_t pc, sp;
 
-	if (TD_IS_SWAPPED(td))
-		panic("stack_save_td: swapped");
+	THREAD_LOCK_ASSERT(td, MA_OWNED);
+	KASSERT(!TD_IS_SWAPPED(td),
+	    ("stack_save_td: thread %p is swapped", td));
+
 	if (TD_IS_RUNNING(td))
-		panic("stack_save_td: running");
+		return (EOPNOTSUPP);
 
 	pc = td->td_pcb->pcb_regs.pc;
 	sp = td->td_pcb->pcb_regs.sp;
 	stack_capture(st, pc, sp);
-}
-
-int
-stack_save_td_running(struct stack *st, struct thread *td)
-{
-
-	return (EOPNOTSUPP);
+	return (0);
 }
 
 void

diff --git a/sys/powerpc/powerpc/stack_machdep.c b/sys/powerpc/powerpc/stack_machdep.c
@@ -30,6 +30,8 @@
 __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
+#include <sys/lock.h>
+#include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/stack.h>
 #include <sys/systm.h>
@@ -86,25 +88,21 @@ stack_capture(struct stack *st, vm_offset_t frame)
 	}
 }
 
-void
+int
 stack_save_td(struct stack *st, struct thread *td)
 {
 	vm_offset_t frame;
 
-	if (TD_IS_SWAPPED(td))
-		panic("stack_save_td: swapped");
+	THREAD_LOCK_ASSERT(td, MA_OWNED);
+	KASSERT(!TD_IS_SWAPPED(td),
+	    ("stack_save_td: thread %p is swapped", td));
+
 	if (TD_IS_RUNNING(td))
-		panic("stack_save_td: running");
+		return (EOPNOTSUPP);
 
 	frame = td->td_pcb->pcb_sp;
 	stack_capture(st, frame);
-}
-
-int
-stack_save_td_running(struct stack *st, struct thread *td)
-{
-
-	return (EOPNOTSUPP);
+	return (0);
 }
 
 void

diff --git a/sys/riscv/riscv/stack_machdep.c b/sys/riscv/riscv/stack_machdep.c
@@ -37,6 +37,8 @@ __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
 #include <sys/systm.h>
+#include <sys/lock.h>
+#include <sys/mutex.h>
 #include <sys/proc.h>
 #include <sys/stack.h>
 
@@ -60,28 +62,24 @@ stack_capture(struct stack *st, struct unwind_state *frame)
 	}
 }
 
-void
+int
 stack_save_td(struct stack *st, struct thread *td)
 {
 	struct unwind_state frame;
 
-	if (TD_IS_SWAPPED(td))
-		panic("stack_save_td: swapped");
+	THREAD_LOCK_ASSERT(td, MA_OWNED);
+	KASSERT(!TD_IS_SWAPPED(td),
+	    ("stack_save_td: thread %p is swapped", td));
+
 	if (TD_IS_RUNNING(td))
-		panic("stack_save_td: running");
+		return (EOPNOTSUPP);
 
 	frame.sp = td->td_pcb->pcb_sp;
 	frame.fp = td->td_pcb->pcb_s[0];
 	frame.pc = td->td_pcb->pcb_ra;
 
 	stack_capture(st, &frame);
-}
-
-int
-stack_save_td_running(struct stack *st, struct thread *td)
-{
-
-	return (EOPNOTSUPP);
+	return (0);
 }
 
 void