Re: [BUG] TASK_DEAD task is able to be woken up in special condition

[Yasunori Goto]

Hello,

I made a trial patch to change task state by cmpxchg to solve this problem.
Please comment.

I just confirmed booting up on my box, and I would like to get rough agreement
about this way to solve this issue at first.
If this way is roughly ok, I will test this patch more.

Thanks,

--------------------

try_to_wake_up() has a problem which may change status from TASK_DEAD to
TASK_RUNNING in race condition with SMI or guest environment of virtual
machine. (See: https://lkml.org/lkml/2011/12/21/523)
As a result, exited task is scheduled() again and panic occurs.

In addition, try_to_wake_up(TASK_INTERRUPTIBLE) can wakeup a
TASK_UNINTERRUPTIBLE task if it temporary sets INTERRUPTIBLE but
doesn't call schedule() in this state.

By this patch, kernel changes task state by cmpxchg when the task is
on run queue. If the task state does not fit required state,
kernel stops waking the task up.


Signed-off-by: Yasunori Goto <y-goto@xxxxxxxxxxxxxx>

---
 kernel/sched.c |   96 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 79 insertions(+), 17 deletions(-)

Index: linux-3.2-rc7/kernel/sched.c
===================================================================
--- linux-3.2-rc7.orig/kernel/sched.c
+++ linux-3.2-rc7/kernel/sched.c
@@ -2650,16 +2650,31 @@ static void ttwu_activate(struct rq *rq,
 		wq_worker_waking_up(p, cpu_of(rq));
 }
 
-/*
- * Mark the task runnable and perform wakeup-preemption.
- */
-static void
-ttwu_do_wakeup(struct rq *rq, struct task_struct *p, int wake_flags)
+static int
+change_task_state_cmpxchg(struct task_struct *p, unsigned int target_state,
+			  unsigned int new)
 {
-	trace_sched_wakeup(p, true);
-	check_preempt_curr(rq, p, wake_flags);
+	unsigned int old, tmp;
 
-	p->state = TASK_RUNNING;
+	old = p->state;
+
+	while (1) {
+		if (old == new)
+			return 1;
+
+		if (unlikely(!(old & target_state)))
+			return 0;
+
+		tmp = cmpxchg(&p->state, old, new);
+		if (likely(tmp == old))
+			return 1;
+
+		old = tmp;
+	}
+}
+
+static void ttwu_do_woken(struct rq *rq, struct task_struct *p)
+{
 #ifdef CONFIG_SMP
 	if (p->sched_class->task_woken)
 		p->sched_class->task_woken(rq, p);
@@ -2677,6 +2692,45 @@ ttwu_do_wakeup(struct rq *rq, struct tas
 #endif
 }
 
+/*
+ * Mark the task runnable and perform wakeup-preemption.
+ */
+static void
+ttwu_do_wakeup(struct rq *rq, struct task_struct *p, int wake_flags)
+{
+	trace_sched_wakeup(p, true);
+	check_preempt_curr(rq, p, wake_flags);
+
+	p->state = TASK_RUNNING;
+
+	ttwu_do_woken(rq, p);
+}
+
+/*
+ * Mark the task runnable and perform wakeup-preemption with
+ * making sure the state of task. It might be changed due to
+ * race condition.
+ */
+static int
+ttwu_do_wakeup_state_check(struct rq *rq, struct task_struct *p,
+			   unsigned int target_state, int wake_flags)
+{
+	int ret;
+
+	ret = change_task_state_cmpxchg(p, target_state, TASK_RUNNING);
+	trace_sched_wakeup(p, ret);
+
+	/* faied to change state due to race? */
+	if (!ret)
+		return -EINVAL;
+
+	check_preempt_curr(rq, p, wake_flags);
+
+	ttwu_do_woken(rq, p);
+
+	return 1;
+}
+
 static void
 ttwu_do_activate(struct rq *rq, struct task_struct *p, int wake_flags)
 {
@@ -2695,16 +2749,16 @@ ttwu_do_activate(struct rq *rq, struct t
  * since all we need to do is flip p->state to TASK_RUNNING, since
  * the task is still ->on_rq.
  */
-static int ttwu_remote(struct task_struct *p, int wake_flags)
+static int
+ttwu_remote(struct task_struct *p, unsigned int state, int wake_flags)
 {
 	struct rq *rq;
 	int ret = 0;
 
 	rq = __task_rq_lock(p);
-	if (p->on_rq) {
-		ttwu_do_wakeup(rq, p, wake_flags);
-		ret = 1;
-	}
+	if (p->on_rq)
+		ret = ttwu_do_wakeup_state_check(rq, p, state, wake_flags);
+
 	__task_rq_unlock(rq);
 
 	return ret;
@@ -2766,7 +2820,8 @@ static void ttwu_queue_remote(struct tas
 }
 
 #ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW
-static int ttwu_activate_remote(struct task_struct *p, int wake_flags)
+static int
+ttwu_activate_remote(struct task_struct *p, int wake_flags)
 {
 	struct rq *rq;
 	int ret = 0;
@@ -2828,11 +2883,18 @@ try_to_wake_up(struct task_struct *p, un
 	if (!(p->state & state))
 		goto out;
 
-	success = 1; /* we're going to change ->state */
 	cpu = task_cpu(p);
 
-	if (p->on_rq && ttwu_remote(p, wake_flags))
-		goto stat;
+	if (p->on_rq) {
+		int ret;
+		ret = ttwu_remote(p, state, wake_flags);
+		if (likely(ret == 1)) {
+			success = 1;
+			goto stat;
+		} else if (unlikely(ret < 0))
+			goto out;
+	}
+	success = 1; /* we're going to change ->state */
 
 #ifdef CONFIG_SMP
 	/*


-- 
Yasunori Goto 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/