[PATCH 0/3] ptrace_reparented fixes/cleanups

[PATCH 0/3] ptrace_reparented fixes/cleanups

[Oleg Nesterov]

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.

[PATCH 1/3] ptrace: ptrace_reparented() should check same_thread_group()

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

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

--- ptrace/include/linux/ptrace.h~9_ptrace_reparanted_sg	2011-06-22 19:24:13.000000000 +0200
+++ ptrace/include/linux/ptrace.h	2011-06-23 19:21:00.000000000 +0200
@@ -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)

Re: [PATCH 1/3] ptrace: ptrace_reparented() should check same_thread_group()

[Tejun Heo]

Hello,

On Fri, Jun 24, 2011 at 05:34:06PM +0200, Oleg Nesterov wrote:
> 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.

This seems correct.

> 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:
...
> 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.

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.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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...

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 1/3] ptrace: ptrace_reparented() should check same_thread_group()

[Oleg Nesterov]

On 06/25, Tejun Heo wrote:
>
> This patch involves very subtle behavior change when the tracer is
> multi-threaded && real parent of the tracee.  Slightly worrisome

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.

> but
> it is a bug fix and extremely fringe, so...

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

>  Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

Oleg.

[PATCH 2/3] ptrace: kill real_parent_is_ptracer() in in favor of ptrace_reparented()

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

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

--- ptrace/kernel/signal.c~10_kill_real_parent_is_ptracer	2011-06-24 16:53:25.000000000 +0200
+++ ptrace/kernel/signal.c	2011-06-24 16:59:20.000000000 +0200
@@ -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;

Re: [PATCH 2/3] ptrace: kill real_parent_is_ptracer() in in favor of ptrace_reparented()

[Tejun Heo]

On Fri, Jun 24, 2011 at 05:34:23PM +0200, Oleg Nesterov wrote:
> 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.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH 3/3] ptrace: wait_consider_task: s/same_thread_group/ptrace_reparented/

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

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

--- ptrace/kernel/exit.c~11_p_r_more_users	2011-06-23 19:11:34.000000000 +0200
+++ ptrace/kernel/exit.c	2011-06-24 17:10:07.000000000 +0200
@@ -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;
 
 		/*

Re: [PATCH 3/3] ptrace: wait_consider_task: s/same_thread_group/ptrace_reparented/

[Tejun Heo]

On Fri, Jun 24, 2011 at 05:34:39PM +0200, Oleg Nesterov wrote:
> 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.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun