fix runqueue corruption, 2.6.1-rc3-A0

the attached patch fixes a nasty, few-instructions race that can result in a corrupted runqueue at thread/process-creation time. The bug is this code in do_fork():

p->state = TASK_STOPPED;
if (!(clone_flags & CLONE_STOPPED))
wake_up_forked_process(p); /* do this last */

when we do copy_process(), the task ends up on the tasklist and is pid-hashed, and is thus accessible as a signal-wakeup target. But it's in TASK_UNINTERRUPTIBLE state so wakeups cannot happen. But:

    void wake_up_forked_process(task_t * p)
    {
            unsigned long flags;
            runqueue_t *rq = task_rq_lock(current, &flags);

            BUG_ON(p->state != TASK_UNINTERRUPTIBLE);
            p->state = TASK_RUNNING;

so the task can be woken up from the place we do the TASK_STOPPED change, to the task_rq_lock(). This window is very small, but not impossible to trigger. The window is more likely to trigger on hyperthreading systems and when CONFIG_PREEMPT is enabled. (in fact we have a number of bugreports that i suspect are related to this race.) The bug was introduced 6 months ago.

The effect of the bug was quite hard to debug: it results in a corrupted runqueue due to the double list_add() - this causes lockups next time this area of the runqueue is used, far away from the buggy code itself.

the fix is to set it to TASK_STOPPED only if we dont call wake_up_forked_process(). (Also, to avoid this bug in the future i've added an assert to catch illegal uses of wake_up_forked_process().)

please apply.

Ingo

@@ -1209,9 +1209,16 @@ long do_fork(unsigned long clone_flags,
 			set_tsk_thread_flag(p, TIF_SIGPENDING);
 		}
 
-		p->state = TASK_STOPPED;
+		/*
+		 * the task is in TASK_UNINTERRUPTIBLE right now, no-one
+		 * can wake it up. Either wake it up as a child, which
+		 * makes it TASK_RUNNING - or make it TASK_STOPPED, after
+		 * which signals can wake the child up.
+		 */
 		if (!(clone_flags & CLONE_STOPPED))
 			wake_up_forked_process(p);	/* do this last */
+		else
+			set_task_state(p, TASK_STOPPED);
 		++total_forks;
 
 		if (unlikely (trace)) {

@@ -684,6 +684,7 @@ void wake_up_forked_process(task_t * p)
 	unsigned long flags;
 	runqueue_t *rq = task_rq_lock(current, &flags);
 
+	BUG_ON(p->state != TASK_UNINTERRUPTIBLE);
 	p->state = TASK_RUNNING;
 	/*
 	 * We decrease the sleep average of forking parents

@@ -2832,6 +2833,7 @@ void __init sched_init(void)
 	rq = this_rq();
 	rq->curr = current;
 	rq->idle = current;
+	current->state = TASK_UNINTERRUPTIBLE;
 	set_task_cpu(current, smp_processor_id());
 	wake_up_forked_process(current);
 

The bug is more than that - we don't do the proper accountign setup for the CLONE_STOPPED case. Look at all the interactive bias stuff that doesn't get run at all..

I don't like it. That "TASK_UNINTERRUPTIBLE" assert is simply wrong: it's perfectly ok to call the function with a TASK_RUNNING thread too, since such a process can not be added to the runqueue by anything else (wake_up_process() won't touch it since it's already awake).

So a more proper assert would be to check

	BUG_ON(p->state & ~TASK_UNINTERRUPIBLE);

or, preferably to get rid of the TASK_UNINTERRUPTIBLE special case altogether, and just say "this function has to be called for something that is already marked as being running".

In other words, wouldn't it be saner with something like this instead (which still leaves the issue of the interactive bias unfixed, and is TOTALLY UNTESTED!).

Linus

---
===== kernel/fork.c 1.149 vs edited =====

@@ -918,7 +918,7 @@
 	p->thread_info->preempt_count = 1;
 #endif
 	p->did_exec = 0;
-	p->state = TASK_UNINTERRUPTIBLE;
+	p->state = TASK_RUNNING;
 
 	copy_flags(clone_flags, p);
 	if (clone_flags & CLONE_IDLETASK)

@@ -1209,9 +1209,10 @@
 			set_tsk_thread_flag(p, TIF_SIGPENDING);
 		}
 
-		p->state = TASK_STOPPED;
 		if (!(clone_flags & CLONE_STOPPED))
 			wake_up_forked_process(p);	/* do this last */
+		else
+			p->state = TASK_STOPPED;
 		++total_forks;
 
 		if (unlikely (trace)) {

===== kernel/sched.c 1.227 vs edited =====

@@ -684,7 +684,8 @@
 	unsigned long flags;
 	runqueue_t *rq = task_rq_lock(current, &flags);
 
-	p->state = TASK_RUNNING;
+	BUG_ON(p->state != TASK_RUNNING);
+
 	/*
 	 * We decrease the sleep average of forking parents
 	 * and children as well, to keep max-interactive tasks

yep, agreed, this is another bug. It is fixed by an existing scheduler patch (queued for 2.6.2, sched-cleanup-2.6.0-A2) but we might want to do it in 2.6.1. I've attached the patch, merged relative to 2.6.1-rc3 + your version of the threading fix. The changelog:

- separate out the scheduler-state initializations from the fork path and push them into sched.c.

- this also fixes a bug: CLONE_STOPPED tasks were not fully set up.

yep, agreed, your patch is cleaner. (i've test-booted it on SMP, it works fine. I've also test-booted the sched-cleanup patch.)

Ingo

- separate out the scheduler-state initializations from the fork path and push them into sched.c.

- this also fixes a bug: CLONE_STOPPED tasks were not full set up.

@@ -579,6 +579,7 @@ extern int FASTCALL(wake_up_process(stru
  static inline void kick_process(struct task_struct *tsk) { }
 #endif
 extern void FASTCALL(wake_up_forked_process(struct task_struct * tsk));
+extern void FASTCALL(sched_fork(task_t * p));
 extern void FASTCALL(sched_exit(task_t * p));
 
 asmlinkage long sys_wait4(pid_t pid,unsigned int * stat_addr, int options, struct rusage * ru);