2000-11-18 04:32:12

by Andrew Morton

[permalink] [raw]
Subject: [patch] potential death in disassociate_ctty()


The call to disassociate_ctty() in exit_notify() is very dangerous. If
disassociate_ctty() calls schedule() then either:

- a parent process who is spinning in fork.c:release() will stop
spinning and will proceed to deallocate the child process's kernel
stack. This will probably have adverse effects when it is rewoken.

- Or, if disassociate_ctty() sets current->state to TASK_RUNNING and
the timing is right, the exitting child will no longer be in state
TASK_ZOMBIE and will continue to run. It hits the fake_volatile goto
in do_exit() and has another go at zombifying itself.

Not sure what happens next, but you end up with every process
sleeping and your machine freezes halfway through your initscripts.

Basically, we mustn't sleep in disassociate_ctty(). But this function
does all sorts of things, inclusing calling the open and close methods
of tty drivers and ldisc drivers. They _do_ call functions which can
sleep.

I think it's safer to not call disassociate_ctty() when we're in state
TASK_ZOMBIE. This patch moves the parent notification and the task
state change to _after_ the call to disassociate_ctty(). As an added
bonus, it's a little more efficient: the later we wake the parent, the
less time the parent has to spend spinning on current->has_cpu.

I'm not particularly confident about this one. What are the effects of
moving the call to do_notify_parent()? None, I think - if it mattered
we'd be racy anyway.

Also, somewhere on the path from kernel 2.2 to 2.4 the call to
do_notify_parent() was moved inside the tasklist lock. Why was this?
It seems unnecessary. This patch backs out that change, perhaps
wrongly...



--- linux-2.4.0-test11-pre7/kernel/exit.c Sat Nov 18 13:55:32 2000
+++ linux-akpm/kernel/exit.c Sat Nov 18 14:37:39 2000
@@ -382,7 +382,6 @@
*/

write_lock_irq(&tasklist_lock);
- do_notify_parent(current, current->exit_signal);
while (current->p_cptr != NULL) {
p = current->p_cptr;
current->p_cptr = p->p_osptr;
@@ -418,6 +417,10 @@

if (current->leader)
disassociate_ctty(1);
+
+ /* From now on, we must not sleep */
+ set_current_state(TASK_ZOMBIE);
+ do_notify_parent(current, current->exit_signal);
}

NORET_TYPE void do_exit(long code)
@@ -444,7 +447,6 @@
__exit_fs(tsk);
exit_sighand(tsk);
exit_thread();
- tsk->state = TASK_ZOMBIE;
tsk->exit_code = code;
exit_notify();
put_exec_domain(tsk->exec_domain);


2000-11-18 06:04:55

by Linus Torvalds

[permalink] [raw]
Subject: Re: [patch] potential death in disassociate_ctty()

In article <[email protected]>,
Andrew Morton <[email protected]> wrote:
>
>Also, somewhere on the path from kernel 2.2 to 2.4 the call to
>do_notify_parent() was moved inside the tasklist lock. Why was this?

Ehh.. Because that is also what protects our "parent" pointer.

Linus