ptrace_reparented fixes/cleanups

Hello.

Another thing I wanted to cleanup for so long time.

ptrace_reparented() is wrong. Now we have real_parent_is_ptracer() which does the right thing, but the name is ugky and it is not exported.

Fix ptrace_reparented() and kill the recently added real_parent_is_ptracer().

Oleg.

ptrace_reparented() naively does parent != real_parent, this means it returns true even if the tracer _is_ the real parent. This is per process thing, not per-thread. The only reason ->real_parent can point to the non-leader thread is that we have __WNOTHREAD.

Change it to check !same_thread_group(parent, real_parent).

It has two callers, and in both cases the current check does not look right.

exit_notify: we should respect ->exit_signal if the exiting leader is traced by any thread from the parent thread group. It is the child of the whole group, and we are going to send the signal to the whole group.

wait_task_zombie: without __WNOTHREAD do_wait() should do the same for any thread, only sys_ptrace() is "bound" to the single thread. However do_wait(WEXITED) succeeds but does not release a traced natural child unless the caller is the tracer.

Test-case:

	void *tfunc(void *arg)
	{
		assert(ptrace(PTRACE_ATTACH, (long)arg, 0,0) == 0);
		pause();
		return NULL;
	}

	int main(void)
	{
		pthread_t thr;
		pid_t pid, stat, ret;

		pid = fork();
		if (!pid) {
			pause();
			assert(0);
		}

		assert(pthread_create(&thr, NULL, tfunc, (void*)(long)pid) == 0);

		assert(waitpid(-1, &stat, 0) == pid);
		assert(WIFSTOPPED(stat));

		kill(pid, SIGKILL);

		assert(waitpid(-1, &stat, 0) == pid);
		assert(WIFSIGNALED(stat) && WTERMSIG(stat) == SIGKILL);

		ret = waitpid(pid, &stat, 0);
		if (ret < 0)
			return 0;

		printf("WTF? %d is dead, but: wait=%d stat=%x\n",
				pid, ret, stat);

		return 1;
	}

Note that the main thread simply does

	pid = fork();
	kill(pid, SIGKILL);

and then without the patch wait4(WEXITED) succeeds twice and reports WTERMSIG(stat) == SIGKILL.

---

 include/linux/ptrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -136,7 +136,7 @@ extern bool ptrace_may_access(struct tas
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
-	return child->real_parent != child->parent;
+	return !same_thread_group(child->real_parent, child->parent);
 }
 
 static inline void ptrace_unlink(struct task_struct *child)

Hello,

This seems correct.

...

This latter one is interesting to say the least. Yes, wait(2) is process-wide operation and should behave identically when for all threads in the same process.

This patch involves very subtle behavior change when the tracer is multi-threaded && real parent of the tracee. Slightly worrisome but it is a bug fix and extremely fringe, so...

Thanks.

tejun

Agreed. That is why, despite the fact the change itself is trivial, the changelog is so long.

I am almost sure nobody will ever notice this change, but still this is the user-visible change.

Agreed. Even if very minor, this looks like a bug, imho makes sense to fix.

Thanks.

Oleg.

Kill real_parent_is_ptracer() and update the callers to use ptrace_reparented(), after the previous patch they do the same.

Remove the unnecessary ->ptrace != 0 check in get_signal_to_deliver(), if ptrace_reparented() == T then the task must be ptraced.

---

 kernel/signal.c |   20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

@@ -1760,15 +1760,6 @@ static int sigkill_pending(struct task_s
 }
 
 /*
- * Test whether the target task of the usual cldstop notification - the
- * real_parent of @child - is in the same group as the ptracer.
- */
-static bool real_parent_is_ptracer(struct task_struct *child)
-{
-	return same_thread_group(child->parent, child->real_parent);
-}
-
-/*
  * This must be called with current->sighand->siglock held.
  *
  * This should be the path for all ptrace stops.

@@ -1848,7 +1839,7 @@ static void ptrace_stop(int exit_code, i
 		 * separately unless they're gonna be duplicates.
 		 */
 		do_notify_parent_cldstop(current, true, why);
-		if (gstop_done && !real_parent_is_ptracer(current))
+		if (gstop_done && ptrace_reparented(current))
 			do_notify_parent_cldstop(current, false, why);
 
 		/*

@@ -2154,7 +2145,6 @@ relock:
 	 * the CLD_ si_code into SIGNAL_CLD_MASK bits.
 	 */
 	if (unlikely(signal->flags & SIGNAL_CLD_MASK)) {
-		struct task_struct *leader;
 		int why;
 
 		if (signal->flags & SIGNAL_CLD_CONTINUED)

@@ -2175,13 +2165,11 @@ relock:
 		 * a duplicate.
 		 */
 		read_lock(&tasklist_lock);
-
 		do_notify_parent_cldstop(current, false, why);
 
-		leader = current->group_leader;
-		if (leader->ptrace && !real_parent_is_ptracer(leader))
-			do_notify_parent_cldstop(leader, true, why);
-
+		if (ptrace_reparented(current->group_leader))
+			do_notify_parent_cldstop(current->group_leader,
+						true, why);
 		read_unlock(&tasklist_lock);
 
 		goto relock;

Thanks.

tejun

wait_consider_task() checks same_thread_group(parent, real_parent), this is the open-coded ptrace_reparented().

__ptrace_detach() remains the only function which has to check this by hand, although we could reorganize the code to delay __ptrace_unlink.

---

 kernel/exit.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

@@ -1598,8 +1598,7 @@ static int wait_consider_task(struct wai
 		 * own children, it should create a separate process which
 		 * takes the role of real parent.
 		 */
-		if (likely(!ptrace) && p->ptrace &&
-		    same_thread_group(p->parent, p->real_parent))
+		if (likely(!ptrace) && p->ptrace && !ptrace_reparented(p))
 			return 0;
 
 		/*

Thanks.

tejun