[PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Sergey Senozhatsky]

printk() without oops_in_progress set is potentially dangerous.
it will attempt to call into console driver, so if oops happened
while console driver port->lock spin_lock was locked on the same
CPU (NMI oops or oops from console driver), then re-entering
console driver from bust_spinlocks() will deadlock the system.

Some serial drivers have are re-entrant from oops path:

static void serial_console_write(struct console *co, const char *s,
                                 unsigned count)
{
...
        if (port->sysrq)
                locked = 0;
        else if (oops_in_progress)
                locked = spin_trylock_irqsave(&port->lock, flags);
        else
                spin_lock_irqsave(&port->lock, flags);
...

        uart_console_write(port, s, count, serial_console_putchar);
...
        if (locked)
                spin_unlock_irqrestore(&port->lock, flags);
}

So it's OK to call printk() or console_unblank() and re-enter
serial console drivers when oops_in_progress set. But once we
clear oops_in_progress serial consoles become non-reentrant.

From the comment it seems that s390 wants to just poke klogd.
There is wake_up_klogd() for this purpose, so we can replace
that printk(" ").

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 arch/s390/mm/fault.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2b8f32f56e0c..244993dc3c70 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -93,15 +93,10 @@ void bust_spinlocks(int yes)
 	} else {
 		int loglevel_save = console_loglevel;
 		console_unblank();
-		oops_in_progress = 0;
-		/*
-		 * OK, the message is on the console.  Now we call printk()
-		 * without oops_in_progress set so that printk will give klogd
-		 * a poke.  Hold onto your hats...
-		 */
-		console_loglevel = 15;
-		printk(" ");
 		console_loglevel = loglevel_save;
+
+		oops_in_progress = 0;
+		wake_up_klogd();
 	}
 }
 
-- 
2.19.1

Re: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Sergey Senozhatsky]

On (10/24/18 13:30), Sergey Senozhatsky wrote:
> -		 * OK, the message is on the console.  Now we call printk()
> -		 * without oops_in_progress set so that printk will give klogd
> -		 * a poke.  Hold onto your hats...
> -		 */
> -		console_loglevel = 15;
> -		printk(" ");
>  		console_loglevel = loglevel_save;
> +
> +		oops_in_progress = 0;
> +		wake_up_klogd();

D'oh... Fat fingers!
I noticed that I have removed "console_loglevel = 15".
Sorry about that.

====

From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

printk() without oops_in_progress set is potentially dangerous.
it will attempt to call into console driver, so if oops happened
while console driver port->lock spin_lock was locked on the same
CPU (NMI oops or oops from console driver), then re-entering
console driver from bust_spinlocks() will deadlock the system.

Some serial drivers have are re-entrant from oops path:

static void serial_console_write(struct console *co, const char *s,
                                 unsigned count)
{
...
        if (port->sysrq)
                locked = 0;
        else if (oops_in_progress)
                locked = spin_trylock_irqsave(&port->lock, flags);
        else
                spin_lock_irqsave(&port->lock, flags);
...

        uart_console_write(port, s, count, serial_console_putchar);
...
        if (locked)
                spin_unlock_irqrestore(&port->lock, flags);
}

So it's OK to call printk() or console_unblank() and re-enter
serial console drivers when oops_in_progress set. But once we
clear oops_in_progress serial consoles become non-reentrant.

From the comment it seems that s390 wants to just poke klogd.
There is wake_up_klogd() for this purpose, so we can replace
that printk(" ").

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 arch/s390/mm/fault.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2b8f32f56e0c..53915c61ad95 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -92,16 +92,12 @@ void bust_spinlocks(int yes)
 		oops_in_progress = 1;
 	} else {
 		int loglevel_save = console_loglevel;
-		console_unblank();
-		oops_in_progress = 0;
-		/*
-		 * OK, the message is on the console.  Now we call printk()
-		 * without oops_in_progress set so that printk will give klogd
-		 * a poke.  Hold onto your hats...
-		 */
+
 		console_loglevel = 15;
-		printk(" ");
+		console_unblank();
 		console_loglevel = loglevel_save;
+		oops_in_progress = 0;
+		wake_up_klogd();
 	}
 }
 
-- 
2.19.1

Re: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Heiko Carstens]

On Wed, Oct 24, 2018 at 01:34:25PM +0900, Sergey Senozhatsky wrote:
> On (10/24/18 13:30), Sergey Senozhatsky wrote:
> From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Subject: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

...

> From the comment it seems that s390 wants to just poke klogd.
> There is wake_up_klogd() for this purpose, so we can replace
> that printk(" ").
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  arch/s390/mm/fault.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 2b8f32f56e0c..53915c61ad95 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -92,16 +92,12 @@ void bust_spinlocks(int yes)
>  		oops_in_progress = 1;
>  	} else {
>  		int loglevel_save = console_loglevel;
> -		console_unblank();
> -		oops_in_progress = 0;
> -		/*
> -		 * OK, the message is on the console.  Now we call printk()
> -		 * without oops_in_progress set so that printk will give klogd
> -		 * a poke.  Hold onto your hats...
> -		 */
> +
>  		console_loglevel = 15;
> -		printk(" ");
> +		console_unblank();
>  		console_loglevel = loglevel_save;
> +		oops_in_progress = 0;
> +		wake_up_klogd();
>  	}

With your patch this looks nearly like the common code variant. I did
some code archaeology and this function is unchanged since ~17 years.
When it was introduced it was close to identical to the x86 variant.
All other architectures use the common code variant in the
meantime. So if we change this I'd prefer that we switch s390 to the
common code variant as well.

Right now I can't see a reason for not doing that, but I might be
wrong of course. So, could you please provide a new version which just
removes this variant and makes s390 use the generic one too.

We'll see if there is any fallout...

Re: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Sergey Senozhatsky]

On (10/25/18 08:28), Heiko Carstens wrote:
[..]
> >  		int loglevel_save = console_loglevel;
> > -		console_unblank();
> > -		oops_in_progress = 0;
> > -		/*
> > -		 * OK, the message is on the console.  Now we call printk()
> > -		 * without oops_in_progress set so that printk will give klogd
> > -		 * a poke.  Hold onto your hats...
> > -		 */
> > +
> >  		console_loglevel = 15;
> > -		printk(" ");
> > +		console_unblank();
> >  		console_loglevel = loglevel_save;
> > +		oops_in_progress = 0;
> > +		wake_up_klogd();
> >  	}
> 
> With your patch this looks nearly like the common code variant. I did
> some code archaeology and this function is unchanged since ~17 years.
> When it was introduced it was close to identical to the x86 variant.
> All other architectures use the common code variant in the
> meantime. So if we change this I'd prefer that we switch s390 to the
> common code variant as well.

Right. I couldn't clearly understand what was so special that s390
bust_spinlock() was doing, but assumed that this `console_loglevel'
manipulation probably was somehow important to s390. Though this
console_loglevel adjustment is not 100% guaranteed to make any difference,
because of the way console_unblank() works: if it can't lock console_sem
and it sees oops_in_progress then it does nothing; it doesn't even print
logbuf messages to the consoles. If, however, console_sem is not locked,
then it does print pending logbuf messages, with temporarily verbose
console_loglevel. I concluded that this might be important to you in
one way or another.

> Right now I can't see a reason for not doing that, but I might be
> wrong of course. So, could you please provide a new version which just
> removes this variant and makes s390 use the generic one too.
> 
> We'll see if there is any fallout...

Will do! Sounds good.

	-ss

Re: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Sergey Senozhatsky]

On (10/25/18 08:28), Heiko Carstens wrote:
> 
> With your patch this looks nearly like the common code variant. I did
> some code archaeology and this function is unchanged since ~17 years.
> When it was introduced it was close to identical to the x86 variant.
> All other architectures use the common code variant in the
> meantime. So if we change this I'd prefer that we switch s390 to the
> common code variant as well.
> 
> Right now I can't see a reason for not doing that, but I might be
> wrong of course. So, could you please provide a new version which just
> removes this variant and makes s390 use the generic one too.
> 
> We'll see if there is any fallout...

Heiko, if this one works for you then I'll submit a format patch.
Let me know. Thanks.

====

From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: [PATCH] arch/s390: use common bust_spinlocks()

s390 is the only architecture that is using own bust_spinlocks()
variant, while other arch-s seem to be OK with the common
implementation.

Heiko Carstens [1] said he would prefer s390 to use the common
bust_spinlocks() as well:
  I did some code archaeology and this function is unchanged since ~17
  years. When it was introduced it was close to identical to the x86
  variant. All other architectures use the common code variant in the
  meantime. So if we change this I'd prefer that we switch s390 to the
  common code variant as well. Right now I can't see a reason for not
  doing that

This patch removes s390 bust_spinlocks() and drops the weak attribute
from the common bust_spinlocks() version.

[1] lkml.kernel.org/r/20181025062800.GB4037@osiris
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 arch/s390/mm/fault.c | 24 ------------------------
 lib/bust_spinlocks.c |  6 +++---
 2 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2b8f32f56e0c..11613362c4e7 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -81,30 +81,6 @@ static inline int notify_page_fault(struct pt_regs *regs)
 	return ret;
 }
 
-
-/*
- * Unlock any spinlocks which will prevent us from getting the
- * message out.
- */
-void bust_spinlocks(int yes)
-{
-	if (yes) {
-		oops_in_progress = 1;
-	} else {
-		int loglevel_save = console_loglevel;
-		console_unblank();
-		oops_in_progress = 0;
-		/*
-		 * OK, the message is on the console.  Now we call printk()
-		 * without oops_in_progress set so that printk will give klogd
-		 * a poke.  Hold onto your hats...
-		 */
-		console_loglevel = 15;
-		printk(" ");
-		console_loglevel = loglevel_save;
-	}
-}
-
 /*
  * Find out which address space caused the exception.
  * Access register mode is impossible, ignore space == 3.
diff --git a/lib/bust_spinlocks.c b/lib/bust_spinlocks.c
index ab719495e2cb..8be59f84eaea 100644
--- a/lib/bust_spinlocks.c
+++ b/lib/bust_spinlocks.c
@@ -2,7 +2,8 @@
 /*
  * lib/bust_spinlocks.c
  *
- * Provides a minimal bust_spinlocks for architectures which don't have one of their own.
+ * Provides a minimal bust_spinlocks for architectures which don't
+ * have one of their own.
  *
  * bust_spinlocks() clears any spinlocks which would prevent oops, die(), BUG()
  * and panic() information from reaching the user.
@@ -16,8 +17,7 @@
 #include <linux/vt_kern.h>
 #include <linux/console.h>
 
-
-void __attribute__((weak)) bust_spinlocks(int yes)
+void bust_spinlocks(int yes)
 {
 	if (yes) {
 		++oops_in_progress;
-- 
2.19.1

Re: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Heiko Carstens]

On Thu, Oct 25, 2018 at 04:05:43PM +0900, Sergey Senozhatsky wrote:
> On (10/25/18 08:28), Heiko Carstens wrote:
> > 
> > With your patch this looks nearly like the common code variant. I did
> > some code archaeology and this function is unchanged since ~17 years.
> > When it was introduced it was close to identical to the x86 variant.
> > All other architectures use the common code variant in the
> > meantime. So if we change this I'd prefer that we switch s390 to the
> > common code variant as well.
> > 
> > Right now I can't see a reason for not doing that, but I might be
> > wrong of course. So, could you please provide a new version which just
> > removes this variant and makes s390 use the generic one too.
> > 
> > We'll see if there is any fallout...
> 
> Heiko, if this one works for you then I'll submit a format patch.
> Let me know. Thanks.
> 
> ====
> 
> From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Subject: [PATCH] arch/s390: use common bust_spinlocks()
> 
> s390 is the only architecture that is using own bust_spinlocks()
> variant, while other arch-s seem to be OK with the common
> implementation.
> 
> Heiko Carstens [1] said he would prefer s390 to use the common
> bust_spinlocks() as well:
>   I did some code archaeology and this function is unchanged since ~17
>   years. When it was introduced it was close to identical to the x86
>   variant. All other architectures use the common code variant in the
>   meantime. So if we change this I'd prefer that we switch s390 to the
>   common code variant as well. Right now I can't see a reason for not
>   doing that
> 
> This patch removes s390 bust_spinlocks() and drops the weak attribute
> from the common bust_spinlocks() version.
> 
> [1] lkml.kernel.org/r/20181025062800.GB4037@osiris
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  arch/s390/mm/fault.c | 24 ------------------------
>  lib/bust_spinlocks.c |  6 +++---
>  2 files changed, 3 insertions(+), 27 deletions(-)

I gave this some testing and forced panic/die in interrupt as well as
process context with different consoles as well as single and multi
cpu systems. Everything still seems to work.

So I'm applying this to our internal queue first. It will hit upstream
latest in the next merge window if there aren't any issues found.

Thanks!

Re: [PATCH] s390/fault: use wake_up_klogd() in bust_spinlocks()

[Sergey Senozhatsky]

On (10/25/18 10:11), Heiko Carstens wrote:
> > s390 is the only architecture that is using own bust_spinlocks()
> > variant, while other arch-s seem to be OK with the common
> > implementation.
> > 
> > Heiko Carstens [1] said he would prefer s390 to use the common
> > bust_spinlocks() as well:
> >   I did some code archaeology and this function is unchanged since ~17
> >   years. When it was introduced it was close to identical to the x86
> >   variant. All other architectures use the common code variant in the
> >   meantime. So if we change this I'd prefer that we switch s390 to the
> >   common code variant as well. Right now I can't see a reason for not
> >   doing that
> > 
> > This patch removes s390 bust_spinlocks() and drops the weak attribute
> > from the common bust_spinlocks() version.
> > 
> > [1] lkml.kernel.org/r/20181025062800.GB4037@osiris
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  arch/s390/mm/fault.c | 24 ------------------------
> >  lib/bust_spinlocks.c |  6 +++---
> >  2 files changed, 3 insertions(+), 27 deletions(-)
> 
> I gave this some testing and forced panic/die in interrupt as well as
> process context with different consoles as well as single and multi
> cpu systems. Everything still seems to work.

That was quick ;) Thanks.

> So I'm applying this to our internal queue first. It will hit upstream
> latest in the next merge window if there aren't any issues found.

Sure; sounds like a plan.

	-ss