Skip to content

Commit

Permalink
Changes that improve DTrace FBT reliability on freebsd/arm64:
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
rwatson committed Jan 11, 2021
1 parent a765078 commit 30b68ec
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 3 deletions.
3 changes: 2 additions & 1 deletion sys/cddl/dev/dtrace/aarch64/dtrace_subr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -163,7 +164,7 @@ dtrace_gethrtime()
{
struct timespec curtime;

nanouptime(&curtime);
dtrace_getnanouptime(&curtime);

return (curtime.tv_sec * 1000000000UL + curtime.tv_nsec);

Expand Down
3 changes: 2 additions & 1 deletion sys/cddl/dev/dtrace/arm/dtrace_subr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -174,7 +175,7 @@ dtrace_gethrtime()
{
struct timespec curtime;

nanouptime(&curtime);
dtrace_getnanouptime(&curtime);

return (curtime.tv_sec * 1000000000UL + curtime.tv_nsec);

Expand Down
3 changes: 2 additions & 1 deletion sys/cddl/dev/dtrace/riscv/dtrace_subr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -165,7 +166,7 @@ dtrace_gethrtime()
{
struct timespec curtime;

nanouptime(&curtime);
dtrace_getnanouptime(&curtime);

return (curtime.tv_sec * 1000000000UL + curtime.tv_nsec);

Expand Down
8 changes: 8 additions & 0 deletions sys/cddl/dev/fbt/aarch64/fbt_isa.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
11 changes: 11 additions & 0 deletions sys/cddl/dev/fbt/fbt.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
15 changes: 15 additions & 0 deletions sys/kern/kern_tc.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 30b68ec

Please sign in to comment.