RE: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI

[æåèå / KAWAIïHIDEHIRO]

Hi,

> On Fri, Nov 20, 2015 at 06:36:44PM +0900, Hidehiro Kawai wrote:
> > If panic on NMI happens just after panic() on the same CPU, panic()
> > is recursively called.  As the result, it stalls after failing to
> > acquire panic_lock.
> >
> > To avoid this problem, don't call panic() in NMI context if
> > we've already entered panic().
> >
> > V4:
> > - Improve comments in io_check_error() and panic()
> >
> > V3:
> > - Introduce nmi_panic() macro to reduce code duplication
> > - In the case of panic on NMI, don't return from NMI handlers
> >   if another cpu already panicked
> >
> > V2:
> > - Use atomic_cmpxchg() instead of current spin_trylock() to
> >   exclude concurrent accesses to the panic routines
> > - Don't introduce no-lock version of panic()
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@xxxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> > ---
> >  arch/x86/kernel/nmi.c  |   16 ++++++++++++----
> >  include/linux/kernel.h |   13 +++++++++++++
> >  kernel/panic.c         |   15 ++++++++++++---
> >  kernel/watchdog.c      |    2 +-
> >  4 files changed, 38 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> > index 697f90d..5131714 100644
> > --- a/arch/x86/kernel/nmi.c
> > +++ b/arch/x86/kernel/nmi.c
> > @@ -231,7 +231,7 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
> >  #endif
> >
> >  	if (panic_on_unrecovered_nmi)
> > -		panic("NMI: Not continuing");
> > +		nmi_panic("NMI: Not continuing");
> >
> >  	pr_emerg("Dazed and confused, but trying to continue\n");
> >
> > @@ -255,8 +255,16 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
> >  		 reason, smp_processor_id());
> >  	show_regs(regs);
> >
> > -	if (panic_on_io_nmi)
> > -		panic("NMI IOCK error: Not continuing");
> > +	if (panic_on_io_nmi) {
> > +		nmi_panic("NMI IOCK error: Not continuing");
> 
> Btw, that panic_on_io_nmi seems undocumented in
> Documentation/sysctl/kernel.txt. Care to document it, please, as a
> separate patch?

Sure. I'll post a documentation patch for it in a separate patch.
Because panic_on_io_nmi has been available since relatively old days,
I didn't check it. 

> > +
> > +		/*
> > +		 * If we return from nmi_panic(), it means we have received
> > +		 * NMI while processing panic().  So, simply return without
> > +		 * a delay and re-enabling NMI.
> > +		 */
> > +		return;
> > +	}
> >
> >  	/* Re-enable the IOCK line, wait for a few seconds */
> >  	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
> > @@ -297,7 +305,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
> >
> >  	pr_emerg("Do you have a strange power saving mode enabled?\n");
> >  	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
> > -		panic("NMI: Not continuing");
> > +		nmi_panic("NMI: Not continuing");
> >
> >  	pr_emerg("Dazed and confused, but trying to continue\n");
> >  }
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 350dfb0..480a4fd 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -445,6 +445,19 @@ extern int sysctl_panic_on_stackoverflow;
> >
> >  extern bool crash_kexec_post_notifiers;
> >
> > +extern atomic_t panic_cpu;
> 
> This needs a comment explaining what this variable is and what it
> denotes.

OK, I'll do that in the next version.

> 
> > +
> > +/*
> > + * A variant of panic() called from NMI context.
> > + * If we've already panicked on this cpu, return from here.
> > + */
> > +#define nmi_panic(fmt, ...)						\
> > +	do {								\
> > +		int this_cpu = raw_smp_processor_id();			\
> > +		if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \
> > +			panic(fmt, ##__VA_ARGS__);			\
> > +	} while (0)
> > +
> >  /*
> >   * Only to be used by arch init code. If the user over-wrote the default
> >   * CONFIG_PANIC_TIMEOUT, honor it.
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 4579dbb..24ee2ea 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -61,6 +61,8 @@ void __weak panic_smp_self_stop(void)
> >  		cpu_relax();
> >  }
> >
> > +atomic_t panic_cpu = ATOMIC_INIT(-1);
> > +
> >  /**
> >   *	panic - halt the system
> >   *	@fmt: The text string to print
> > @@ -71,17 +73,17 @@ void __weak panic_smp_self_stop(void)
> >   */
> >  void panic(const char *fmt, ...)
> >  {
> > -	static DEFINE_SPINLOCK(panic_lock);
> >  	static char buf[1024];
> >  	va_list args;
> >  	long i, i_next = 0;
> >  	int state = 0;
> > +	int old_cpu, this_cpu;
> >
> >  	/*
> >  	 * Disable local interrupts. This will prevent panic_smp_self_stop
> >  	 * from deadlocking the first cpu that invokes the panic, since
> >  	 * there is nothing to prevent an interrupt handler (that runs
> > -	 * after the panic_lock is acquired) from invoking panic again.
> > +	 * after setting panic_cpu) from invoking panic again.
> >  	 */
> >  	local_irq_disable();
> >
> > @@ -94,8 +96,15 @@ void panic(const char *fmt, ...)
> >  	 * multiple parallel invocations of panic, all other CPUs either
> >  	 * stop themself or will wait until they are stopped by the 1st CPU
> >  	 * with smp_send_stop().
> > +	 *
> > +	 * `old_cpu == -1' means this is the 1st CPU which comes here, so
> > +	 * go ahead.
> > +	 * `old_cpu == this_cpu' means we came from nmi_panic() which sets
> > +	 * panic_cpu to this cpu.  In this case, this is also the 1st CPU.
> 
> I'd prefer that -1 to be
> 
> #define INVALID_CPU_NUM		-1
> 
> or
> 
> #define UNDEFINED_CPU_NUM	-1
> 
> or so instead of a naked number.

OK, I'll use a macro.

Regards,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group