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().
Signed-off-by: Pranith Kumar <[email protected]>
---
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);
--
1.9.1
On Thu, 2014-11-13 at 22:21 -0500, 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().
Not recommended does not mean "don't ever use".
Forcing the volatile at each use site instead
of the declaration isn't necessarily better.
I think the code is more readable as-is but I'm
not going to object if Andrew picks this up...
On Thu, 13 Nov 2014 22:21:21 -0500
Pranith Kumar <[email protected]> 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?
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 <[email protected]>
-- Steve
>
> Signed-off-by: Pranith Kumar <[email protected]>
> ---
> 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);
On Thu, 13 Nov 2014 19:41:00 -0800
Joe Perches <[email protected]> wrote:
> On Thu, 2014-11-13 at 22:21 -0500, 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().
>
> Not recommended does not mean "don't ever use".
I would argue that the use of volatile in open code is evil and prone
to bugs. I agree that this is one of the few occasions that this is not
the case.
>
> Forcing the volatile at each use site instead
> of the declaration isn't necessarily better.
>
> I think the code is more readable as-is but I'm
> not going to object if Andrew picks this up...
>
The ACCESS_ONCE() calls at each location makes it a bit uglier, but it
drives in the point of what that is doing. Where as the volatile may be
missed.
-- Steve
On Thu, Nov 13, 2014 at 10:47 PM, Steven Rostedt <[email protected]> wrote:
> On Thu, 13 Nov 2014 22:21:21 -0500
> Pranith Kumar <[email protected]> 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 <[email protected]>
>
> -- Steve
>
>
>>
>> Signed-off-by: Pranith Kumar <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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);
>
On Thu, 13 Nov 2014 22:48:33 -0600
Alex Elder <[email protected]> wrote:
> > 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.)
>
I can bet you that it doesn't compile.
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.
-- Steve
On Thu, 13 Nov 2014 23:57:22 -0500
Steven Rostedt <[email protected]> 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.
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
On 11/13/2014 11:24 PM, Steven Rostedt wrote:
> On Thu, 13 Nov 2014 23:57:22 -0500
> Steven Rostedt <[email protected]> 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
>
On Fri, 14 Nov 2014 10:39:34 -0600
Alex Elder <[email protected]> wrote:
> 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
Unless an NMI called printk.
> must be due to simple recursion into printk() while inside
> the spinlock-protected critical section.
> QED. (Well, please correct me where I'm wrong...)
>
Except for the NMI case, I believe you are correct with the rest of
your analysis.
-- Steve
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 <[email protected]> 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?