[PATCH] Avoid dead lock of console related locks in panic case

[PATCH] Avoid dead lock of console related locks in panic case

[Seiji Aguchi]

[Issue]

If one cpu ,which is taking a logbuf_lock or console_sem, 
receive IPI/NMI from a panicked cpu via smp_send_stop(),
the panicked cpu hangs up in subsequent kmsg_dump()/printk()
because logbuf_lock and console_sem are taken in the function calls.

This causes a console blank and users can't see panic messages.

[Solution]

this patch introduces a logic initializing logbuf_lock and console_sem
just after smp_send_stop() to avoid dead locks above.

Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
---
 include/linux/printk.h |    5 +++++
 kernel/panic.c         |    1 +
 kernel/printk.c        |   17 +++++++++++++++++
 lib/bust_spinlocks.c   |   21 ++++++++++++++++++---
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9afc01e..ffd3e55 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -139,6 +139,7 @@ extern int kptr_restrict;
 
 void log_buf_kexec_setup(void);
 void __init setup_log_buf(int early);
+void zap_locks_force(void);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -172,6 +173,10 @@ static inline void log_buf_kexec_setup(void)
 static inline void setup_log_buf(int early)
 {
 }
+
+static inline void zap_locks_force(void)
+{
+}
 #endif
 
 extern void dump_stack(void) __cold;
diff --git a/kernel/panic.c b/kernel/panic.c
index e1b2822..c27e58e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -122,6 +122,7 @@ void panic(const char *fmt, ...)
 	 * situation.
 	 */
 	smp_send_stop();
+	bust_spinlocks(2);
 
 	kmsg_dump(KMSG_DUMP_PANIC);
 
diff --git a/kernel/printk.c b/kernel/printk.c
index 2d607f4..a6fa638 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1278,6 +1278,23 @@ static void call_console_drivers(int level, const char *text, size_t len)
 }
 
 /*
+ * Zap console related locks in panic situation.
+ * It should be called by bust_spinlocks().
+ */
+void zap_locks_force(void)
+{
+	debug_locks_off();
+
+	/* If a crash is occurring, make sure we can't deadlock */
+	if (raw_spin_is_locked(&logbuf_lock))
+		raw_spin_lock_init(&logbuf_lock);
+
+	/* And make sure that we print immediately */
+	if (is_console_locked())
+		sema_init(&console_sem, 1);
+}
+
+/*
  * Zap console related locks when oopsing. Only zap at most once
  * every 10 seconds, to leave time for slow consoles to print a
  * full oops.
diff --git a/lib/bust_spinlocks.c b/lib/bust_spinlocks.c
index 9681d54..ffa1c64 100644
--- a/lib/bust_spinlocks.c
+++ b/lib/bust_spinlocks.c
@@ -13,19 +13,34 @@
 #include <linux/wait.h>
 #include <linux/vt_kern.h>
 #include <linux/console.h>
+#include <linux/printk.h>
 
 
+/*
+ * yes = 0: make sure that messages are printed to console immediately.
+ * yes = 1: avoid a recursive lock on a single cpu in a panic case.
+ * yes = 2: avoid a deadlock after stopping cpus by smp_send_stop()
+ *          in a panic case.
+ */
 void __attribute__((weak)) bust_spinlocks(int yes)
 {
-	if (yes) {
-		++oops_in_progress;
-	} else {
+	switch (yes) {
+	case 0:
 #ifdef CONFIG_VT
 		unblank_screen();
 #endif
 		console_unblank();
 		if (--oops_in_progress == 0)
 			wake_up_klogd();
+		break;
+	case 1:
+		++oops_in_progress;
+		break;
+	case 2:
+		zap_locks_force();
+		break;
+	default:
+		break;
 	}
 }
 
-- 
1.7.1

Re: [PATCH] Avoid dead lock of console related locks in panic case

[Andrew Morton]

On Fri, 30 Nov 2012 17:11:07 +0000
Seiji Aguchi <seiji.aguchi@hds.com> wrote:

> If one cpu ,which is taking a logbuf_lock or console_sem, 
> receive IPI/NMI from a panicked cpu via smp_send_stop(),
> the panicked cpu hangs up in subsequent kmsg_dump()/printk()
> because logbuf_lock and console_sem are taken in the function calls.
> 
> This causes a console blank and users can't see panic messages.
> 
> [Solution]
> 
> this patch introduces a logic initializing logbuf_lock and console_sem
> just after smp_send_stop() to avoid dead locks above.

That is one nasty looking patch :(

- Makes the logic in this area even more twisty and complex, when
  what we need to do is to simplify it

- Reinitialises in-use locks

- Gives the boolean variable "yes" three states, but didn't rename
  that variable to something appropriate.

- Passes yes==2 into s390's unsuspecting bust_spinlocks() implementation.


Let's step back a bit.  Please identify with great specificity the code
sites which are stopping other CPUs before taking locks which those
other CPUs might have been holding.

Then let's see what we can do to fix up the callers, instead of trying
to tidy up after they have made this mess.

RE: [PATCH] Avoid dead lock of console related locks in panic case

[Seiji Aguchi]

Thank you  for giving me the comment.

> - Makes the logic in this area even more twisty and complex, when
>   what we need to do is to simplify it
> 
> - Reinitialises in-use locks
> 
> - Gives the boolean variable "yes" three states, but didn't rename
>   that variable to something appropriate.

I understand "yes" is odd.
I just wanted to know if an idea reinitializing locks is acceptable.
But now I understand I have to take another approach.

> 
> - Passes yes==2 into s390's unsuspecting bust_spinlocks() implementation.
>

Sorry. I missed the code.
 
> 
> Let's step back a bit.  Please identify with great specificity the code sites which are stopping other CPUs before taking locks which
> those other CPUs might have been holding.
> 
> Then let's see what we can do to fix up the callers, instead of trying to tidy up after they have made this mess.

OK.
I will update my patch without adding complexity.
The logic will be as follows, if I understand your comment correctly.

 - take console related locks (logbuf_lock, console_sem)
 - stop other cpus
 - release those locks

Seiji

Re: [PATCH] Avoid dead lock of console related locks in panic case

[Andrew Morton]

On Fri, 30 Nov 2012 22:59:13 +0000
Seiji Aguchi <seiji.aguchi@hds.com> wrote:

> > 
> > Let's step back a bit.  Please identify with great specificity the code sites which are stopping other CPUs before taking locks which
> > those other CPUs might have been holding.
> > 
> > Then let's see what we can do to fix up the callers, instead of trying to tidy up after they have made this mess.
> 
> OK.
> I will update my patch without adding complexity.
> The logic will be as follows, if I understand your comment correctly.
> 
>  - take console related locks (logbuf_lock, console_sem)
>  - stop other cpus
>  - release those locks

That would be one way around the problem.  It's still a bit messy,
because we'll have to take more and more locks in the future, as we
identify them.

What I actually meant was: can "this" CPU avoid stopping other CPUs so
early?  If we stop the other CPUs when this CPU is ready to stop itself
then there will never be such deadlocks.

RE: [PATCH] Avoid dead lock of console related locks in panic case

[Seiji Aguchi]

> What I actually meant was: can "this" CPU avoid stopping other CPUs so early?  If we stop the other CPUs when this CPU is ready to
> stop itself then there will never be such deadlocks.

Let me explain my opinion.
When we focus on the deadlock only,   the code will be simple by moving smp_send_stop() at the end of panic().

But, panic situation is not normal.
I don't think that keeping running multiple cpus is safe, because they may touch corrupted data/variables and unnecessary 
panic/BUG() may happen.

IMO, cpus should be stopped "as early as" possible when panic happens.
And then panic() has to take minimal steps with a single cpu.
 - output messages
 - kicking troubleshooting features like kdump/kmsg_dump

Seiji