Re: svn commit: r344452 - in head/sys/cddl: contrib/opensolaris/uts/common/dtrace contrib/opensolaris/uts/intel/dtrace dev/dtrace

2019-02-21 Thread Conrad Meyer
Thanks Mark!

Prior to this change, if you used userspace dtrace enough, eventually
you would get a spurious SIGTRAP on a process, which has the default
behavior of dumping core.  Perhaps understandably, people balk at
random core files lying around.  Or at dtrace killing their programs.
This patch should address that long-standing problem.

An easy repro scenario was described in the differential:

> It's possible to reproduce this by, for example, calling strlen()
> in a loop, probing every instruction in strlen(), and killing dtrace(1).


On Thu, Feb 21, 2019 at 2:54 PM Mark Johnston  wrote:
>
> Author: markj
> Date: Thu Feb 21 22:54:17 2019
> New Revision: 344452
> URL: https://svnweb.freebsd.org/changeset/base/344452
>
> Log:
>   Fix a tracepoint lookup race in fasttrap_pid_probe().
>
>   fasttrap hooks the userspace breakpoint handler; the hook looks up the
>   breakpoint address in a hash table of tracepoints.  It is possible for
>   the tracepoint to be removed by a different thread in between the
>   breakpoint trap and the hash table lookup, in which case SIGTRAP gets
>   delivered to the target process.  Fix the problem by adding a
>   per-process generation counter that gets incremented when a tracepoint
>   belonging to that process is removed.  Then, when a lookup fails, the
>   trapping instruction is restarted if the thread's counter doesn't match
>   that of the process.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


svn commit: r344452 - in head/sys/cddl: contrib/opensolaris/uts/common/dtrace contrib/opensolaris/uts/intel/dtrace dev/dtrace

2019-02-21 Thread Mark Johnston
Author: markj
Date: Thu Feb 21 22:54:17 2019
New Revision: 344452
URL: https://svnweb.freebsd.org/changeset/base/344452

Log:
  Fix a tracepoint lookup race in fasttrap_pid_probe().
  
  fasttrap hooks the userspace breakpoint handler; the hook looks up the
  breakpoint address in a hash table of tracepoints.  It is possible for
  the tracepoint to be removed by a different thread in between the
  breakpoint trap and the hash table lookup, in which case SIGTRAP gets
  delivered to the target process.  Fix the problem by adding a
  per-process generation counter that gets incremented when a tracepoint
  belonging to that process is removed.  Then, when a lookup fails, the
  trapping instruction is restarted if the thread's counter doesn't match
  that of the process.
  
  Reviewed by:  cem
  MFC after:2 weeks
  Sponsored by: The FreeBSD Foundation
  Differential Revision:https://reviews.freebsd.org/D19273

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
  head/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c
  head/sys/cddl/dev/dtrace/dtrace_cddl.h

Modified: head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
==
--- head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c  Thu Feb 
21 22:49:39 2019(r344451)
+++ head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c  Thu Feb 
21 22:54:17 2019(r344452)
@@ -1089,6 +1089,8 @@ fasttrap_tracepoint_disable(proc_t *p, fasttrap_probe_
ASSERT(p->p_proc_flag & P_PR_LOCK);
 #endif
p->p_dtrace_count--;
+
+   atomic_add_rel_64(>p_fasttrap_tp_gen, 1);
}
 
/*

Modified: head/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c
==
--- head/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c   Thu Feb 
21 22:49:39 2019(r344451)
+++ head/sys/cddl/contrib/opensolaris/uts/intel/dtrace/fasttrap_isa.c   Thu Feb 
21 22:54:17 2019(r344452)
@@ -967,6 +967,7 @@ fasttrap_pid_probe(struct trapframe *tf)
struct reg reg, *rp;
proc_t *p = curproc, *pp;
struct rm_priotracker tracker;
+   uint64_t gen;
uintptr_t pc;
uintptr_t new_pc = 0;
fasttrap_bucket_t *bucket;
@@ -1026,8 +1027,22 @@ fasttrap_pid_probe(struct trapframe *tf)
while (pp->p_vmspace == pp->p_pptr->p_vmspace)
pp = pp->p_pptr;
pid = pp->p_pid;
+   if (pp != p) {
+   PROC_LOCK(pp);
+   if ((pp->p_flag & P_WEXIT) != 0) {
+   /*
+* This can happen if the child was created with
+* rfork(2).  Userspace tracing cannot work reliably in
+* such a scenario, but we can at least try.
+*/
+   PROC_UNLOCK(pp);
+   sx_sunlock(_lock);
+   return (-1);
+   }
+   _PHOLD_LITE(pp);
+   PROC_UNLOCK(pp);
+   }
sx_sunlock(_lock);
-   pp = NULL;
 
rm_rlock(_tp_lock, );
 #endif
@@ -1051,11 +1066,28 @@ fasttrap_pid_probe(struct trapframe *tf)
if (tp == NULL) {
 #ifdef illumos
mutex_exit(pid_mtx);
+   return (-1);
 #else
rm_runlock(_tp_lock, );
-#endif
+   gen = atomic_load_acq_64(>p_fasttrap_tp_gen);
+   if (pp != p)
+   PRELE(pp);
+   if (curthread->t_fasttrap_tp_gen != gen) {
+   /*
+* At least one tracepoint associated with this PID has
+* been removed from the table since #BP was raised.
+* Speculate that we hit a tracepoint that has since
+* been removed, and retry the instruction.
+*/
+   curthread->t_fasttrap_tp_gen = gen;
+   tf->tf_rip = pc;
+   return (0);
+   }
return (-1);
+#endif
}
+   if (pp != p)
+   PRELE(pp);
 
/*
 * Set the program counter to the address of the traced instruction

Modified: head/sys/cddl/dev/dtrace/dtrace_cddl.h
==
--- head/sys/cddl/dev/dtrace/dtrace_cddl.h  Thu Feb 21 22:49:39 2019
(r344451)
+++ head/sys/cddl/dev/dtrace/dtrace_cddl.h  Thu Feb 21 22:54:17 2019
(r344452)
@@ -37,7 +37,7 @@ typedef struct kdtrace_proc {
u_int64_t   p_dtrace_count; /* Number of DTrace tracepoints 
*/
void*p_dtrace_helpers;  /* DTrace helpers, if any */
int p_dtrace_model;
-
+   uint64_tp_fasttrap_tp_gen;  /*