linux-kernel - Re: PATCH? hrtimer_wakeup: fix a theoretical race wrt rt_mutex

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.58.0611060720440.14553@gandalf.stny.rr.com>
Date:	Mon, 6 Nov 2006 07:26:55 -0500 (EST)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Oleg Nesterov <oleg@...sign.ru>
cc:	Linus Torvalds <torvalds@...l.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...l.org>, linux-kernel@...r.kernel.org
Subject: Re: PATCH? hrtimer_wakeup: fix a theoretical race wrt rt_mutex_slowlock()

On Mon, 6 Nov 2006, Oleg Nesterov wrote:

> On 11/05, Linus Torvalds wrote:
> >
> > Yeah, that seems a real bug. You _always_ need to actually do the thing
> > that you wait for _before_ you want it up. That's what all the scheduling
> > primitives depend on - you can't wake people up first, and then set the
> > condition variable.
> >
> > So if a rt_mutex depeds on something that is set inside the rq-lock, it
> > needs to get the task rw-lock in order to check it.
>
> No, rt_mutex is fine (I think).

I agree.  The problem isn't with rt_mutex.  It rt_mutex doesn't depend on
that condition to break out of the loop. That condition is an extra
condition that's there to stop in the case we timed out before the owner
released the lock. The real condition to break out of the loop is the
owner releasing the lock, and there's no known races there.

So the time out is what needs to make sure that rt_mutex sees the event.

>
> My changelog was very unclean and confusing, I'll try again. What we are
> doing is:
>
> 	rt_mutex_slowlock:
>
> 		task->state = TASK_INTERRUPTIBLE;
>
> 		mb();
>
> 		if (CONDITION)
> 			return -ETIMEDOUT;
>
> 		schedule();
>
> This is common and correct.
>
> 	hrtimer_wakeup:
>
> 		CONDITION = 1;			// [1]

Right! The changing of the condition isn't even in any spin lock. But that
condition needs be be completed before the changes done within the spin
lock, but unfortunately, the spin lock itself doesn't guarantee anything.

>
> 		spin_lock(rq->lock);
>
> 		task->state = TASK_RUNNING;	// [2]
>
> This needs 'wmb()' between [1] and [2] unless spin_lock() garantees memory
> ordering. Of course, rt_mutex can take rq->lock to solve this, but I don't
> think it would be right.

Correct, the solution should NOT change rt_mutex.  Your original patch is
the correct approach.

-- Steve

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