Re: [PATCH 3 of 4 RFC] x86/nmi: Prevent reentrant execution of the C nmi handler

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/12/12 09:21, Jan Beulich wrote:
>>>> On 04.12.12 at 19:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> The (old) function do_nmi() is not reentrantly-safe.  Rename it to
>> _do_nmi() and present a new do_nmi() which reentrancy guards.
>>
>> If a reentrant NMI has been detected, then it is highly likely that the
>> outer NMI exception frame has been corrupted, meaning we cannot return
>> to the original context.  In this case, we panic() obviously rather than
>> falling into an infinite loop.
>>
>> panic() however is not safe to reenter from an NMI context, as an NMI
>> (or MCE) can interrupt it inside its critical section, at which point a
>> new call to panic() will deadlock.  As a result, we bail early if a
>> panic() is already in progress, as Xen is about to die anyway.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> --
>> I am fairly sure this is safe with the current kexec_crash functionality
>> which involves holding all non-crashing pcpus in an NMI loop.  In the
>> case of reentrant NMIs and panic_in_progress, we will repeatedly bail
>> early in an infinite loop of NMIs, which has the same intended effect of
>> simply causing all non-crashing CPUs to stay out of the way while the
>> main crash occurs.
>>
>> diff -r 48a60a407e15 -r f6ad86b61d5a xen/arch/x86/traps.c
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -88,6 +88,7 @@ static char __read_mostly opt_nmi[10] = 
>>  string_param("nmi", opt_nmi);
>>  
>>  DEFINE_PER_CPU(u64, efer);
>> +static DEFINE_PER_CPU(bool_t, nmi_in_progress) = 0;
>>  
>>  DEFINE_PER_CPU_READ_MOSTLY(u32, ler_msr);
>>  
>> @@ -3182,7 +3183,8 @@ static int dummy_nmi_callback(struct cpu
>>   
>>  static nmi_callback_t nmi_callback = dummy_nmi_callback;
>>  
>> -void do_nmi(struct cpu_user_regs *regs)
>> +/* This function should never be called directly.  Use do_nmi() instead. */
>> +static void _do_nmi(struct cpu_user_regs *regs)
>>  {
>>      unsigned int cpu = smp_processor_id();
>>      unsigned char reason;
>> @@ -3208,6 +3210,44 @@ void do_nmi(struct cpu_user_regs *regs)
>>      }
>>  }
>>  
>> +/* This function is NOT SAFE to call from C code in general.
>> + * Use with extreme care! */
>> +void do_nmi(struct cpu_user_regs *regs)
>> +{
>> +    bool_t * in_progress = &this_cpu(nmi_in_progress);
>> +
>> +    if ( is_panic_in_progress() )
>> +    {
>> +        /* A panic is already in progress.  It may have reenabled NMIs,
>> +         * or we are simply unluckly to receive one right now.  Either
>> +         * way, bail early, as Xen is about to die.
>> +         *
>> +         * TODO: Ideally we should exit without executing an iret, to
>> +         * leave NMIs disabled, but that option is not currently
>> +         * available to us.
> You could easily provide the ground work for this here by having
> the function return a bool_t (even if not immediately consumed by
> the caller in this same patch).
>
> Jan

Will do.  I had considered a bool_t and was planning to integrate it
later in development.

~Andrew

>
>> +         */
>> +        return;
>> +    }
>> +
>> +    if ( test_and_set_bool(*in_progress) )
>> +    {
>> +        /* Crash in an obvious mannor, as opposed to falling into
>> +         * infinite loop because our exception frame corrupted the
>> +         * exception frame of the previous NMI.
>> +         *
>> +         * TODO: This check does not cover all possible cases of corrupt
>> +         * exception frames, but it is substantially better than
>> +         * nothing.
>> +         */
>> +        console_force_unlock();
>> +        show_execution_state(regs);
>> +        panic("Reentrant NMI detected\n");
>> +    }
>> +
>> +    _do_nmi(regs);
>> +    *in_progress = 0;
>> +}
>> +
>>  void set_nmi_callback(nmi_callback_t callback)
>>  {
>>      nmi_callback = callback;
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org 
>> http://lists.xen.org/xen-devel 
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com