Re: Q: down_killable() is racy? or schedule() is not right?

[Oleg Nesterov <oleg@tv-sign.ru>]

On 06/03, Matthew Wilcox wrote:
>
> On Tue, Jun 03, 2008 at 04:33:09PM +0400, Oleg Nesterov wrote:
> > 
> > Why _irqsave ? we must not do down() with irqs disabled, and of course
> > __down() restores/clears irqs unconditionally.
> 
> How about reading the fine comments?

Thanks,

> I would paste it, but Debian has fucked up my X copy and paste.  Line 13
> of kernel/semaphore.c.
> 
> > 	__down_common(TASK_KILLABLE):
> > 
> > 			if (state == TASK_KILLABLE && fatal_signal_pending(task))
> > 				goto interrupted;
> > 
> > 			/* --- WINDOW --- */
> > 
> > 			__set_task_state(task, TASK_KILLABLE);
> > 			schedule_timeout(timeout);
> > 
> > This looks racy. If SIGKILL comes in the WINDOW above, the event is lost.
> > The task will wait for up() or timeout with the fatal signal pending, and
> > it is not possible to wakeup it via kill() again.
> 
> Hmmm.  I think you're right.  But mutex.c has the same problem, then.

and do_wait_for_common()

> > This is easy to fix, but I wonder if we should change schedule() instead.
> > Note that __down_common() does 2 checks,
> > 
> > 		if (state == TASK_INTERRUPTIBLE && signal_pending(task))
> > 			goto interrupted;
> > 		if (state == TASK_KILLABLE && fatal_signal_pending(task))
> > 			goto interrupted;
> > 
> > they look very symmetrical, but the first one is OK, and the second is racy.
> 
> Oh, because of the special casing in sched.c.  Why not just move the
> __set_task_state before the checks for signals pending?

Yes sure, this all is fixeable (we need set_task_state() of course).

But please compare

	current->state = TASK_INTERRUPTIBLE;
	schedule();
and
	current->state = TASK_KILLABLE;
	schedule();

it seems to me that it is not nice they behave "differently".

> > Also, I think we have the similar issues with lock_page_killable().
>
> I don't think so because __wait_on_bit_lock sets the state before
> checking the 'action' (sync_page_killable).

You are right.

> > 	int signal_pending_state(struct task_struct *tsk)
> > 	{
> > 		if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
> > 			return 0;
> > 		if (signal_pending(tsk))
> > 			return 0;
> > 
> > 		return (state & TASK_INTERRUPTIBLE) ||
> > 			__fatal_signal_pending(tsk);
> > 	}
> > 
> > now,
> > 
> > 	--- kernel/sched.c
> > 	+++ kernel/sched.c
> > 	@@ -4510,8 +4510,7 @@ need_resched_nonpreemptible:
> > 		clear_tsk_need_resched(prev);
> > 	 
> > 		if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
> > 	-		if (unlikely((prev->state & TASK_INTERRUPTIBLE) &&
> > 	-				signal_pending(prev))) {
> > 	+		if (unlikely(signal_pending_state(prev))) {
> > 				prev->state = TASK_RUNNING;
> > 			} else {
> > 				deactivate_task(rq, prev, 1);
> > 
> > Thoughts?
> 
> That might be worth doing anyway, but I'd leave that up to Ingo.

Yes, we need Ingo's opinion ;)

Oleg.