Re: [PATCH v2] x86: Fix MCE error handing when kdump is enabled

[Corey Minyard <minyard@acm.org>]

On Thu, Oct 01, 2020 at 01:33:18PM +0200, Borislav Petkov wrote:
> On Wed, Sep 30, 2020 at 01:49:06PM -0500, Corey Minyard wrote:
> > That is the original post for this, yes.
> > 
> > Wu, what kernel version are you using?  Can you try to reproduce on the
> > current mainstream kernel?  I just assumed it was current.
> > 
> > The description isn't that great, no.  I'll try again.
> > 
> > The problem is that while waiting in wait_for_panic() in the mce code,
> > interrupts are enabled.  In the kdump case, there is nothing that will
> > wake them up, so they will sit there in the loop until they time out.
> > 
> > In the mean time, the cpu handling the panic calls some IPMI code that
> > stores panic information in the IPMI event log.  Since interrupts are
> > enabled on the CPUs in wait_for_panic(), those CPUs are handling
> > interrupts from the IPMI hardware.  They will not, however, handle
> > the NMI IPI that gets sent from the panic() code for kdump.
> > 
> > The IPMI code has disabled locks to avoid a deadlock if the exception
> > happens while in IPMI code.  So the panic-handling part of IPMI and the
> > IPMI interrupt handling are both running at the same time, messing each
> > other up.
> > 
> > It seems, in general, like a bad idea to have interrupts enabled on some
> > CPUs while running through the panic() code and after the new kdump
> > kernel has started.  There are other issues that might come from this.
> > 
> > I'm also not quite sure how kdump register information for the CPUs
> > in wait_for_panic() gets put into the kernel coredump if you don't do
> > something like my patch.
> > 
> 
> Ok, thanks for taking the time, this makes a lot more sense to me.
> 
> Now, from looking at the code, I'm thinking that we should simply "let
> go" the other CPUs just like we do in mce_check_crashing_cpu(), if
> kdump is starting. Instead of spinning with IRQs enabled.
> 
> Simply run mce_check_crashing_cpu() at wait_for_panic() entry, just like
> exc_machine_check_kernel() does now. The logic being, if we're going
> to wait for panic but we're starting the kdump kernel anyway, then we
> better let the CPUs go so that they can do all kinds of IRQ servicing
> etc and don't interfere.

I don't understand the last sentence.  You don't want to do IRQ
servicing when you are going to kdump.  That's going to change the state
of the kernel and you may lose information, and it may interfere with
the kdump process.

That's why (well, one of many reasons why) kdump goes straight to NMI
shootdown.

Also, it's still unclear to me how kdump would get the register
information for the CPUs that enter wait_for_panic().

> 
> *If* we don't kdump, we timeout the usual way.

I was thinking about this some yesterday.  It seems to me that enabling
IRQS in an MCE handler is just a bad idea, but it's really a bad idea
for kdump.

I think you could just remove the irq enable in wait_for_panic() and
call run_crash_ipi_callback() from the loop there without messing
with irqs.  In the non-kdump case, it waits a second for the
RESET_VECTOR to happen in native_stop_other_cpus() then it uses an NMI
shootdown.  So it will delay for a second in the normal panic case.
The kdump case uses nmi_shootdown_cpus(), which doesn't do the
RESET_VECTOR stop.

-corey

> 
> Tony, how does that logic sound?
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette