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

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

On 1/24/2012 5:55 AM, Peter Zijlstra wrote:
> On Tue, 2012-01-24 at 11:19 +0100, Peter Zijlstra wrote:
>> On Wed, 2012-01-18 at 15:20 +0100, Oleg Nesterov wrote:
>>> do_exit() is different because it can not handle the spurious wakeup.
>>> Well, may be we can? we can simply do
>>>
>>>                 for (;;) {
>>>                         tsk->state = TASK_DEAD;
>>>                         schedule();
>>>                 }
>>>
>>> __schedule() can't race with ttwu() once it takes rq->lock. If the
>>> exiting task is deactivated, finish_task_switch() will see EXIT_DEAD.
>>
>> TASK_DEAD, right?
>>
>>> Unless I missed something, the only problem is preempt_disable(),
>>> but schedule_debug() checks ->exit_state.
>>>
>>> OTOH, if we fix this race then probably schedule_debug() should
>>> check state == EXIT_DEAD instead. 
>>
>> Hmm, interesting. On the up side that removes the need for that inf loop
>> after BUG, down side is of course that we loose the BUG itself too. Now
>> I'm not too sure we actually care about that, a task spinning at 100% in
>> x state should be fairly obvious borkage and its not like we hit this
>> thing very often.
> 
> Something like so, right? schedule_debug() already tests
> prev->exit_state so it should DTRT afaict.
> 
> Also, while going over this again, I think Yasunori-San's patch isn't
> sufficient, note how the p->state = TASK_RUNNING in ttwu_do_wakeup() can
> happen outside of p->pi_lock when the task gets queued on a remote cpu.
> 
> ---
>  kernel/exit.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 294b170..ccd4f84 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1039,13 +1039,18 @@ void do_exit(long code)
>  		__this_cpu_add(dirty_throttle_leaks, tsk->nr_dirtied);
>  	exit_rcu();
>  	/* causes final put_task_struct in finish_task_switch(). */
> -	tsk->state = TASK_DEAD;
>  	tsk->flags |= PF_NOFREEZE;	/* tell freezer to ignore us */
> -	schedule();
> -	BUG();
> -	/* Avoid "noreturn function does return".  */
> -	for (;;)
> -		cpu_relax();	/* For when BUG is null */
> +	for (;;) {
> +		/*
> +		 * A spurious wakeup, eg. generated by rwsem when down()'s call
> +		 * to schedule() doesn't happen but the wakeup from the
> +		 * previous owner's up() did, can stomp on our ->state.
> +		 *
> +		 * This loop also avoids "noreturn functions does return"
> +		 */
> +		tsk->state = TASK_DEAD;
> +		schedule();
> +	}
>  }

This looks ok to me.