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: <CAJhHMCCWaQNVFuSUvpbcR=CMwxW6ownZ7W=NrXaiYjRvOQ1WDg@mail.gmail.com>
Date:	Thu, 13 Nov 2014 23:02:07 -0500
From:	Pranith Kumar <bobby.prani@...il.com>
To:	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>,
	Alex Elder <elder@...aro.org>, Joe Perches <joe@...ches.com>,
	open list <linux-kernel@...r.kernel.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>
Subject: Re: [RFC PATCH] printk: Use ACCESS_ONCE() instead of a volatile type

On Thu, Nov 13, 2014 at 10:47 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> On Thu, 13 Nov 2014 22:21:21 -0500
> Pranith Kumar <bobby.prani@...il.com> 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().
>
> I'm a little confused by the above paragraph about it's not clear how
> it is being accessed concurrently. Do you mean the code is unclear, or
> your understanding of it is unclear?

It is definitely got to do with my understanding. recursion explains
how it can be concurrently accessed. Thanks!

>
> Regardless of your answer, this patch is correct. logbuf_cpu is used to
> determine if printk has recursed on itself before it takes the
> logbuf_lock and deadlocks. Your ACCESS_ONCE keeps the compiler from
> optimizing out the logbuf_cpu as it will see that this function is the
> only one that can touch it and may try to do weird things to it.
>
> Reviewed-by: Steven Rostedt <rostedt@...dmis.org>
>
> -- Steve
>
>
>>
>> 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 (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);
>



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