From ec3db25c3d70d075be26d62cb6df403ea21e164a Mon Sep 17 00:00:00 2001 From: "Robert N. M. Watson" Date: Wed, 23 Dec 2020 13:00:20 +0000 Subject: [PATCH 1/4] Don't call instrumentable nanouptime() from within DTrace probes. --- sys/cddl/dev/dtrace/aarch64/dtrace_subr.c | 3 ++- sys/cddl/dev/dtrace/arm/dtrace_subr.c | 3 ++- sys/cddl/dev/dtrace/riscv/dtrace_subr.c | 3 ++- sys/kern/kern_tc.c | 15 +++++++++++++++ 4 files changed, 21 insertions(+), 3 deletions(-) diff --git a/sys/cddl/dev/dtrace/aarch64/dtrace_subr.c b/sys/cddl/dev/dtrace/aarch64/dtrace_subr.c index 000de00944927c..2d59b29b850b1a 100644 --- a/sys/cddl/dev/dtrace/aarch64/dtrace_subr.c +++ b/sys/cddl/dev/dtrace/aarch64/dtrace_subr.c @@ -49,6 +49,7 @@ __FBSDID("$FreeBSD$"); extern dtrace_id_t dtrace_probeid_error; extern int (*dtrace_invop_jump_addr)(struct trapframe *); extern void dtrace_getnanotime(struct timespec *tsp); +extern void dtrace_getnanouptime(struct timespec *tsp); int dtrace_invop(uintptr_t, struct trapframe *, uintptr_t); void dtrace_invop_init(void); @@ -163,7 +164,7 @@ dtrace_gethrtime() { struct timespec curtime; - nanouptime(&curtime); + dtrace_getnanouptime(&curtime); return (curtime.tv_sec * 1000000000UL + curtime.tv_nsec); diff --git a/sys/cddl/dev/dtrace/arm/dtrace_subr.c b/sys/cddl/dev/dtrace/arm/dtrace_subr.c index 8cd9c9c1f2046b..e98a9ded5442db 100644 --- a/sys/cddl/dev/dtrace/arm/dtrace_subr.c +++ b/sys/cddl/dev/dtrace/arm/dtrace_subr.c @@ -54,6 +54,7 @@ __FBSDID("$FreeBSD$"); extern dtrace_id_t dtrace_probeid_error; extern int (*dtrace_invop_jump_addr)(struct trapframe *); extern void dtrace_getnanotime(struct timespec *tsp); +extern void dtrace_getnanouptime(struct timespec *tsp); int dtrace_invop(uintptr_t, struct trapframe *, uintptr_t); void dtrace_invop_init(void); @@ -174,7 +175,7 @@ dtrace_gethrtime() { struct timespec curtime; - nanouptime(&curtime); + dtrace_getnanouptime(&curtime); return (curtime.tv_sec * 1000000000UL + curtime.tv_nsec); diff --git a/sys/cddl/dev/dtrace/riscv/dtrace_subr.c b/sys/cddl/dev/dtrace/riscv/dtrace_subr.c index 463465e119f91d..c1ac339b3a2623 100644 --- a/sys/cddl/dev/dtrace/riscv/dtrace_subr.c +++ b/sys/cddl/dev/dtrace/riscv/dtrace_subr.c @@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$"); extern dtrace_id_t dtrace_probeid_error; extern int (*dtrace_invop_jump_addr)(struct trapframe *); extern void dtrace_getnanotime(struct timespec *tsp); +extern void dtrace_getnanouptime(struct timespec *tsp); int dtrace_invop(uintptr_t, struct trapframe *); void dtrace_invop_init(void); @@ -165,7 +166,7 @@ dtrace_gethrtime() { struct timespec curtime; - nanouptime(&curtime); + dtrace_getnanouptime(&curtime); return (curtime.tv_sec * 1000000000UL + curtime.tv_nsec); diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c index 467e5cf8ee3aab..cc3219a885ab58 100644 --- a/sys/kern/kern_tc.c +++ b/sys/kern/kern_tc.c @@ -146,6 +146,7 @@ static void tc_windup(struct bintime *new_boottimebin); static void cpu_tick_calibrate(int); void dtrace_getnanotime(struct timespec *tsp); +void dtrace_getnanouptime(struct timespec *tsp); static int sysctl_kern_boottime(SYSCTL_HANDLER_ARGS) @@ -997,6 +998,20 @@ dtrace_getnanotime(struct timespec *tsp) GETTHMEMBER(tsp, th_nanotime); } +/* + * This is a clone of getnanouptime used for time since boot. + * The dtrace_ prefix prevents fbt from creating probes for + * it so an uptime that can be safely used in all fbt probes. + */ +void +dtrace_getnanouptime(struct timespec *tsp) +{ + struct bintime bt; + + GETTHMEMBER(&bt, th_offset); + bintime2timespec(&bt, tsp); +} + /* * System clock currently providing time to the system. Modifiable via sysctl * when the FFCLOCK option is defined. From 4de37d6790d09389cb5611c838a54a45441dc7e4 Mon Sep 17 00:00:00 2001 From: "Robert N. M. Watson" Date: Wed, 23 Dec 2020 14:47:27 +0000 Subject: [PATCH 2/4] On FreeBSD/amd64, don't allow instrumentation of handle_el1h_sync. --- sys/cddl/dev/fbt/fbt.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c index cf5971d2d9bdab..746178bf242e6b 100644 --- a/sys/cddl/dev/fbt/fbt.c +++ b/sys/cddl/dev/fbt/fbt.c @@ -145,6 +145,15 @@ fbt_excluded(const char *name) return (1); #endif + /* + * Some assembly-language exception handlers are not suitable for + * instrumentation. + */ +#if defined(__aarch64__) + if (strcmp(name, "handle_el1h_sync") == 0) + return (1); +#endif + /* * When DTrace is built into the kernel we need to exclude * the FBT functions from instrumentation. From e14fc5f60a8a8afafadb9a05454da99e1e116050 Mon Sep 17 00:00:00 2001 From: "Robert N. M. Watson" Date: Wed, 23 Dec 2020 15:55:11 +0000 Subject: [PATCH 3/4] Don't allow FBT to instrument DDB, which makes debugging FBT quite hard. Don't allow FBT to instrument memcpy(). This is a bit lazy, but avoids several panic variations I've encountered. I need to figure out if it is because memcpy() is being substituted by the compiler, or whether there is an undesirable explicit call to memcpy() somewhere in DTrace or its dependencies. --- sys/cddl/dev/fbt/fbt.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c index 746178bf242e6b..9db7adb645bce1 100644 --- a/sys/cddl/dev/fbt/fbt.c +++ b/sys/cddl/dev/fbt/fbt.c @@ -127,6 +127,13 @@ fbt_excluded(const char *name) return (1); } + /* + * Omit instrumentation of functions that are probably in DDB. It + * makes it too hard to debug broken FBT. + */ + if (strncmp(name, "db_", 3) == 0) + return (1); + /* * Lock owner methods may be called from probe context. */ @@ -136,6 +143,15 @@ fbt_excluded(const char *name) strcmp(name, "owner_sx") == 0) return (1); + /* + * The compiler will sometimes replace memcpy()-like code with calls + * to memcpy() (or similar), which can occur in the DTrace code + * itself. For now, rather than prevent that in the build, just don't + * instrument at run time. + */ + if (strcmp(name, "memcpy") == 0) + return (1); + /* * Stack unwinders may be called from probe context on some * platforms. From e73f2bbd906bd2b269dc2d964ff8e6b21f33ae5f Mon Sep 17 00:00:00 2001 From: "Robert N. M. Watson" Date: Wed, 23 Dec 2020 20:01:59 +0000 Subject: [PATCH 4/4] Blacklist one more exception-handling related function from being instrumented by FBT. With this change, we can now instrument all remaining function entries in the kernel using FBT and not immediately crash in boring operation. --- sys/cddl/dev/fbt/fbt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c index 9db7adb645bce1..f28d6a7a3e1fe2 100644 --- a/sys/cddl/dev/fbt/fbt.c +++ b/sys/cddl/dev/fbt/fbt.c @@ -166,7 +166,8 @@ fbt_excluded(const char *name) * instrumentation. */ #if defined(__aarch64__) - if (strcmp(name, "handle_el1h_sync") == 0) + if (strcmp(name, "handle_el1h_sync") == 0 || + strcmp(name, "do_el1h_sync") == 0) return (1); #endif