Commit 2f4e6e2a authored by Oleg Nesterov's avatar Oleg Nesterov Committed by Linus Torvalds

wait_task_zombie: fix 2/3 races vs forget_original_parent()

Two threads, T1 and T2.  T2 ptraces P, and P is not a child of ptracer's
thread group.  P exits and goes to TASK_ZOMBIE.

T1 does wait_task_zombie(P):

	P->exit_state = TASK_DEAD;
	...
	read_unlock(&tasklist_lock);

					T2 does exit(), takes tasklist,
					forget_original_parent() does
					__ptrace_unlink(P) but doesn't
					call do_notify_parent(P) because
					p->exit_state == EXIT_DEAD.

Now, P is not visible to our process: __ptrace_unlink() removed it from
->children. We should send notification to P->parent and release P if and
only if SIGCHLD is ignored.

And we have 3 bugs:

1. P->parent does do_wait() and gets -ECHILD (P is on ->parent->children,
   but its state is TASK_DEAD).

2. // wait_task_zombie() continues

	if (put_user(...)) {
		// TODO: is this safe?
		p->exit_state = EXIT_ZOMBIE;
		return;
	}

   we return without notification/release, task_struct leaked.

   Solution: ignore -EFAULT and proceed. It is an application's bug if
   we can't fill infop/stat_addr (in case of VM_FAULT_OOM we have much
   more problems).

3. // wait_task_zombie() continues

	if (p->real_parent != p->parent) {
		// Not taken, it was untraced'ed
		...
	}

	release_task(p);

   we released the task which we shouldn't.

   Solution: check ->real_parent != ->parent before, under tasklist_lock,
   but use ptrace_unlink() instead of __ptrace_unlink() to check ->ptrace.

This patch hopefully solves 2 and 3, the 1st bug will be fixed later, we need
some cleanups in forget_original_parent/reparent_thread.

However, the first race is very unlikely and not critical, so I hope it makes
sense to fix 1 and 2 for now.

4. Small cleanup: don't "restore" EXIT_ZOMBIE unless we know we are not going
   to realease the child.
Signed-off-by: default avatarOleg Nesterov <oleg@tv-sign.ru>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Roland McGrath <roland@redhat.com>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent 407af46a
...@@ -1158,8 +1158,7 @@ static int wait_task_zombie(struct task_struct *p, int noreap, ...@@ -1158,8 +1158,7 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
int __user *stat_addr, struct rusage __user *ru) int __user *stat_addr, struct rusage __user *ru)
{ {
unsigned long state; unsigned long state;
int retval; int retval, status, traced;
int status;
if (unlikely(noreap)) { if (unlikely(noreap)) {
pid_t pid = p->pid; pid_t pid = p->pid;
...@@ -1201,7 +1200,10 @@ static int wait_task_zombie(struct task_struct *p, int noreap, ...@@ -1201,7 +1200,10 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
return 0; return 0;
} }
if (likely(p->real_parent == p->parent)) { /* traced means p->ptrace, but not vice versa */
traced = (p->real_parent != p->parent);
if (likely(!traced)) {
struct signal_struct *psig; struct signal_struct *psig;
struct signal_struct *sig; struct signal_struct *sig;
...@@ -1288,19 +1290,13 @@ static int wait_task_zombie(struct task_struct *p, int noreap, ...@@ -1288,19 +1290,13 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
retval = put_user(p->pid, &infop->si_pid); retval = put_user(p->pid, &infop->si_pid);
if (!retval && infop) if (!retval && infop)
retval = put_user(p->uid, &infop->si_uid); retval = put_user(p->uid, &infop->si_uid);
if (retval) { if (!retval)
// TODO: is this safe?
p->exit_state = EXIT_ZOMBIE;
return retval;
}
retval = p->pid; retval = p->pid;
if (p->real_parent != p->parent) {
if (traced) {
write_lock_irq(&tasklist_lock); write_lock_irq(&tasklist_lock);
/* Double-check with lock held. */ /* We dropped tasklist, ptracer could die and untrace */
if (p->real_parent != p->parent) { ptrace_unlink(p);
__ptrace_unlink(p);
// TODO: is this safe?
p->exit_state = EXIT_ZOMBIE;
/* /*
* If this is not a detached task, notify the parent. * If this is not a detached task, notify the parent.
* If it's still not detached after that, don't release * If it's still not detached after that, don't release
...@@ -1308,7 +1304,8 @@ static int wait_task_zombie(struct task_struct *p, int noreap, ...@@ -1308,7 +1304,8 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
*/ */
if (p->exit_signal != -1) { if (p->exit_signal != -1) {
do_notify_parent(p, p->exit_signal); do_notify_parent(p, p->exit_signal);
if (p->exit_signal != -1) if (p->exit_signal != -1) {
p->exit_state = EXIT_ZOMBIE;
p = NULL; p = NULL;
} }
} }
...@@ -1316,7 +1313,7 @@ static int wait_task_zombie(struct task_struct *p, int noreap, ...@@ -1316,7 +1313,7 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
} }
if (p != NULL) if (p != NULL)
release_task(p); release_task(p);
BUG_ON(!retval);
return retval; return retval;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment