From owner-dev-commits-src-main@freebsd.org Mon Jan 11 15:44:42 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id CF88C4DD6CC; Mon, 11 Jan 2021 15:44:42 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DDygt2qzKz3vy0; Mon, 11 Jan 2021 15:44:41 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id D52941CD4B; Mon, 11 Jan 2021 15:44:41 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 10BFifMP039550; Mon, 11 Jan 2021 15:44:41 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 10BFifrH039549; Mon, 11 Jan 2021 15:44:41 GMT (envelope-from git) Date: Mon, 11 Jan 2021 15:44:41 GMT Message-Id: <202101111544.10BFifrH039549@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Robert Watson Subject: git: 30b68ecda84e - main - Changes that improve DTrace FBT reliability on freebsd/arm64: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: rwatson X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 30b68ecda84e38648511f5585a6f36b94b167f0d Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jan 2021 15:44:42 -0000 The branch main has been updated by rwatson: URL: https://cgit.FreeBSD.org/src/commit/?id=30b68ecda84e38648511f5585a6f36b94b167f0d commit 30b68ecda84e38648511f5585a6f36b94b167f0d Author: Robert Watson AuthorDate: 2021-01-09 08:38:11 +0000 Commit: Robert Watson CommitDate: 2021-01-11 15:42:22 +0000 Changes that improve DTrace FBT reliability on freebsd/arm64: - Implement a dtrace_getnanouptime(), matching the existing dtrace_getnanotime(), to avoid DTrace calling out to a potentially instrumentable function. (These should probably both be under KDTRACE_HOOKS. Also, it's not clear to me that they are correct implementations for the DTrace thread time functions they are used in .. fixes for another commit.) - Don't allow FBT to instrument functions involved in EL1 exception handling that are involved in FBT trap processing: handle_el1h_sync() and do_el1h_sync(). - Don't allow FBT to instrument DDB and KDB functions, as that makes it rather harder to debug FBT problems. Prior to these changes, use of FBT on FreeBSD/arm64 rapidly led to kernel panics due to recursion in DTrace. Reliable FBT on FreeBSD/arm64 is reliant on another change from @andrew to have the aarch64 instrumentor more carefully check that instructions it replaces are against the stack pointer, which can otherwise lead to memory corruption. That change remains under review. MFC after: 2 weeks Reviewed by: andrew, kp, markj (earlier version), jrtc27 (earlier version) Differential revision: https://reviews.freebsd.org/D27766 --- 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/cddl/dev/fbt/aarch64/fbt_isa.c | 8 ++++++++ sys/cddl/dev/fbt/fbt.c | 11 +++++++++++ sys/kern/kern_tc.c | 15 +++++++++++++++ 6 files changed, 40 insertions(+), 3 deletions(-) diff --git a/sys/cddl/dev/dtrace/aarch64/dtrace_subr.c b/sys/cddl/dev/dtrace/aarch64/dtrace_subr.c index 000de0094492..2d59b29b850b 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 8cd9c9c1f204..e98a9ded5442 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 463465e119f9..c1ac339b3a26 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/cddl/dev/fbt/aarch64/fbt_isa.c b/sys/cddl/dev/fbt/aarch64/fbt_isa.c index 581f390baf89..3c5070785614 100644 --- a/sys/cddl/dev/fbt/aarch64/fbt_isa.c +++ b/sys/cddl/dev/fbt/aarch64/fbt_isa.c @@ -96,6 +96,14 @@ fbt_provide_module_function(linker_file_t lf, int symindx, if (fbt_excluded(name)) return (0); + /* + * Instrumenting certain exception handling functions can lead to FBT + * recursion, so exclude from instrumentation. + */ + if (strcmp(name, "handle_el1h_sync") == 0 || + strcmp(name, "do_el1h_sync") == 0) + return (1); + instr = (uint32_t *)(symval->value); limit = (uint32_t *)(symval->value + symval->size); diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c index cf5971d2d9bd..a378ae0eb317 100644 --- a/sys/cddl/dev/fbt/fbt.c +++ b/sys/cddl/dev/fbt/fbt.c @@ -127,6 +127,17 @@ 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. + * + * NB: kdb_enter() can be excluded, but its call to printf() can't be. + * This is generally OK since we're not yet in debugging context. + */ + if (strncmp(name, "db_", 3) == 0 || + strncmp(name, "kdb_", 4) == 0) + return (1); + /* * Lock owner methods may be called from probe context. */ diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c index 467e5cf8ee3a..cc3219a885ab 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.