Re: [PATCH] x86/nmi: Make external NMI injection reliably crash the host

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 08/26/2014 01:59 PM, Jan Beulich wrote:
>>>> On 26.08.14 at 12:10, <ross.lagerwall@citrix.com> wrote:
>> --- a/xen/arch/x86/nmi.c
>> +++ b/xen/arch/x86/nmi.c
>> @@ -15,6 +15,7 @@
>>
>>   #include <xen/config.h>
>>   #include <xen/init.h>
>> +#include <xen/stdbool.h>
>
> ???

I thought this was needed for bool_t but obviously it's not.

>
>> @@ -432,35 +436,23 @@ int __init watchdog_setup(void)
>>       return 0;
>>   }
>>
>> -void nmi_watchdog_tick(const struct cpu_user_regs *regs)
>> +/* Returns true if this was a watchdog NMI, false otherwise */
>> +bool_t nmi_watchdog_tick(const struct cpu_user_regs *regs)
>>   {
>> +    bool_t watchdog_tick = 0;
>>       unsigned int sum = this_cpu(nmi_timer_ticks);
>>
>> -    if ( (this_cpu(last_irq_sums) == sum) && watchdog_enabled() )
>> -    {
>> -        /*
>> -         * Ayiee, looks like this CPU is stuck ... wait for the timeout
>> -         * before doing the oops ...
>> -         */
>> -        this_cpu(alert_counter)++;
>> -        if ( this_cpu(alert_counter) == opt_watchdog_timeout*nmi_hz )
>> -        {
>> -            console_force_unlock();
>> -            printk("Watchdog timer detects that CPU%d is stuck!\n",
>> -                   smp_processor_id());
>> -            fatal_trap(TRAP_nmi, regs);
>> -        }
>> -    }
>> -    else
>> -    {
>> -        this_cpu(last_irq_sums) = sum;
>> -        this_cpu(alert_counter) = 0;
>> -    }
>> -
>>       if ( nmi_perfctr_msr )
>>       {
>
> So if we don't get into this block ...
>
>> +        uint64_t msr_content;
>> +
>> +        /* Work out if this is a watchdog tick by checking for overflow. */
>>           if ( nmi_perfctr_msr == MSR_P4_IQ_PERFCTR0 )
>>           {
>> +            rdmsrl(MSR_P4_IQ_CCCR0, msr_content);
>> +            if ( msr_content & P4_CCCR_OVF )
>> +                watchdog_tick = 1;
>> +
>>               /*
>>                * P4 quirks:
>>                * - An overflown perfctr will assert its interrupt
>> @@ -473,14 +465,52 @@ void nmi_watchdog_tick(const struct cpu_user_regs *regs)
>>           }
>>           else if ( nmi_perfctr_msr == MSR_P6_PERFCTR0 )
>>           {
>> +            rdmsrl(MSR_P6_PERFCTR0, msr_content);
>> +            watchdog_tick = !(msr_content & (1ULL << P6_EVENT_WIDTH));
>> +
>>               /*
>>                * Only P6 based Pentium M need to re-unmask the apic vector but
>>                * it doesn't hurt other P6 variants.
>>                */
>>               apic_write(APIC_LVTPC, APIC_DM_NMI);
>>           }
>> -        write_watchdog_counter(NULL);
>> +        else if ( nmi_perfctr_msr == MSR_K7_PERFCTR0 )
>> +        {
>> +            rdmsrl(MSR_K7_PERFCTR0, msr_content);
>> +            watchdog_tick = !(msr_content & (1ULL << K7_EVENT_WIDTH));
>> +        }
>> +
>> +        if ( watchdog_tick )
>> +        {
>
> ... we'll never get here. That's clearly a change in behavior for
> systems with no suitable perfctr MSR. I'm of the opinion that for
> such systems behavior should not change from what it is right
> now, i.e. if you can't exclude the NMI to be watchdog induced,
> assume it is (rather than assuming that to be a fatal condition).
>
> Or wait - is this only a theoretical consideration? If so, the patch
> description should be adjusted to make clear we can't get there.
> And perhaps the surrounding if(nmi_perfctr_msr) should then
> become ASSERT(nmi_perfctr_msr).

A comment in the source code says that the watchdog may also be driven 
from the I/O APIC timer so this is probably a valid consideration. I 
shall rework this a bit.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -3312,8 +3312,8 @@ void do_nmi(const struct cpu_user_regs *regs)
>>       if ( nmi_callback(regs, cpu) )
>>           return;
>>
>> -    if ( nmi_watchdog )
>> -        nmi_watchdog_tick(regs);
>> +    if ( nmi_watchdog && nmi_watchdog_tick(regs) )
>> +        return;
>>
>>       /* Only the BSP gets external NMIs from the system. */
>>       if ( cpu == 0 )
>> @@ -3323,7 +3323,7 @@ void do_nmi(const struct cpu_user_regs *regs)
>>               pci_serr_error(regs);
>>           if ( reason & 0x40 )
>>               io_check_error(regs);
>> -        if ( !(reason & 0xc0) && !nmi_watchdog )
>> +        if ( !(reason & 0xc0) )
>>               unknown_nmi_error(regs, reason);
>
> As much as I like the original idea, I'm afraid this won't fly: I do
> know of systems where bad motherboard design leads to neither
> of these two bits ever getting set. I.e. at the very minimum we'd
> need a command line option to restore old behavior. Personally I
> think it should in fact remain default behavior, and new behavior
> should only be enabled via command line option.
>

Well the old behavior was different depending on whether the watchdog 
was enabled or not. Since the watchdog was disabled by default, that's 
no different from the behavior here.

So are you thinking something like an ignore_unknown_nmi boolean 
parameter that defaults to true?

Regards
-- 
Ross Lagerwall