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
| ||
|
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/
Powered by blists - more mailing lists