Re: [PATCH] fix wait_task_inactive race (was Re: Race condition inptrace)

[Nick Piggin]

Ingo Molnar wrote:
> * Nick Piggin <nickpiggin@xxxxxxxxxxxx> wrote:
>
>
> > When a task is put to sleep, it is dequeued from the runqueue
> > while it is still running. The problem is that the runqueue
> > lock can be dropped and retaken in schedule() before the task
> > actually schedules off, and wait_task_inactive did not account
> > for this.
>
>
> ugh. This has been the Nth time we got bitten by the fundamental
> unrobustness of non-atomic scheduling on some architectures ...
> (And i'll say the N+1th time that this is not good.)

This is actually due to wake_sleeping_dependent and
dependent_sleeper dropping the runqueue lock.

Actually idle_balance can (in rare cases) drop the lock as well,
which I didn't notice earlier, so it is something that we
have been doing forever. The smtnice locking has just exposed
the problem for us, so I wrongfully bashed it earlier *blush*.

> > +static int task_onqueue(runqueue_t *rq, task_t *p)
> > +{
> > +	return (p->array || task_running(rq, p));
> > +}
>
>
> the fix looks good, but i'd go for the simplified oneliner patch below. 
> I dont like the name 'task_onqueue()', a because a task is 'on the
> queue' when it has p->array != NULL. The task is running when it's
> task_running().  On architectures with nonatomic scheduling a task may
> be running while not on the queue and external observers with the
> runqueue lock might notice this - and wait_task_inactive() has to take
> care of this case. I'm not sure how introducing a function named
> "task_onqueue()" will make the situation any clearer.
>
> ok?

Well just because there is a specific condition that both callsites
require. That is, the task is neither running nor on the runqueue.

While task_onqueue is technically wrong if you're looking down into
the fine details of the priority queue management, I think it is OK
to go up a level of abstraction and say that the task is
"on the runqueue" if it is either running or waiting to run.

It is really the one condition that is made un-intuitive due to the
locking in schedule(), so I thought formalising it would be better.
Suggestions for a better name welcome? ;)

Your one liner would fix the problem too, of course. The important
thing at this stage is that it gets fixed for 2.6.11.

Thanks,
Nick


-
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/