Re: Race condition in ptrace

[Nick Piggin]

Nick Piggin wrote:
> Andrew Morton wrote:
>
> > Nick Piggin <nickpiggin@xxxxxxxxxxxx> wrote:

> > > Andrew, IMO this is another bug to hold 2.6.11 for.
> >
> >
> >
> > Sure.  I wouldn't consider Bodo's patch to be the one to use though..
>
>
> No. Something similar could be done that works on all architectures
> and all wait_task_inactive callers (and is confined to sched.c). That
> would still be more or less a hack to work around smtnice's unfortunate
> locking though.

Something like the following (untested) extension of Bodo's work
could be the minimal fix for 2.6.11. As I've said though, I'd
consider it a hack and prefer to do something about the locking.
That could be done after 2.6.11 though. Depends how you feel.

Bodo, I wonder if this looks like a suitable fix for your problem?


---

 linux-2.6-npiggin/include/linux/init_task.h |    1 +
 linux-2.6-npiggin/include/linux/sched.h     |    1 +
 linux-2.6-npiggin/kernel/sched.c            |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff -puN include/linux/sched.h~sched-fixup-locking include/linux/sched.h
--- linux-2.6/include/linux/sched.h~sched-fixup-locking	2005-02-05 15:24:00.000000000 +1100
+++ linux-2.6-npiggin/include/linux/sched.h	2005-02-05 15:24:39.000000000 +1100
@@ -533,6 +533,7 @@ struct task_struct {
 	unsigned long ptrace;
 
 	int lock_depth;		/* Lock depth */
+	int on_cpu;		/* Is the task on the CPU, or in a ctxsw */
 
 	int prio, static_prio;
 	struct list_head run_list;
diff -puN kernel/sched.c~sched-fixup-locking kernel/sched.c
--- linux-2.6/kernel/sched.c~sched-fixup-locking	2005-02-05 15:24:02.000000000 +1100
+++ linux-2.6-npiggin/kernel/sched.c	2005-02-05 15:34:37.000000000 +1100
@@ -294,7 +294,7 @@ static DEFINE_PER_CPU(struct runqueue, r
 #ifndef prepare_arch_switch
 # define prepare_arch_switch(rq, next)	do { } while (0)
 # define finish_arch_switch(rq, next)	spin_unlock_irq(&(rq)->lock)
-# define task_running(rq, p)		((rq)->curr == (p))
+# define task_running(rq, p)		((p)->on_cpu)
 #endif
 
 /*
@@ -867,7 +867,7 @@ void wait_task_inactive(task_t * p)
 repeat:
 	rq = task_rq_lock(p, &flags);
 	/* Must be off runqueue entirely, not preempted. */
-	if (unlikely(p->array)) {
+	if (unlikely(p->array || p->on_cpu)) {
 		/* If it's preempted, we yield.  It could be a while. */
 		preempted = !task_running(rq, p);
 		task_rq_unlock(rq, &flags);
@@ -2805,11 +2805,18 @@ switch_tasks:
 		rq->curr = next;
 		++*switch_count;
 
+		next->on_cpu = 1;
 		prepare_arch_switch(rq, next);
 		prev = context_switch(rq, prev, next);
 		barrier();
 
 		finish_task_switch(prev);
+		/*
+		 * Some architectures release the runqueue lock before
+		 * context switching. Make sure this isn't reordered.
+		 */
+		smp_wmb();
+		prev->on_cpu = 0;
 	} else
 		spin_unlock_irq(&rq->lock);
 
@@ -4055,6 +4062,7 @@ void __devinit init_idle(task_t *idle, i
 	set_task_cpu(idle, cpu);
 
 	spin_lock_irqsave(&rq->lock, flags);
+	idle->on_cpu = 1;
 	rq->curr = rq->idle = idle;
 	set_tsk_need_resched(idle);
 	spin_unlock_irqrestore(&rq->lock, flags);
diff -puN include/linux/init_task.h~sched-fixup-locking include/linux/init_task.h
--- linux-2.6/include/linux/init_task.h~sched-fixup-locking	2005-02-05 15:24:56.000000000 +1100
+++ linux-2.6-npiggin/include/linux/init_task.h	2005-02-05 15:25:07.000000000 +1100
@@ -73,6 +73,7 @@ extern struct group_info init_groups;
 	.usage		= ATOMIC_INIT(2),				\
 	.flags		= 0,						\
 	.lock_depth	= -1,						\
+	.on_cpu		= 0,						\
 	.prio		= MAX_PRIO-20,					\
 	.static_prio	= MAX_PRIO-20,					\
 	.policy		= SCHED_NORMAL,					\

_