linux-kernel - RE: Re: [V2 PATCH 1/3] x86/panic: Fix re-entrance problem due to panic on NMI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <04EAB7311EE43145B2D3536183D1A8445491F23A@GSjpTKYDCembx31.service.hitachi.net>
Date:	Thu, 30 Jul 2015 01:45:35 +0000
From:	河合英宏 / KAWAI，HIDEHIRO 
	<hidehiro.kawai.ez@...achi.com>
To:	"'Michal Hocko'" <mhocko@...nel.org>
CC:	Jonathan Corbet <corbet@....net>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Vivek Goyal <vgoyal@...hat.com>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	平松雅巳 / HIRAMATU，MASAMI 
	<masami.hiramatsu.pt@...achi.com>
Subject: RE: Re: [V2 PATCH 1/3] x86/panic: Fix re-entrance problem due to
 panic on NMI

Hi,

> From: Michal Hocko [mailto:mhocko@...nel.org]
> 
> On Wed 29-07-15 09:09:18, 河合英宏 / KAWAI，HIDEHIRO wrote:
> > > From: Michal Hocko [mailto:mhocko@...nel.org]
> > > On Wed 29-07-15 05:48:47, 河合英宏 / KAWAI，HIDEHIRO wrote:
> > > > Hi,
> > > >
> > > > > From: linux-kernel-owner@...r.kernel.org [mailto:linux-kernel-owner@...r.kernel.org] On Behalf Of Hidehiro Kawai
> > > > > (2015/07/27 23:34), Michal Hocko wrote:
> > > > > > On Mon 27-07-15 10:58:50, Hidehiro Kawai wrote:
> > > > [...]
> > > > > > The check could be also relaxed a bit and nmi_panic would
> > > > > > return only if the ongoing panic is the current cpu when we really have
> > > > > > to return and allow the preempted panic to finish.
> > > > >
> > > > > It's reasonable.  I'll do that in the next version.
> > > >
> > > > I noticed atomic_read() is insufficient.  Please consider the following
> > > > scenario.
> > > >
> > > > CPU 1: call panic() in the normal context
> > > > CPU 0: call nmi_panic(), check the value of panic_cpu, then call panic()
> > > > CPU 1: set 1 to panic_cpu
> > > > CPU 0: fail to set 0 to panic_cpu, then do an infinite loop
> > > > CPU 1: call crash_kexec(), then call kdump_nmi_shootdown_cpus()
> > > >
> > > > At this point, since CPU 0 loops in NMI context, it never executes
> > > > the NMI handler registered by kdump_nmi_shootdown_cpus().  This means
> > > > that no register states are saved and no cleanups for VMX/SVM are
> > > > performed.
> > >
> > > Yes this is true but it is no different from the current state, isn't
> > > it? So if you want to handle that then it deserves a separate patch.
> > > It is certainly not harmful wrt. panic behavior.
> > >
> > > > So, we should still use atomic_cmpxchg() in nmi_panic() to
> > > > prevent other cpus from running panic routines.
> > >
> > > Not sure what you mean by that.
> >
> > I mean that we should use the same logic as my V2 patch like this:
> >
> > #define nmi_panic(fmt, ...)                                            \
> >        do {                                                            \
> >                if (atomic_cmpxchg(&panic_cpu, -1, raw_smp_processor_id()) \
> >                    == -1)                                              \
> >                        panic(fmt, ##__VA_ARGS__);                      \
> >        } while (0)
> 
> This would allow to return from NMI too eagerly.

Yes, but what's the problem?
The root cause of your case hasn't been clarified yet.
I can't fix for an unclear issue because I don't know what's the right
solution.

> When I was testing my
> previous approach (on 3.0 based kernel) I had basically the same thing
> (one NMI to process panic) and others to return. This led to a strange
> behavior when the NMI button triggered NMI on all (hundreds) CPUs.

It's strange.  Usually, NMI caused by NMI button is routed to only CPU 0
as an external NMI.  External NMI for CPUs other than CPU 0 are masked
at boot time.  Does it really happen?  Does the problem still happen on
the latest kernel?  What kind of NMI is deliverd to each CPU?

Traditionally, we should have assumed that NMI for crash dumping is
delivered to only one cpu.  Otherwise, we should often fail to take
a proper crash dump.  It seems that your case is another problem to be
solved separately.

> The
> crash kernel booted eventually but the log contained lockups when a
> CPU waited for an IPI to the CPU which was handling the NMI panic.

Could you explain more precisely?

> Anyway, I do not thing this is really necessary to solve the panic
> reentrancy issue.
> If the missing saved state is a real problem then it
> should be handled separately - maybe it can be achieved without an IPI
> and directly from the panic context if we are in NMI.

What I would like to do via this patchse is to solve race issues
among NMI, panic() and crash_kexec().  So, I don't think we should fix
that separately, although I would need to reword some descriptions
and titles.

Anyway, I'm going to sent out my revised version once in order to
tidy up.  I also would like to hear kexec/kdump guys' opinions.

Regards,
Kawai