linux-kernel - Re: [RFC PATCH] printk: Use ACCESS_ONCE() instead of a volatile type

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

Date:	Fri, 14 Nov 2014 13:23:27 -0500
From:	Pranith Kumar <bobby.prani@...il.com>
To:	Alex Elder <elder@...aro.org>, Steven Rostedt <rostedt@...dmis.org>
CC:	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/14/2014 11:39 AM, Alex Elder wrote:
> 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...)
>

Thanks Alex, for the in-depth analysis. Please drop my patch in favour of removing volatile and without ACCESS_ONCE(). Will you send in such a patch?
--
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/