linux-kernel - Re: [tip:locking/core] locking/pvqspinlock: Replace xchg() by the more descriptive set

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: <20150512084529.GC21418@twins.programming.kicks-ass.net>
Date:	Tue, 12 May 2015 10:45:29 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Douglas Hatch <doug.hatch@...com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	Waiman Long <Waiman.Long@...com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"Norton, Scott J" <scott.norton@...com>,
	Peter Anvin <hpa@...or.com>,
	"linux-tip-commits@...r.kernel.org" 
	<linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:locking/core] locking/pvqspinlock: Replace xchg() by the
 more descriptive set_mb()

On Mon, May 11, 2015 at 10:50:42AM -0700, Linus Torvalds wrote:
> On Mon, May 11, 2015 at 7:54 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > Hmm, so I looked at the set_mb() definitions and I figure we want to do
> > something like the below, right?
> 
> I don't think you need to do this for the non-smp cases. 

Well, its the store tearing thing again, we use WRITE_ONCE() in
smp_store_release() for the same reason. We want it to be a single
store.

> The whole
> thing is about smp memory ordering, so on UP you don't even need the
> WRITE_ONCE(), much less a barrier.

No, we actually need both still on UP.

Imagine the following sequence:

	for (;;) {
		set_current_state(TASK_KILLABLE);
		if (cond)
			break;

		schedule();
	}
	__set_current_state(TASK_RUNNING);

vs

	<IRQ>
		  wake_up_process(p);

As we know, set_current_state() is set_mb(), and thus will look like:

	current->state = TASK_KILLABLE;
	smp_mb();
	if (cond)
		break;

So without the WRITE_ONCE() we can get store tearing, and suppose our
compiler is insane and translates the store into 4 byte stores.

	current->state[0] = TASK_UNINTERRUPTIBLE;
	current->state[1] = TASK_WAKEKILL >> 8;
	current->state[2] = 0;
	current->state[3] = 0;

The obvious fail here is to get the wakeup interrupt between [0] and
[1].

	current->state[0] = TASK_UNINTERRUPTIBLE;

	<IRQ>
		wake_up_process(p);
		p->state = TASK_RUNNING;

	current->state[1] = TASK_WAKEKILL >> 8;
	current->state[2] = 0;
	current->state[3] = 0;

With the end result that ->state == TASK_WAKEKILL, from which we'll not
wake up unless killed.

Similarly, without the barrier(), our friendly compiler is allowed to
do:

	if (cond)
		break
	current->state = TASK_KILLABLE;
	schedule();

Which we all know to be broken.

So no, set_mb() (or smp_store_mb()) very much does need the WRITE_ONCE()
and a barrier() on UP.
--
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/