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

Message-ID: <546589A1.9040100@linaro.org>
Date:	Thu, 13 Nov 2014 22:48:33 -0600
From:	Alex Elder <elder@...aro.org>
To:	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>,
	Steven Rostedt <rostedt@...dmis.org>,
	Joe Perches <joe@...ches.com>,
	open list <linux-kernel@...r.kernel.org>
CC:	paulmck@...ux.vnet.ibm.com
Subject: Re: [RFC PATCH] printk: Use ACCESS_ONCE() instead of a volatile type

On 11/13/2014 09:21 PM, Pranith Kumar wrote:
> Remove volatile type qualifier and use ACCESS_ONCE() in its place for each
> access. Using volatile is not recommended as documented in
> Documentation/volatile-considered-harmful.txt.
> 
> Here logbuf_cpu is a local variable and it is not clear how it is being accessed
> concurrently. We should remove volatile accesses entirely here, but for now make
> a safer change of using ACCESS_ONCE().

Although logbuf_cpu is declared locally, it has static scope and
hence its value is persistent across calls to the function,
including concurrent calls on different CPUs.

This is a very interesting bit of code.  I have a
question, below.

					-Alex

> 
> Signed-off-by: Pranith Kumar <bobby.prani@...il.com>
> ---
>  kernel/printk/printk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index e748971..4790191 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1624,7 +1624,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	int printed_len = 0;
>  	bool in_sched = false;
>  	/* cpu currently holding logbuf_lock in this function */
> -	static volatile unsigned int logbuf_cpu = UINT_MAX;
> +	static unsigned int logbuf_cpu = UINT_MAX;

If this is not volatile, can the compiler assume that it
can't change before the first access?  Put another way,
does this assignment need to be done more like this?

	static unsigned int ACCESS_ONCE(logbuf_cpu) = UINT_MAX;

(I haven't checked, but I don't believe that expands to valid code.)

>  	if (level == LOGLEVEL_SCHED) {
>  		level = LOGLEVEL_DEFAULT;
> @@ -1641,7 +1641,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	/*
>  	 * Ouch, printk recursed into itself!
>  	 */
> -	if (unlikely(logbuf_cpu == this_cpu)) {
> +	if (unlikely(ACCESS_ONCE(logbuf_cpu) == this_cpu)) {
>  		/*
>  		 * If a crash is occurring during printk() on this CPU,
>  		 * then try to get the crash message out but make sure
> @@ -1659,7 +1659,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  
>  	lockdep_off();
>  	raw_spin_lock(&logbuf_lock);
> -	logbuf_cpu = this_cpu;
> +	ACCESS_ONCE(logbuf_cpu) = this_cpu;
>  
>  	if (unlikely(recursion_bug)) {
>  		static const char recursion_msg[] =
> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  						 dict, dictlen, text, text_len);
>  	}
>  
> -	logbuf_cpu = UINT_MAX;
> +	ACCESS_ONCE(logbuf_cpu) = UINT_MAX;
>  	raw_spin_unlock(&logbuf_lock);
>  	lockdep_on();
>  	local_irq_restore(flags);
> 

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