Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
On Tue, 24 Nov 2015 21:27:13 +0100 Michal Hocko wrote: > > What happens if: > > > > CPU 0: CPU 1: > > -- -- > > nmi_panic(); > > > > nmi_panic(); > > > > nmi_panic(); > > I thought that nmi_panic is called only from the nmi context. If so how > can we get a nested NMI like that? Nevermind. I was thinking the external NMI could nest, but I'm guessing it cant. Anyway, the patches later on modify this code which checks for something other than != this_cpu, which makes this issue mute even if it could nest. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
On Tue 24-11-15 10:05:10, Steven Rostedt wrote:
> On Fri, Nov 20, 2015 at 06:36:44PM +0900, Hidehiro Kawai wrote:
> > 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;
> > +
> > +/*
> > + * 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__); \
>
> Hmm,
>
> What happens if:
>
> CPU 0: CPU 1:
> -- --
> nmi_panic();
>
> nmi_panic();
>
> nmi_panic();
I thought that nmi_panic is called only from the nmi context. If so how
can we get a nested NMI like that?
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
On Fri, Nov 20, 2015 at 06:36:44PM +0900, Hidehiro Kawai wrote:
> 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;
> +
> +/*
> + * 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__); \
Hmm,
What happens if:
CPU 0: CPU 1:
-- --
nmi_panic();
nmi_panic();
nmi_panic();
?
cmpxchg(&panic_cpu, -1, 0) != 0
returns -1 for cpu 0, thus 0 != 0, and sets panic_cpu to 0
cmpxchg(&panic_cpu, -1, 1) != 1
returns 0, and then it too panics, but does not set panic_cpu to 1
Now you have your external NMI triggering on CPU 1
cmpxchg(&panic_cpu, -1, 1) != 1
returns 0 again, and you call panic again within the panic of CPU 1.
Is this OK?
Perhaps you want a per cpu bitmask, and do a test_and_set() on the CPU. That
would prevent any CPU from rerunning a panic() twice on any CPU.
-- Steve
> + } 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
On Tue, 24 Nov 2015 10:05:10 -0500 Steven Rostedt wrote: > cmpxchg(&panic_cpu, -1, 0) != 0 > > returns -1 for cpu 0, thus 0 != 0, and sets panic_cpu to 0 That was suppose to be "thus -1 != 0". -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
On Fri 20-11-15 18:36:44, 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
I've finally seen testing results for these patches and managed to look
at them again.
Acked-by: 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");
> +
> + /*
> + * 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;
> +
> +/*
> + * 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 un
RE: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
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_s
Re: [V5 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI
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?
> +
> + /*
> + * 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.
> +
> +/*
> + * 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, ...)
>
