svn commit: r268600 - in head/sys: amd64/amd64 cddl/dev/dtrace/amd64 cddl/dev/dtrace/i386 cddl/dev/dtrace/mips cddl/dev/dtrace/powerpc i386/i386 mips/mips powerpc/aim sys

[Mark Johnston]

On Mon, Jul 14, 2014 at 11:10:50AM +0300, Konstantin Belousov wrote:
> On Mon, Jul 14, 2014 at 04:38:17AM +0000, Mark Johnston wrote:
> > Author: markj
> > Date: Mon Jul 14 04:38:17 2014
> > New Revision: 268600
> > URL: http://svnweb.freebsd.org/changeset/base/268600
> > 
> > Log:
> >   Invoke the DTrace trap handler before calling trap() on amd64. This matches
> >   the upstream implementation and helps ensure that a trap induced by tracing
> >   fbt::trap:entry is handled without recursively generating another trap.
> >   
> >   This makes it possible to run most (but not all) of the DTrace tests under
> >   common/safety/ without triggering a kernel panic.
> >   
> >   Submitted by:	Anton Rang <anton.rang at isilon.com> (original version)
> >   Phabric:	D95
> > 
> > Modified:
> >   head/sys/amd64/amd64/exception.S
> >   head/sys/amd64/amd64/trap.c
> >   head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c
> >   head/sys/cddl/dev/dtrace/i386/dtrace_subr.c
> >   head/sys/cddl/dev/dtrace/mips/dtrace_subr.c
> >   head/sys/cddl/dev/dtrace/powerpc/dtrace_subr.c
> >   head/sys/i386/i386/trap.c
> >   head/sys/mips/mips/trap.c
> >   head/sys/powerpc/aim/trap.c
> >   head/sys/sys/dtrace_bsd.h
> > 
> > Modified: head/sys/amd64/amd64/exception.S
> > ==============================================================================
> > --- head/sys/amd64/amd64/exception.S	Mon Jul 14 00:16:49 2014	(r268599)
> > +++ head/sys/amd64/amd64/exception.S	Mon Jul 14 04:38:17 2014	(r268600)
> > @@ -228,7 +228,24 @@ alltraps_pushregs_no_rdi:
> >  	.type	calltrap, at function
> >  calltrap:
> >  	movq	%rsp,%rdi
> > +#ifdef KDTRACE_HOOKS
> > +	/*
> > +	 * Give DTrace a chance to vet this trap and skip the call to trap() if
> > +	 * it turns out that it was caused by a DTrace probe.
> > +	 */
> > +	movq	dtrace_trap_func,%rax
> > +	testq	%rax,%rax
> > +	je	skiphook
> > +	call	*%rax
> > +	testq	%rax,%rax
> > +	jne	skiptrap
> > +	movq	%rsp,%rdi
> > +skiphook:
> > +#endif
> >  	call	trap
> Why is this fragment done in asm ?  I see it relatively easy to
> either move to the start of trap(), or create a new wrapper around
> current trap() which calls dtrace_trap_func conditionally, and then
> resorts to the old trap.

You're right, it looks like this could be done in C by introducing a
wrapper. I'm not sure how moving the conditional call to
dtrace_trap_func() around within trap() would fix the original problem,
though.

The diff below adds such a wrapper, and modifies DTrace to explicitly
ignore it when creating function boundary probes. Additionally, trap()
is marked __noinline to ensure that it can still be instrumented. I've
named it "_trap" for now for lack of a better name, but that will need
to change.

Thanks,
-Mark

diff --git a/sys/amd64/amd64/exception.S b/sys/amd64/amd64/exception.S
index bb5fd56..3d34724 100644
--- a/sys/amd64/amd64/exception.S
+++ b/sys/amd64/amd64/exception.S
@@ -228,24 +228,7 @@ alltraps_pushregs_no_rdi:
 	.type	calltrap, at function
 calltrap:
 	movq	%rsp,%rdi
-#ifdef KDTRACE_HOOKS
-	/*
-	 * Give DTrace a chance to vet this trap and skip the call to trap() if
-	 * it turns out that it was caused by a DTrace probe.
-	 */
-	movq	dtrace_trap_func,%rax
-	testq	%rax,%rax
-	je	skiphook
-	call	*%rax
-	testq	%rax,%rax
-	jne	skiptrap
-	movq	%rsp,%rdi
-skiphook:
-#endif
-	call	trap
-#ifdef KDTRACE_HOOKS
-skiptrap:
-#endif
+	call	_trap
 	MEXITCOUNT
 	jmp	doreti			/* Handle any pending ASTs */
 
diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index d9203bc..545174a 100644
--- a/sys/amd64/amd64/trap.c
+++ b/sys/amd64/amd64/trap.c
@@ -97,7 +97,8 @@ PMC_SOFT_DEFINE( , , page_fault, write);
 #include <sys/dtrace_bsd.h>
 #endif
 
-extern void trap(struct trapframe *frame);
+extern void _trap(struct trapframe *frame);
+extern void __noinline trap(struct trapframe *frame);
 extern void syscall(struct trapframe *frame);
 void dblfault_handler(struct trapframe *frame);
 
@@ -604,6 +605,19 @@ out:
 	return;
 }
 
+/*
+ * Ensure that we ignore any DTrace-induced faults. This function cannot
+ * be instrumented, so it cannot generate such faults itself.
+ */
+void
+_trap(struct trapframe *frame)
+{
+
+	if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame))
+		return;
+	trap(frame);
+}
+
 static int
 trap_pfault(frame, usermode)
 	struct trapframe *frame;
diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c
index 7e256e7..0cc2db2 100644
--- a/sys/cddl/dev/fbt/fbt.c
+++ b/sys/cddl/dev/fbt/fbt.c
@@ -232,13 +232,18 @@ fbt_provide_module_function(linker_file_t lf, int symindx,
 	int size;
 	u_int8_t *instr, *limit;
 
-	if (strncmp(name, "dtrace_", 7) == 0 &&
-	    strncmp(name, "dtrace_safe_", 12) != 0) {
+	if ((strncmp(name, "dtrace_", 7) == 0 &&
+	    strncmp(name, "dtrace_safe_", 12) != 0) ||
+	    strcmp(name, "_trap") == 0) {
 		/*
 		 * Anything beginning with "dtrace_" may be called
 		 * from probe context unless it explicitly indicates
 		 * that it won't be called from probe context by
 		 * using the prefix "dtrace_safe_".
+		 *
+		 * Additionally, we avoid instrumenting _trap() to avoid the
+		 * possibility of generating a fault in probe context before
+		 * DTrace's fault handler is called.
 		 */
 		return (0);
 	}