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: Fri, 14 Nov 2014 10:39:34 -0600 From: Alex Elder <elder@...aro.org> To: Steven Rostedt <rostedt@...dmis.org> CC: Pranith Kumar <bobby.prani@...il.com>, Andrew Morton <akpm@...ux-foundation.org>, Petr Mladek <pmladek@...e.cz>, Jan Kara <jack@...e.cz>, "Luis R. Rodriguez" <mcgrof@...e.com>, Joe Perches <joe@...ches.com>, open list <linux-kernel@...r.kernel.org>, paulmck@...ux.vnet.ibm.com Subject: Re: [RFC PATCH] printk: Use ACCESS_ONCE() instead of a volatile type On 11/13/2014 11:24 PM, Steven Rostedt wrote: > On Thu, 13 Nov 2014 23:57:22 -0500 > Steven Rostedt <rostedt@...dmis.org> wrote: > >> That assignment is what it is initialized to at boot up. I can't see >> any optimization that would cause gcc to modify that. Especially since >> we are hiding its accesses within the ACCESS_ONCE(). That alone should >> confuse gcc enough to leave it a hell alone J. > > > I'm actually wondering if the ACCESS_ONCE or volatile is even needed. > > static variables are used to maintain state, and that goes for > recursive functions. gcc should not touch it. I think you're right. Here's some extra analysis. I may be wrong on a detail or two but see if it makes sense. The logbuf_cpu variable has static storage duration, so will be initialized before program startup. This function (vprintk_emit()) can be called on multiple CPUs concurrently. So we can assume that there is more than one thread executing in window from the start of the function until the raw_spin_lock(&logbuf_lock) call is made. The only writes to logbuf_lock are made under protection of the spinlock. It is initially UINT_MAX; it is changed to the current processor id right after taking the lock; and it is reverted to UINT_MAX right before releasing the lock. So logbuf_cpu will either contain UINT_MAX, or will hold the processor id of the CPU that is holding logbuf_lock. The spinlock barrier ensures that the only value a CPU will see is UINT_MAX, unless it is the CPU that holds the spinlock. There is only one read of logbuf_cpu: if (unlikely(logbuf_cpu == this_cpu)) { This is called only while local interrupts are disabled, so if this condition holds it cannot be due to an interrupt--it must be due to simple recursion into printk() while inside the spinlock-protected critical section. We *can* recurse into printk() via a function call within the protected section--through vscnprintf(), which can descend into printk() via WARN() calls in format_decode(). (There may be others after that point, but up to there it looks like no other function call in that section can fail.) So it *is* possible to hit this recursion (I wanted to verify that...). OK. So back to the original issue... How do we ensure the value of logbuf_cpu is in fact the last set value, and is not affected by any compiler reordering? If its value is anything other than UINT_MAX, it will be the current CPU's processor id, which will have been set by the current CPU. There are no issues related to caches or barriers. Since vprintk_emit() is a public entry point there's no magic inter-function optimization or inlining that could allow the value of the static logbuf_cpu to be preserved between calls. So the first read of logbuf_cpu in a given function call will have to fetch its current value from memory (regardless of whether there's a "volatile" qualifier). And therefore the one read of that value will involve fetching the "real" value from memory, and it will either be UINT_MAX or the CPU's own processor id. So there should be no need to declare the variable volatile, nor to access it with ACCESS_ONCE(). QED. (Well, please correct me where I'm wrong...) -Alex > Now perhaps it can see that there is no recursion for logbuf_cpu to be > set to the current cpu (which would be interesting since the > smp_processor_id() call is also hidden from gcc), and it might optimize > it out. But that would not protect us from NMIs doing a printk(). > Although this code doesn't protect us from that anyway if an NMI were > to come in right after taking the logbuf_lock and before setting > logbuf_cpu. In that case, logbuf_cpu will not be set to this_cpu and a > deadlock can still occur. This code only makes the race window smaller. > > I'm thinking the correct change is to nuke all of it. Perhaps the only > reason using volatile here was not a bug is because volatile wasn't > needed in the first place! > > -- Steve > -- 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