Handle bug #6500 fallout.

2008-05-28  David Smith  <dsmith@redhat.com>

        * tapsets.cxx (utrace_derived_probe_group::emit_probe_decl):
        Instead of adding clone handlers, just call the probes directly.
        (utrace_derived_probe_group::emit_module_decls): For syscall
        probes, on exec detach the parent's utrace engine from the child.

2008-05-28  David Smith  <dsmith@redhat.com>

        * task_finder.c (__stp_utrace_attach_match_filename): Added
        register_p parameter, which is passed on to the callback.  Only
        adds death notification if register_p is 1.  If register_p is 0,
        removes death notification.
        (__stp_utrace_attach_match_tsk): Moved code from
        __stp_utrace_task_finder_report_clone that handles the details of
        grabbing a task's path.
        (__stp_utrace_task_finder_report_clone): Calls new
        __stp_utrace_attach_match_tsk().
        (__stp_utrace_task_finder_report_exec): Notifies upper layer that
        it might need to detach from newly exec'ed process.`

4 files changed, 152 insertions, 66 deletions

diff --git a/ChangeLog b/ChangeLog
index 806fcfa3..a6fdffa2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2008-05-28  David Smith  <dsmith@redhat.com>
+
+	* tapsets.cxx (utrace_derived_probe_group::emit_probe_decl):
+	Instead of adding clone handlers, just call the probes directly.
+	(utrace_derived_probe_group::emit_module_decls): For syscall
+	probes, on exec detach the parent's utrace engine from the child.
+
 2008-05-27  Josh Stone  <joshua.i.stone@intel.com>
 
 	PR 6432
@@ -25,7 +32,7 @@
 	* tapset/ppc64/registers.stp: Support powerpc register + arg lookup
 	* stapfuncs.5.in: Add powerpc bits; indicate scope of uarg_* access
 
-2008-05-21  David Smith  <dsmith@redhat.com>:ChangeLog
+2008-05-21  David Smith  <dsmith@redhat.com>
 
 	* tapsets.cxx (utrace_derived_probe_group::emit_module_decls):
 	Added new 'event_flag' parameter to task_finder callback.  Only
@@ -200,8 +207,6 @@
 
 	* systemtap.spec.in: Simplify configuration defaults.
 
-2008-04-29  David Smith  <dsmith@redhat.com>:ChangeLog
-
 2008-04-25  David Smith  <dsmith@redhat.com>
 
 	PR 6455.
diff --git a/runtime/ChangeLog b/runtime/ChangeLog
index 0b7559b9..cc8bf1b0 100644
--- a/runtime/ChangeLog
+++ b/runtime/ChangeLog
@@ -1,3 +1,17 @@
+2008-05-28  David Smith  <dsmith@redhat.com>
+
+	* task_finder.c (__stp_utrace_attach_match_filename): Added
+	register_p parameter, which is passed on to the callback.  Only
+	adds death notification if register_p is 1.  If register_p is 0,
+	removes death notification.
+	(__stp_utrace_attach_match_tsk): Moved code from
+	__stp_utrace_task_finder_report_clone that handles the details of
+	grabbing a task's path.
+	(__stp_utrace_task_finder_report_clone): Calls new
+	__stp_utrace_attach_match_tsk().
+	(__stp_utrace_task_finder_report_exec): Notifies upper layer that
+	it might need to detach from newly exec'ed process.`
+
 2008-05-27  Josh Stone  <joshua.i.stone@intel.com>
 
 	PR 6432
diff --git a/runtime/task_finder.c b/runtime/task_finder.c
index a8de13d0..9b03234d 100644
--- a/runtime/task_finder.c
+++ b/runtime/task_finder.c
@@ -239,6 +239,7 @@ __stp_utrace_attach(struct task_struct *tsk,
 static inline void
 __stp_utrace_attach_match_filename(struct task_struct *tsk,
 				   const char * const filename,
+				   int register_p,
 				   unsigned long event_flag)
 {
 	size_t filelen;
@@ -272,8 +273,8 @@ __stp_utrace_attach_match_filename(struct task_struct *tsk,
 				continue;
 
 			if (cb_tgt->callback != NULL) {
-				int rc = cb_tgt->callback(tsk, 1, event_flag,
-							  cb_tgt);
+				int rc = cb_tgt->callback(tsk, register_p,
+							  event_flag, cb_tgt);
 				if (rc != 0) {
 					_stp_error("callback for %d failed: %d",
 						   (int)tsk->pid, rc);
@@ -282,64 +283,102 @@ __stp_utrace_attach_match_filename(struct task_struct *tsk,
 			}
 
 			// Set up thread death notification.
-			rc = __stp_utrace_attach(tsk, &cb_tgt->ops, cb_tgt,
-						 __STP_UTRACE_ATTACHED_TASK_EVENTS);
-			if (rc != 0 && rc != EPERM)
-				break;
-			cb_tgt->engine_attached = 1;
+			if (register_p) {
+				rc = __stp_utrace_attach(tsk, &cb_tgt->ops,
+							 cb_tgt,
+							 __STP_UTRACE_ATTACHED_TASK_EVENTS);
+				if (rc != 0 && rc != EPERM)
+					break;
+				cb_tgt->engine_attached = 1;
+			}
+			else {
+				struct utrace_attached_engine *engine;
+				engine = utrace_attach(tsk,
+						       UTRACE_ATTACH_MATCH_OPS,
+						       &cb_tgt->ops, 0);
+				if (! IS_ERR(engine) && engine != NULL) {
+					utrace_detach(tsk, engine);
+				}
+			}
 		}
 	}
 }
 
-static u32
-__stp_utrace_task_finder_report_clone(struct utrace_attached_engine *engine,
-				      struct task_struct *parent,
-				      unsigned long clone_flags,
-				      struct task_struct *child)
+// This function handles the details of getting a task's associated
+// pathname, and calling __stp_utrace_attach_match_filename() to
+// attach to it if we find the pathname "interesting".  So, what's the
+// difference between path_tsk and match_tsk?  Normally they are the
+// same, except in one case.  In an UTRACE_EVENT(EXEC), we need to
+// detach engines from the newly exec'ed process (since its path has
+// changed).  In this case, we have to match the path of the parent
+// (path_tsk) against the child (match_tsk).
+
+static void
+__stp_utrace_attach_match_tsk(struct task_struct *path_tsk,
+			      struct task_struct *match_tsk, int register_p,
+			      unsigned long event_flag)
 {
-	int rc;
 	struct mm_struct *mm;
 	char *mmpath_buf;
 	char *mmpath;
 
-	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING)
-		return UTRACE_ACTION_RESUME;
+	if (path_tsk->pid <= 1 || match_tsk->pid <= 1)
+		return;
 
-	// On clone, attach to the child.
-	rc = __stp_utrace_attach(child, engine->ops, 0,
-				 __STP_UTRACE_TASK_FINDER_EVENTS);
-	if (rc != 0 && rc != EPERM)
-		return UTRACE_ACTION_RESUME;
-
-	/* Grab the path associated with this task. */
-	mm = get_task_mm(child);
+	/* Grab the path associated with the path_tsk. */
+	mm = get_task_mm(path_tsk);
 	if (! mm) {
 		/* If the thread doesn't have a mm_struct, it is
 		 * a kernel thread which we need to skip. */
-		return UTRACE_ACTION_RESUME;
+		return;
 	}
 
 	// Allocate space for a path
 	mmpath_buf = _stp_kmalloc(PATH_MAX);
 	if (mmpath_buf == NULL) {
+		mmput(mm);
 		_stp_error("Unable to allocate space for path");
-		return UTRACE_ACTION_RESUME;
+		return;
 	}
 
 	// Grab the path associated with the new task
 	mmpath = __stp_get_mm_path(mm, mmpath_buf, PATH_MAX);
 	mmput(mm);			/* We're done with mm */
 	if (mmpath == NULL || IS_ERR(mmpath)) {
-		rc = -PTR_ERR(mmpath);
+		int rc = -PTR_ERR(mmpath);
 		_stp_error("Unable to get path (error %d) for pid %d",
-			   rc, (int)child->pid);
+			   rc, (int)path_tsk->pid);
 	}
 	else {
-		__stp_utrace_attach_match_filename(child, mmpath,
-						   UTRACE_EVENT(CLONE));
+		__stp_utrace_attach_match_filename(match_tsk, mmpath,
+						   register_p, event_flag);
 	}
 
 	_stp_kfree(mmpath_buf);
+	return;
+}
+
+static u32
+__stp_utrace_task_finder_report_clone(struct utrace_attached_engine *engine,
+				      struct task_struct *parent,
+				      unsigned long clone_flags,
+				      struct task_struct *child)
+{
+	int rc;
+	struct mm_struct *mm;
+	char *mmpath_buf;
+	char *mmpath;
+
+	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING)
+		return UTRACE_ACTION_RESUME;
+
+	// On clone, attach to the child.
+	rc = __stp_utrace_attach(child, engine->ops, 0,
+				 __STP_UTRACE_TASK_FINDER_EVENTS);
+	if (rc != 0 && rc != EPERM)
+		return UTRACE_ACTION_RESUME;
+
+	__stp_utrace_attach_match_tsk(parent, child, 1, UTRACE_EVENT(CLONE));
 	return UTRACE_ACTION_RESUME;
 }
 
@@ -357,11 +396,21 @@ __stp_utrace_task_finder_report_exec(struct utrace_attached_engine *engine,
 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING)
 		return UTRACE_ACTION_RESUME;
 
+	// When exec'ing, we need to let callers detach from the
+	// parent thread (if necessary).  For instance, assume
+	// '/bin/bash' clones and then execs '/bin/ls'.  If the user
+	// was probing '/bin/bash', the cloned thread is still
+	// '/bin/bash' up until the exec.
+	if (tsk != NULL && tsk->parent != NULL && tsk->parent->pid > 1) {
+		__stp_utrace_attach_match_tsk(tsk->parent, tsk, 0,
+					      UTRACE_EVENT(EXEC));
+	}
+
 	// On exec, check bprm
 	if (bprm->filename == NULL)
 		return UTRACE_ACTION_RESUME;
 
-	__stp_utrace_attach_match_filename(tsk, bprm->filename,
+	__stp_utrace_attach_match_filename(tsk, bprm->filename, 1,
 					   UTRACE_EVENT(EXEC));
 
 	return UTRACE_ACTION_RESUME;
diff --git a/tapsets.cxx b/tapsets.cxx
index c1bc88b0..755f6c07 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -5381,8 +5381,10 @@ utrace_derived_probe_group::emit_probe_decl (systemtap_session& s,
   switch (p->flags)
     {
     case UDPF_CLONE:
-      s.op->line() << " .ops={ .report_clone=stap_utrace_probe_clone, .report_death=stap_utrace_task_finder_report_death },";
-      s.op->line() << " .flags=(UTRACE_EVENT(CLONE)|UTRACE_EVENT(DEATH)),";
+      // Notice we're not setting up a .ops/.report_clone handler here.
+      // Instead, we'll just call the probe directly when we get
+      // notified the clone happened.
+      s.op->line() << " .flags=(UTRACE_EVENT(CLONE)),";
       break;
     case UDPF_EXEC:
       // Notice we're not setting up a .ops/.report_exec handler here.
@@ -5431,26 +5433,9 @@ utrace_derived_probe_group::emit_module_decls (systemtap_session& s)
   s.op->newline() << "int engine_attached;";
   s.op->newline(-1) << "};";
 
-  // Output handler function for CLONE events
-  if (flags_seen[UDPF_CLONE])
-    {
-      s.op->newline() << "static u32 stap_utrace_probe_clone(struct utrace_attached_engine *engine, struct task_struct *parent, unsigned long clone_flags, struct task_struct *child) {";
-      s.op->indent(1);
-      s.op->newline() << "struct stap_utrace_probe *p = (struct stap_utrace_probe *)engine->data;";
-
-      common_probe_entryfn_prologue (s.op, "STAP_SESSION_RUNNING");
-      s.op->newline() << "c->probe_point = p->pp;";
-
-      // call probe function
-      s.op->newline() << "(*p->ph) (c);";
-      common_probe_entryfn_epilogue (s.op);
-
-      s.op->newline() << "return UTRACE_ACTION_RESUME;";
-      s.op->newline(-1) << "}";
-    }
-
-  // Output handler function for EXEC and DEATH events
-  if (flags_seen[UDPF_EXEC] || flags_seen[UDPF_DEATH])
+  // Output handler function for CLONE, EXEC, and DEATH events
+  if (flags_seen[UDPF_CLONE] || flags_seen[UDPF_EXEC]
+      || flags_seen[UDPF_DEATH])
     {
       s.op->newline() << "static void stap_utrace_probe_handler(struct task_struct *tsk, struct stap_utrace_probe *p) {";
       s.op->indent(1);
@@ -5498,12 +5483,23 @@ utrace_derived_probe_group::emit_module_decls (systemtap_session& s)
 
   s.op->newline() << "switch (p->flags) {";
   s.op->indent(1);
-  // When registering an exec probe, we can't install a utrace engine,
-  // since we're already in a exec event.  So, we just call the probe
-  // directly.  Note that for existing threads, this won't really work
-  // since our state isn't STAP_SESSION_RUNNING yet.  But that's OK,
-  // since this isn't really a 'exec' event - it is a notification
-  // that task_finder found an interesting process.
+  // When registering an clone/exec probe, we can't install a utrace
+  // engine, since we're already in the clone/exec event.  So, we just
+  // call the probe directly.  Note that for existing threads, this
+  // won't really work since our state isn't STAP_SESSION_RUNNING yet.
+  // But that's OK, since this isn't really a 'clone/exec' event - it
+  // is a notification that task_finder found an interesting process.
+  if (flags_seen[UDPF_CLONE])
+    {
+      s.op->newline() << "case UTRACE_EVENT(CLONE):";
+      s.op->indent(1);
+      s.op->newline() << "if (event_flag == UTRACE_EVENT(CLONE)) {";
+      s.op->indent(1);
+      s.op->newline() << "stap_utrace_probe_handler(tsk, p);";
+      s.op->newline(-1) << "}";
+      s.op->newline() << "break;";
+      s.op->indent(-1);
+    }
   if (flags_seen[UDPF_EXEC])
     {
       s.op->newline() << "case UTRACE_EVENT(EXEC):";
@@ -5524,11 +5520,9 @@ utrace_derived_probe_group::emit_module_decls (systemtap_session& s)
       s.op->newline() << "break;";
       s.op->indent(-1);
     }
-  // Attach an engine for CLONE, SYSCALL_ENTRY, and SYSCALL_EXIT events.
-  if (flags_seen[UDPF_CLONE] || flags_seen[UDPF_SYSCALL_ENTRY]
-      || flags_seen[UDPF_SYSCALL_EXIT])
+  // Attach an engine for SYSCALL_ENTRY and SYSCALL_EXIT events.
+  if (flags_seen[UDPF_SYSCALL_ENTRY] || flags_seen[UDPF_SYSCALL_EXIT])
     {
-      s.op->newline() << "case (UTRACE_EVENT(CLONE)|UTRACE_EVENT(DEATH)):";
       s.op->newline() << "case (UTRACE_EVENT(SYSCALL_ENTRY)|UTRACE_EVENT(DEATH)):";
       s.op->newline() << "case (UTRACE_EVENT(SYSCALL_EXIT)|UTRACE_EVENT(DEATH)):";
       s.op->indent(1);
@@ -5561,14 +5555,38 @@ utrace_derived_probe_group::emit_module_decls (systemtap_session& s)
   // cleanup here.  We'll cleanup at module unload time.
   s.op->newline() << "else {";
   s.op->indent(1);
+  s.op->newline() << "switch (p->flags) {";
+  s.op->indent(1);
   // For death probes, go ahead and call the probe directly.
   if (flags_seen[UDPF_DEATH])
     {
-      s.op->newline() << "if (p->flags == UTRACE_EVENT(DEATH) && event_flag == UTRACE_EVENT(DEATH)) {";
+      s.op->newline() << "case UTRACE_EVENT(DEATH):";
+      s.op->indent(1);
+      s.op->newline() << "if (event_flag == UTRACE_EVENT(DEATH)) {";
       s.op->indent(1);
       s.op->newline() << "stap_utrace_probe_handler(tsk, p);";
       s.op->newline(-1) << "}";
+      s.op->newline() << "break;";
+      s.op->indent(-1);
     }
+  if (flags_seen[UDPF_SYSCALL_ENTRY] || flags_seen[UDPF_SYSCALL_EXIT])
+    {
+      s.op->newline() << "case (UTRACE_EVENT(SYSCALL_ENTRY)|UTRACE_EVENT(DEATH)):";
+      s.op->newline() << "case (UTRACE_EVENT(SYSCALL_EXIT)|UTRACE_EVENT(DEATH)):";
+      s.op->indent(1);
+      s.op->newline() << "if (event_flag == UTRACE_EVENT(EXEC)) {";
+      s.op->indent(1);
+      s.op->newline() << "engine = utrace_attach(tsk, UTRACE_ATTACH_MATCH_OPS, &p->ops, 0);";
+      s.op->newline() << "if (! IS_ERR(engine) && engine != NULL) {";
+      s.op->indent(1);
+      s.op->newline() << "utrace_detach(tsk, engine);";
+      s.op->newline() << "_stp_dbug(__FUNCTION__, __LINE__, \"*** disconnected!\\n\");";
+      s.op->newline(-1) << "}";
+      s.op->newline(-1) << "}";
+      s.op->newline() << "break;";
+      s.op->indent(-1);
+    }
+  s.op->newline(-1) << "}";
   s.op->newline(-1) << "}";
   s.op->newline() << "return rc;";
   s.op->newline(-1) << "}";