aboutsummaryrefslogtreecommitdiffstats
path: root/kernel
diff options
context:
space:
mode:
authorOleg Nesterov <oleg@tv-sign.ru>2007-10-17 02:26:58 -0400
committerLinus Torvalds <torvalds@woody.linux-foundation.org>2007-10-17 11:42:51 -0400
commit2f4e6e2a814eb1305a873a045401708d73f870bc (patch)
tree8860a0e557c6849143c363eb7c1c9e82333e7fe3 /kernel
parent407af46a967ffd2f208f0a5fb3f1ff954801494a (diff)
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: Oleg Nesterov <oleg@tv-sign.ru> Cc: Ingo Molnar <mingo@elte.hu> Cc: Roland McGrath <roland@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Diffstat (limited to 'kernel')
-rw-r--r--kernel/exit.c45
1 files changed, 21 insertions, 24 deletions
diff --git a/kernel/exit.c b/kernel/exit.c
index 9d6e0897a447..b4d569675d4b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1158,8 +1158,7 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
1158 int __user *stat_addr, struct rusage __user *ru) 1158 int __user *stat_addr, struct rusage __user *ru)
1159{ 1159{
1160 unsigned long state; 1160 unsigned long state;
1161 int retval; 1161 int retval, status, traced;
1162 int status;
1163 1162
1164 if (unlikely(noreap)) { 1163 if (unlikely(noreap)) {
1165 pid_t pid = p->pid; 1164 pid_t pid = p->pid;
@@ -1201,7 +1200,10 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
1201 return 0; 1200 return 0;
1202 } 1201 }
1203 1202
1204 if (likely(p->real_parent == p->parent)) { 1203 /* traced means p->ptrace, but not vice versa */
1204 traced = (p->real_parent != p->parent);
1205
1206 if (likely(!traced)) {
1205 struct signal_struct *psig; 1207 struct signal_struct *psig;
1206 struct signal_struct *sig; 1208 struct signal_struct *sig;
1207 1209
@@ -1288,35 +1290,30 @@ static int wait_task_zombie(struct task_struct *p, int noreap,
1288 retval = put_user(p->pid, &infop->si_pid); 1290 retval = put_user(p->pid, &infop->si_pid);
1289 if (!retval && infop) 1291 if (!retval && infop)
1290 retval = put_user(p->uid, &infop->si_uid); 1292 retval = put_user(p->uid, &infop->si_uid);
1291 if (retval) { 1293 if (!retval)
1292 // TODO: is this safe? 1294 retval = p->pid;
1293 p->exit_state = EXIT_ZOMBIE; 1295
1294 return retval; 1296 if (traced) {
1295 }
1296 retval = p->pid;
1297 if (p->real_parent != p->parent) {
1298 write_lock_irq(&tasklist_lock); 1297 write_lock_irq(&tasklist_lock);
1299 /* Double-check with lock held. */ 1298 /* We dropped tasklist, ptracer could die and untrace */
1300 if (p->real_parent != p->parent) { 1299 ptrace_unlink(p);
1301 __ptrace_unlink(p); 1300 /*
1302 // TODO: is this safe? 1301 * If this is not a detached task, notify the parent.
1303 p->exit_state = EXIT_ZOMBIE; 1302 * If it's still not detached after that, don't release
1304 /* 1303 * it now.
1305 * If this is not a detached task, notify the parent. 1304 */
1306 * If it's still not detached after that, don't release 1305 if (p->exit_signal != -1) {
1307 * it now. 1306 do_notify_parent(p, p->exit_signal);
1308 */
1309 if (p->exit_signal != -1) { 1307 if (p->exit_signal != -1) {
1310 do_notify_parent(p, p->exit_signal); 1308 p->exit_state = EXIT_ZOMBIE;
1311 if (p->exit_signal != -1) 1309 p = NULL;
1312 p = NULL;
1313 } 1310 }
1314 } 1311 }
1315 write_unlock_irq(&tasklist_lock); 1312 write_unlock_irq(&tasklist_lock);
1316 } 1313 }
1317 if (p != NULL) 1314 if (p != NULL)
1318 release_task(p); 1315 release_task(p);
1319 BUG_ON(!retval); 1316
1320 return retval; 1317 return retval;
1321} 1318}
1322 1319