printk: Use ACCESS_ONCE() instead of a volatile type

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().

Quoted 7 lines · Pranith Kumar

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

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(-)

kernel/printk/printk.c +4 -4

index e748971..4790191 100644
@@ -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);

Quoted 4 lines · 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.

Quoted 3 lines

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

Quoted 46 lines

Signed-off-by Pranith Kumar <[email protected]>

---
 kernel/printk/printk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

kernel/printk/printk.c +4 -4

index e748971..4790191 100644
@@ -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);

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(-)

kernel/printk/printk.c +0 -0

index e748971..4790191 100644

kernel/printk/printk.c +3 -3

 	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);

kernel/printk/printk.c asmlinkage int vprintk_emit(int facility, int level, · line 1627

Quoted 10 lines

kernel/printk/printk.c +1 -1

index e748971..4790191 100644
@@ -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.)

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.

Quoted 4 lines · 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

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.

QED. (Well, please correct me where I'm wrong...)

Quoted 8 lines · Steven Rostedt

Quoted 4 lines · 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.

Quoted 4 lines

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...)