commit 8d1ae0139eed2a1c2d118e6d3ea7facb52aab40c

Author: David Smith <dsmith@redhat.com>

Date:   Mon Jan 15 10:23:45 2018 -0600

    Fix BZ1525651 by making the task_finder shutdown more robust.

    * runtime/linux/task_finder.c (__stap_utrace_detach): New function, based

      on stap_utrace_detach(), but lets the caller know if any the task had

      the utrace engine attached to it.

      (stap_utrace_detach): Just call __stap_utrace_detach().

      (stap_utrace_detach_ops): Perform the detaching in a loop, so if the

      process list snapshot we operate on becomes out of date, we'll still

      detach everything.

      (__stp_utrace_task_finder_report_clone): Move the

      __stp_tf_handler_start() call to the very start of the function.

      (__stp_utrace_task_finder_report_exec): Ditto.

      (__stp_utrace_task_finder_target_exec): Ditto.

      (__stp_utrace_task_finder_target_exit): Ditto.

      (__stp_utrace_task_finder_target_quiesce): Ditto.

      (__stp_utrace_task_finder_target_syscall_entry): Ditto.

      (__stp_utrace_task_finder_target_syscall_exit): Ditto.

diff --git a/runtime/linux/task_finder.c b/runtime/linux/task_finder.c

index 69c27fe..3346f2e 100644

--- a/runtime/linux/task_finder.c

+++ b/runtime/linux/task_finder.c

@@ -283,13 +283,17 @@ stap_register_task_finder_target(struct stap_task_finder_target *new_tgt)

 }

 static int

-stap_utrace_detach(struct task_struct *tsk,

-		   const struct utrace_engine_ops *ops)

+__stap_utrace_detach(struct task_struct *tsk,

+		     const struct utrace_engine_ops *ops,

+		     int *ops_matched)

 {

 	struct utrace_engine *engine;

 	struct mm_struct *mm;

 	int rc = 0;

+	if (ops_matched)

+		*ops_matched = 0;

+

 	// Ignore invalid tasks.

 	if (tsk == NULL || tsk->pid <= 0)

 		return 0;

@@ -331,6 +335,8 @@ stap_utrace_detach(struct task_struct *tsk,

 		rc = EFAULT;

 	}

 	else {

+		if (ops_matched)

+			*ops_matched = 1;

 		rc = utrace_control(tsk, engine, UTRACE_DETACH);

 		switch (rc) {

 		case 0:			/* success */

@@ -363,6 +369,13 @@ stap_utrace_detach(struct task_struct *tsk,

 	return rc;

 }

+static int

+stap_utrace_detach(struct task_struct *tsk,

+		   const struct utrace_engine_ops *ops)

+{

+	return __stap_utrace_detach(tsk, ops, NULL);

+}

+

 static void

 stap_utrace_detach_ops(struct utrace_engine_ops *ops)

 {

@@ -370,42 +383,66 @@ stap_utrace_detach_ops(struct utrace_engine_ops *ops)

 	struct utrace_engine *engine;

 	pid_t pid = 0;

 	int rc = 0;

-

-	// Notice we're not calling get_task_mm() in this loop. In

-	// every other instance when calling do_each_thread, we avoid

-	// tasks with no mm, because those are kernel threads.  So,

-	// why is this function different?  When a thread is in the

-	// process of dying, its mm gets freed.  Then, later the

-	// thread gets in the dying state and the thread's

-	// UTRACE_EVENT(DEATH) event handler gets called (if any).

+	int ops_matched;

+	int iterations = 0;

+

+	// We're going to do the detach in a loop. Why?

+	// do_each_thread() presents a snapshot of the list of

+	// processes on the system. If the system is currently

+	// creating lots of processes, we'll miss some during this

+	// loop (especially since utrace_barrier() can wait for a

+	// handler to finish).

 	//

-	// If a thread is in this "mortally wounded" state - no mm

-	// but not dead - and at that moment this function is called,

-	// we'd miss detaching from it if we were checking to see if

-	// it had an mm.

-

-	rcu_read_lock();

-	do_each_thread(grp, tsk) {

+	// So, we'll repeat the loop until we longer find any

+	// processes that have this ops attached to it.

+	do {

+		// Notice we're not calling get_task_mm() in this

+		// loop. In every other instance when calling

+		// do_each_thread, we avoid tasks with no mm, because

+		// those are kernel threads.  So, why is this function

+		// different?  When a thread is in the process of

+		// dying, its mm gets freed.  Then, later the thread

+		// gets in the dying state and the thread's

+		// UTRACE_EVENT(DEATH) event handler gets called (if

+		// any).

+		//

+		// If a thread is in this "mortally wounded" state -

+		// no mm but not dead - and at that moment this

+		// function is called, we'd miss detaching from it if

+		// we were checking to see if it had an mm.

+

+		ops_matched = 0;

+		rcu_read_lock();

+		do_each_thread(grp, tsk) {

+			int matched = 0;

 #ifdef PF_KTHREAD

-		// Ignore kernel threads.  On systems without

-		// PF_KTHREAD, we're ok, since kernel threads won't be

-		// matched by the stap_utrace_detach() call.

-		if (tsk->flags & PF_KTHREAD)

-			continue;

+			// Ignore kernel threads.  On systems without

+			// PF_KTHREAD, we're ok, since kernel threads

+			// won't be matched by the

+			// __stap_utrace_detach() call.

+			if (tsk->flags & PF_KTHREAD)

+				continue;

 #endif

-		/* Notice we're purposefully ignoring errors from

-		 * stap_utrace_detach().  Even if we got an error on

-		 * this task, we need to keep detaching from other

-		 * tasks.  But warn, we might be unloading and dangling

-		 * engines are bad news. */

-		rc = stap_utrace_detach(tsk, ops);

-		if (rc != 0)

-			_stp_error("stap_utrace_detach returned error %d on pid %d", rc, tsk->pid);

-		WARN_ON(rc != 0);

-	} while_each_thread(grp, tsk);

-	rcu_read_unlock();

-	debug_task_finder_report();

+			/* Notice we're purposefully ignoring errors

+			 * from __stap_utrace_detach().  Even if we

+			 * got an error on this task, we need to keep

+			 * detaching from other tasks.  But warn, we

+			 * might be unloading and dangling engines are

+			 * bad news. */

+			rc = __stap_utrace_detach(tsk, ops, &matched);

+			if (rc != 0)

+				_stp_error("stap_utrace_detach returned error %d on pid %d", rc, tsk->pid);

+			WARN_ON(rc != 0);

+			ops_matched |= matched;

+		} while_each_thread(grp, tsk);

+		rcu_read_unlock();

+		debug_task_finder_report();

+		iterations++;

+	} while (ops_matched && iterations < 10);

+#ifdef DEBUG_TASK_FINDER

+	_stp_dbug(__FUNCTION__, __LINE__, "took %d attempts\n", iterations);

+#endif

 }

 static void

@@ -980,13 +1017,13 @@ __stp_utrace_task_finder_report_clone(enum utrace_resume_action action,

 #endif

 	int rc;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	__stp_tf_handler_start();

-

 	// On clone, attach to the child.

 	rc = __stp_utrace_attach(child, engine->ops, 0,

 				 __STP_TASK_FINDER_EVENTS, UTRACE_RESUME);

@@ -1034,13 +1071,13 @@ __stp_utrace_task_finder_report_exec(enum utrace_resume_action action,

 	struct stap_task_finder_target *tgt;

 	int found_node = 0;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	__stp_tf_handler_start();

-

 	// If the original task was "interesting",

 	// __stp_utrace_task_finder_target_exec() will handle calling

 	// callbacks. 

@@ -1101,13 +1138,13 @@ __stp_utrace_task_finder_target_exec(enum utrace_resume_action action,

 	struct stap_task_finder_target *tgt = engine->data;

 	int rc;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	__stp_tf_handler_start();

-

 	// We'll hardcode this as a process end.  If a thread

 	// calls exec() (which it isn't supposed to), the kernel

 	// "promotes" it to being a process.  Call the callbacks.

@@ -1123,8 +1160,8 @@ __stp_utrace_task_finder_target_exec(enum utrace_resume_action action,

 	// __stp_utrace_attach_match_tsk() to figure out if the

 	// exec'ed program is "interesting".

-	__stp_tf_handler_end();

 	debug_task_finder_detach();

+	__stp_tf_handler_end();

 	return UTRACE_DETACH;

 }

@@ -1144,12 +1181,13 @@ __stp_utrace_task_finder_target_exit(struct utrace_engine *engine,

 #endif

 	struct stap_task_finder_target *tgt = engine->data;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	__stp_tf_handler_start();

 	// The first implementation of this added a

 	// UTRACE_EVENT(DEATH) handler to

 	// __stp_utrace_task_finder_ops.  However, dead threads don't

@@ -1163,8 +1201,8 @@ __stp_utrace_task_finder_target_exit(struct utrace_engine *engine,

 		__stp_call_callbacks(tgt, tsk, 0, (tsk->pid == tsk->tgid));

 	}

-	__stp_tf_handler_end();

 	debug_task_finder_detach();

+	__stp_tf_handler_end();

 	return UTRACE_DETACH;

 }

@@ -1332,18 +1370,19 @@ __stp_utrace_task_finder_target_quiesce(enum utrace_resume_action action,

 	struct stap_task_finder_target *tgt = engine->data;

 	int rc;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

 	if (tgt == NULL || tsk == NULL) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	__stp_tf_handler_start();

-

 	// Turn off quiesce handling

 	rc = utrace_set_events(tsk, engine,

 			       __STP_ATTACHED_TASK_BASE_EVENTS(tgt));

@@ -1418,13 +1457,17 @@ __stp_utrace_task_finder_target_syscall_entry(enum utrace_resume_action action,

 	int is_mprotect = 0;

 	int is_munmap = 0;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	if (tgt == NULL)

+	if (tgt == NULL) {

+		__stp_tf_handler_end();

 		return UTRACE_RESUME;

+	}

 	// See if syscall is one we're interested in.  On x86_64, this

 	// is a potentially expensive operation (since we have to

@@ -1442,20 +1485,23 @@ __stp_utrace_task_finder_target_syscall_entry(enum utrace_resume_action action,

 				     ? 1 : 0);

 		}

 	}

-	if (!is_mmap_or_mmap2 && !is_mprotect && !is_munmap)

+	if (!is_mmap_or_mmap2 && !is_mprotect && !is_munmap) {

+		__stp_tf_handler_end();

 		return UTRACE_RESUME;

+	}

 	// The syscall is one we're interested in, but do we have a

 	// handler for it?

 	if ((is_mmap_or_mmap2 && tgt->mmap_events == 0)

 	    || (is_mprotect && tgt->mprotect_events == 0)

-	    || (is_munmap && tgt->munmap_events == 0))

+	    || (is_munmap && tgt->munmap_events == 0)) {

+		__stp_tf_handler_end();

 		return UTRACE_RESUME;

+	}

 	// Save the needed arguments.  Note that for mmap, we really

 	// just need the return value, so there is no need to save

 	// any arguments.

-	__stp_tf_handler_start();

 	if (is_munmap) {

 		// We need 2 arguments for munmap()

 		syscall_get_arguments(tsk, regs, 0, 2, args);

@@ -1501,23 +1547,28 @@ __stp_utrace_task_finder_target_syscall_exit(enum utrace_resume_action action,

 	unsigned long rv;

 	struct __stp_tf_map_entry *entry;

+	__stp_tf_handler_start();

 	if (atomic_read(&__stp_task_finder_state) != __STP_TF_RUNNING) {

 		debug_task_finder_detach();

+		__stp_tf_handler_end();

 		return UTRACE_DETACH;

 	}

-	if (tgt == NULL)

+	if (tgt == NULL) {

+		__stp_tf_handler_end();

 		return UTRACE_RESUME;

+	}

 	// See if we can find saved syscall info.  If we can, it must

 	// be one of the syscalls we are interested in (and we must

 	// have callbacks to call for it).

 	entry = __stp_tf_get_map_entry(tsk);

-	if (entry == NULL)

+	if (entry == NULL) {

+		__stp_tf_handler_end();

 		return UTRACE_RESUME;

+	}

 	// Get return value

-	__stp_tf_handler_start();

 	rv = syscall_get_return_value(tsk, regs);

 	dbug_task_vma(1,

@@ -1547,8 +1598,8 @@ __stp_utrace_task_finder_target_syscall_exit(enum utrace_resume_action action,

 					      entry->arg1, entry->arg2);

 	}

-	__stp_tf_handler_end();

 	__stp_tf_remove_map_entry(entry);

+	__stp_tf_handler_end();

 	return UTRACE_RESUME;

 }