linux-kernel - Q: down_killable() is racy? or schedule() is not right?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080603123309.GA472@tv-sign.ru>
Date:	Tue, 3 Jun 2008 16:33:09 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Ingo Molnar <mingo@...e.hu>, Matthew Wilcox <matthew@....cx>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	linux-kernel@...r.kernel.org
Subject: Q: down_killable() is racy? or schedule() is not right?

I just noticed we have generic semaphores, a couple of questions.

	down():

		spin_lock_irqsave(&sem->lock, flags);
		...
		__down(sem);

Why _irqsave ? we must not do down() with irqs disabled, and of course
__down() restores/clears irqs unconditionally.


Another question,

	__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.

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.
Also, I think we have the similar issues with lock_page_killable().

How about something like

	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?

Oleg.

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