lkml.org 
[lkml]   [2012]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86-64: fix CFI annotations for NMI nesting code
>>> On 24.02.12 at 15:11, Steven Rostedt <srostedt@redhat.com> wrote:
> On Fri, 2012-02-24 at 12:06 +0000, Jan Beulich wrote:
>> /* Make another copy, this one may be modified by nested NMIs */
>> .rept 5
>> pushq_cfi 4*8(%rsp)
>
> Half way through this copy (the rept is not atomic) we get an NMI, it
> sees that we are nested so it updates this copy of the interrupt frame
> and returns. Now the rest of the copy finishes so we end up with a half
> and half stack frame (half to go back to restart_nmi and half to go back
> to the original location of the stack). The result is a system crash.
>
> But! There is a fix to this. We can move the setting of the special
> variable to here and make this the "repeart_nmi" instead. The nested NMI
> checks that we are not in the repeat nmi before updating the stack. If
> it interrupted in the repeat nmi it will just return knowing a repeat is
> about to take place anyway.
>
>
> So instead of adding restart_nmi here, add:
>
> repeat_nmi:
> /* Update the stack variable to say we are in NMI */
> movq $1, 5*8(%rsp)
>
> /* Make another copy, this one may be modified by nested NMIs */
> .rept 5
> pushq_cfi 4*8(%rsp)
> end_repeat_nmi:
>
>
> And just continue. We only need to protect against updating the stack
> and the nested NMI will not touch it if it interrupted RIP is between
> repeat_nmi and end_repeat_nmi. But there's no reason that we can't just
> set the iret jump to go back into the middle of the NMI handler.
>
> Would that work for you?

Of course, thanks for spotting this oversight of mine. I assume you
meant the end_repeat_nmi to go after the .endr though, not before.
I'll get a fixed patch sent out soon.

Jan

> Also we need to add a comment about this. Just above the repeat_nmi:
>
> /*
> * If there was a nested NMI, the first NMI's iret will return
> * here. But NMIs are still enabled and we can take another
> * nested NMI. The nested NMI checks the interrupted RIP to see
> * if it is between repeat_nmi and end_repeat_nmi, and if so
> * it will just return, as we are about to repeat an NMI anyway.
> * This makes it safe to copy to the stack frame that a nested
> * NMI will update.
> */
>
>
> -- Steve
>
>> .endr
>> -
>> - /* Do not pop rdx, nested NMIs will corrupt it */
>> - movq 11*8(%rsp), %rdx
>> + CFI_DEF_CFA_OFFSET SS+8-RIP
>>
>> /*
>> * Everything below this point can be preempted by a nested
>> @@ -1639,7 +1647,6 @@ first_nmi:
>> * caused by an exception and nested NMI will start here, and
>> * can still be preempted by another NMI.
>> */
>> -restart_nmi:
>> pushq_cfi $-1 /* ORIG_RAX: no syscall to restart */
>> subq $ORIG_RAX-R15, %rsp
>> CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
>> @@ -1665,8 +1672,6 @@ nmi_restore:
>> /* Clear the NMI executing stack variable */
>> movq $0, 10*8(%rsp)
>> jmp irq_return
>> - CFI_ENDPROC
>> -END(nmi)
>>
>> /*
>> * If an NMI hit an iret because of an exception or breakpoint,
>> @@ -1675,18 +1680,12 @@ END(nmi)
>> * stack to jump to here when it does the final iret.
>> */
>> repeat_nmi:
>> - INTR_FRAME
>> /* Update the stack variable to say we are still in NMI */
>> movq $1, 5*8(%rsp)
>> -
>> - /* copy the saved stack back to copy stack */
>> - .rept 5
>> - pushq_cfi 4*8(%rsp)
>> - .endr
>> -
>> jmp restart_nmi
>> - CFI_ENDPROC
>> end_repeat_nmi:
>> + CFI_ENDPROC
>> +END(nmi)
>>
>> ENTRY(ignore_sysret)
>> CFI_STARTPROC
>>
>>
>>





\
 
 \ /
  Last update: 2012-02-24 15:39    [W:0.053 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site