Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753121AbbKXEGN (ORCPT ); Mon, 23 Nov 2015 23:06:13 -0500 Received: from mail9.hitachi.co.jp ([133.145.228.44]:50356 "EHLO mail9.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752949AbbKXEGL (ORCPT ); Mon, 23 Nov 2015 23:06:11 -0500 From: =?utf-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= To: "'Borislav Petkov'" CC: Jonathan Corbet , Peter Zijlstra , Ingo Molnar , "Eric W. Biederman" , "H. Peter Anvin" , Andrew Morton , Thomas Gleixner , Vivek Goyal , Baoquan He , "linux-doc@vger.kernel.org" , "x86@kernel.org" , "kexec@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Michal Hocko , Ingo Molnar , =?utf-8?B?5bmz5p2+6ZuF5bezIC8gSElSQU1BVFXvvIxNQVNBTUk=?= Subject: RE: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI Thread-Topic: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI Thread-Index: AQHRI3eQfslqjNfXkkuusbGs5dKomp6pYY8AgAEvwPA= Date: Tue, 24 Nov 2015 04:06:03 +0000 Message-ID: <04EAB7311EE43145B2D3536183D1A84454A24B0F@GSjpTKYDCembx31.service.hitachi.net> References: <20151120093641.4285.97253.stgit@softrs> <20151120093644.4285.9349.stgit@softrs> <20151123184930.GH5134@pd.tnic> In-Reply-To: <20151123184930.GH5134@pd.tnic> Accept-Language: ja-JP, en-US Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.198.220.44] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tAO46IFe018398 Content-Length: 5995 Lines: 182 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 > > Cc: Andrew Morton > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: Peter Zijlstra > > Cc: Michal Hocko > > --- > > 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 ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?