[PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]


Ingo,

Jan posted a patch that fixes the CFI annotations. I recommend getting
this into 3.3 as this is new code and it would be nice to have CFI
correct. It also does a little simplification of it as well.

The second patch is comment changes only (very low impact on messing
anything up). I realized that the comments had some references to
previous approaches that I tried, and I fixed them to reflect what
the final result was. I also added some more comments to describe
the code a bit better.

Thanks,

-- Steve

Please pull the latest tip/x86/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/x86/urgent

Head SHA1: 79fb4ad63e8266ffac1f69bbb45a6f86570493e7


Jan Beulich (1):
      x86-64: Fix CFI annotations for NMI nesting code

Steven Rostedt (1):
      x86: Fix the NMI nesting comments

----
 arch/x86/kernel/entry_64.S |   64 ++++++++++++++++++++++++--------------------
 1 files changed, 35 insertions(+), 29 deletions(-)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] x86-64: Fix CFI annotations for NMI nesting code

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 4265 bytes --]

From: Jan Beulich <JBeulich@suse.com>

The saving and restoring of %rdx wasn't annotated at all, and the
jumping over sections where state gets partly restored wasn't handled
either.

Further, by folding the pushing of the previous frame in repeat_nmi
into that which so far was immediately preceding restart_nmi (after
moving the restore of %rdx ahead of that, since it doesn't get used
anymore when pushing prior frames), annotations of the replicated
frame creations can be made consistent too.

v2: Fully fold repeat_nmi into the normal code flow (adding a single
    redundant instruction to the "normal" code path), thus retaining
    the special protection of all instructions between repeat_nmi and
    end_repeat_nmi.

Link: http://lkml.kernel.org/r/4F478B630200007800074A31@nat28.tlf.novell.com

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/entry_64.S |   52 +++++++++++++++++++++++--------------------
 1 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 1333d98..e0eca00 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1530,6 +1530,7 @@ ENTRY(nmi)
 
 	/* Use %rdx as out temp variable throughout */
 	pushq_cfi %rdx
+	CFI_REL_OFFSET rdx, 0
 
 	/*
 	 * If %cs was not the kernel segment, then the NMI triggered in user
@@ -1554,6 +1555,7 @@ ENTRY(nmi)
 	 */
 	lea 6*8(%rsp), %rdx
 	test_in_nmi rdx, 4*8(%rsp), nested_nmi, first_nmi
+	CFI_REMEMBER_STATE
 
 nested_nmi:
 	/*
@@ -1585,10 +1587,12 @@ nested_nmi:
 
 nested_nmi_out:
 	popq_cfi %rdx
+	CFI_RESTORE rdx
 
 	/* No need to check faults here */
 	INTERRUPT_RETURN
 
+	CFI_RESTORE_STATE
 first_nmi:
 	/*
 	 * Because nested NMIs will use the pushed location that we
@@ -1624,6 +1628,10 @@ first_nmi:
 	 * NMI may zero out. The original stack frame and the temp storage
 	 * is also used by nested NMIs and can not be trusted on exit.
 	 */
+	/* Do not pop rdx, nested NMIs will corrupt it */
+	movq (%rsp), %rdx
+	CFI_RESTORE rdx
+
 	/* Set the NMI executing variable on the stack. */
 	pushq_cfi $1
 
@@ -1631,14 +1639,31 @@ first_nmi:
 	.rept 5
 	pushq_cfi 6*8(%rsp)
 	.endr
+	CFI_DEF_CFA_OFFSET SS+8-RIP
+
+	/*
+	 * 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.
+	 */
+repeat_nmi:
+	/*
+	 * Update the stack variable to say we are still in NMI (the update
+	 * is benign for the non-repeat case, where 1 was pushed just above
+	 * to this very stack slot).
+	 */
+	movq $1, 5*8(%rsp)
 
 	/* Make another copy, this one may be modified by nested NMIs */
 	.rept 5
 	pushq_cfi 4*8(%rsp)
 	.endr
-
-	/* Do not pop rdx, nested NMIs will corrupt it */
-	movq 11*8(%rsp), %rdx
+	CFI_DEF_CFA_OFFSET SS+8-RIP
+end_repeat_nmi:
 
 	/*
 	 * Everything below this point can be preempted by a nested
@@ -1646,7 +1671,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
@@ -1675,26 +1699,6 @@ nmi_restore:
 	CFI_ENDPROC
 END(nmi)
 
-	/*
-	 * If an NMI hit an iret because of an exception or breakpoint,
-	 * it can lose its NMI context, and a nested NMI may come in.
-	 * In that case, the nested NMI will change the preempted NMI's
-	 * 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:
-
 ENTRY(ignore_sysret)
 	CFI_STARTPROC
 	mov $-ENOSYS,%eax
-- 
1.7.8.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] x86: Fix the NMI nesting comments

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Some of the comments for the nesting NMI algorithm were stale and
had some references to some prototypes that were first tried.

I also updated the comments to be a little easier to understand
the flow of the code. It definitely needs the documentation.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/entry_64.S |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index e0eca00..2de3e45 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -1624,11 +1624,12 @@ first_nmi:
 	 * | pt_regs                 |
 	 * +-------------------------+
 	 *
-	 * The saved RIP is used to fix up the copied RIP that a nested
-	 * NMI may zero out. The original stack frame and the temp storage
+	 * The saved stack frame is used to fix up the copied stack frame
+	 * that a nested NMI may change to make the interrupted NMI iret jump
+	 * to the repeat_nmi. The original stack frame and the temp storage
 	 * is also used by nested NMIs and can not be trusted on exit.
 	 */
-	/* Do not pop rdx, nested NMIs will corrupt it */
+	/* Do not pop rdx, nested NMIs will corrupt that part of the stack */
 	movq (%rsp), %rdx
 	CFI_RESTORE rdx
 
@@ -1641,6 +1642,8 @@ first_nmi:
 	.endr
 	CFI_DEF_CFA_OFFSET SS+8-RIP
 
+	/* Everything up to here is safe from nested NMIs */
+
 	/*
 	 * If there was a nested NMI, the first NMI's iret will return
 	 * here. But NMIs are still enabled and we can take another
@@ -1667,9 +1670,8 @@ end_repeat_nmi:
 
 	/*
 	 * Everything below this point can be preempted by a nested
-	 * NMI if the first NMI took an exception. Repeated NMIs
-	 * caused by an exception and nested NMI will start here, and
-	 * can still be preempted by another NMI.
+	 * NMI if the first NMI took an exception and reset our iret stack
+	 * so that we repeat another NMI.
 	 */
 	pushq_cfi $-1		/* ORIG_RAX: no syscall to restart */
 	subq $ORIG_RAX-R15, %rsp
-- 
1.7.8.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Ingo,
> 
> Jan posted a patch that fixes the CFI annotations. I recommend getting
> this into 3.3 as this is new code and it would be nice to have CFI
> correct. It also does a little simplification of it as well.
> 
> The second patch is comment changes only (very low impact on messing
> anything up). I realized that the comments had some references to
> previous approaches that I tried, and I fixed them to reflect what
> the final result was. I also added some more comments to describe
> the code a bit better.
> 
> Thanks,
> 
> -- Steve
> 
> Please pull the latest tip/x86/urgent tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> tip/x86/urgent
> 
> Head SHA1: 79fb4ad63e8266ffac1f69bbb45a6f86570493e7
> 
> 
> Jan Beulich (1):
>       x86-64: Fix CFI annotations for NMI nesting code
> 
> Steven Rostedt (1):
>       x86: Fix the NMI nesting comments
> 
> ----
>  arch/x86/kernel/entry_64.S |   64 ++++++++++++++++++++++++--------------------
>  1 files changed, 35 insertions(+), 29 deletions(-)

I don't think we want a 30+ lines diffstat to this rather 
non-trivial NMI codepath - and it changes real instructions, not 
just the CFI annotations.

Also, the 'update comments' commit does not belong into 
x86/urgent either.

So either you do an obviously trivial patch that only adds CFI 
annotations and nothing else, or I can pull these bits into 
tip:x86/debug, for a v3.4 merge.

Thanks,

	Ingo

Re: [PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Steven Rostedt]

Hi Ingo,

On Mon, 2012-02-27 at 08:39 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Ingo,
> > 
> > Jan posted a patch that fixes the CFI annotations. I recommend getting
> > this into 3.3 as this is new code and it would be nice to have CFI
> > correct. It also does a little simplification of it as well.
> > 
> > The second patch is comment changes only (very low impact on messing
> > anything up). I realized that the comments had some references to
> > previous approaches that I tried, and I fixed them to reflect what
> > the final result was. I also added some more comments to describe
> > the code a bit better.
> > 
> > Thanks,
> > 
> > -- Steve
> > 
> > Please pull the latest tip/x86/urgent tree, which can be found at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> > tip/x86/urgent
> > 
> > Head SHA1: 79fb4ad63e8266ffac1f69bbb45a6f86570493e7
> > 
> > 
> > Jan Beulich (1):
> >       x86-64: Fix CFI annotations for NMI nesting code
> > 
> > Steven Rostedt (1):
> >       x86: Fix the NMI nesting comments
> > 
> > ----
> >  arch/x86/kernel/entry_64.S |   64 ++++++++++++++++++++++++--------------------
> >  1 files changed, 35 insertions(+), 29 deletions(-)
> 
> I don't think we want a 30+ lines diffstat to this rather 

Some of that was just movement of code.

> non-trivial NMI codepath - and it changes real instructions, not 
> just the CFI annotations.

The changes to the real code made the CFI code easier to fix. But if you
are nervous about the code change (which actually simplifies the code),
I can ask Jan (Cc'd) to break out the patch into two changes if
possible.

I'm not sure how the CFI can handle the current trampoline, but perhaps
we can just fix the main part of the code and leave the trampoline part
broken? Then we can add the rest of the CFI changes and the movement of
the trampoline back into the function for the next release.

> 
> Also, the 'update comments' commit does not belong into 
> x86/urgent either.

Hmm, I didn't know that fixing comments was for a merge window only.
Some of the comments are currently wrong and I didn't think we would
want those in a main release. While reading the code again I realized
that I could also add more comments to make it easier to understand. I
would think that comments would be fine for the -rc releases because
they have almost no chance of introducing bugs.


> 
> So either you do an obviously trivial patch that only adds CFI 
> annotations and nothing else, or I can pull these bits into 
> tip:x86/debug, for a v3.4 merge.

I'm fine with waiting for v3.4 before these changes get in.

-- Steve

Re: [PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Ingo,
> 
> On Mon, 2012-02-27 at 08:39 +0100, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > 
> > > Ingo,
> > > 
> > > Jan posted a patch that fixes the CFI annotations. I recommend getting
> > > this into 3.3 as this is new code and it would be nice to have CFI
> > > correct. It also does a little simplification of it as well.
> > > 
> > > The second patch is comment changes only (very low impact on messing
> > > anything up). I realized that the comments had some references to
> > > previous approaches that I tried, and I fixed them to reflect what
> > > the final result was. I also added some more comments to describe
> > > the code a bit better.
> > > 
> > > Thanks,
> > > 
> > > -- Steve
> > > 
> > > Please pull the latest tip/x86/urgent tree, which can be found at:
> > > 
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> > > tip/x86/urgent
> > > 
> > > Head SHA1: 79fb4ad63e8266ffac1f69bbb45a6f86570493e7
> > > 
> > > 
> > > Jan Beulich (1):
> > >       x86-64: Fix CFI annotations for NMI nesting code
> > > 
> > > Steven Rostedt (1):
> > >       x86: Fix the NMI nesting comments
> > > 
> > > ----
> > >  arch/x86/kernel/entry_64.S |   64 ++++++++++++++++++++++++--------------------
> > >  1 files changed, 35 insertions(+), 29 deletions(-)
> > 
> > I don't think we want a 30+ lines diffstat to this rather 
> 
> Some of that was just movement of code.

That's why I said 30+ lines, not 60.

> > non-trivial NMI codepath - and it changes real instructions, 
> > not just the CFI annotations.
> 
> The changes to the real code made the CFI code easier to fix. 
> But if you are nervous about the code change (which actually 
> simplifies the code), I can ask Jan (Cc'd) to break out the 
> patch into two changes if possible.
> 
> I'm not sure how the CFI can handle the current trampoline, 
> but perhaps we can just fix the main part of the code and 
> leave the trampoline part broken? Then we can add the rest of 
> the CFI changes and the movement of the trampoline back into 
> the function for the next release.
> 
> > 
> > Also, the 'update comments' commit does not belong into 
> > x86/urgent either.
> 
> Hmm, I didn't know that fixing comments was for a merge window 
> only. [...]

We try to avoid them for later -rc's, and we are now in -rc5 
territory already.

> [...] Some of the comments are currently wrong and I didn't 
> think we would want those in a main release. While reading the 
> code again I realized that I could also add more comments to 
> make it easier to understand. I would think that comments 
> would be fine for the -rc releases because they have almost no 
> chance of introducing bugs.

By that argument we could be doing mechanic API renames in later 
-rc's as well.

In general we don't want "noise" around the really relevant 
changes to make them *really* obvious regression fixes - even if 
this noise is obviously correct code.

This is also code that *everyone* uses so if it breaks then 
everyone suffers (and seeing people suffer, even hypothetically, 
makes me sad), so the maintenance filter is quite a bit 
stricter. I wouldn't worry nearly as much about 
drivers/some/obscure/piece/of/hw.c.

> > So either you do an obviously trivial patch that only adds 
> > CFI annotations and nothing else, or I can pull these bits 
> > into tip:x86/debug, for a v3.4 merge.
> 
> I'm fine with waiting for v3.4 before these changes get in.

Jan, is that fine with you?

Thanks,

	Ingo

Re: [PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Jan Beulich]

>>> On 28.02.12 at 10:06, Ingo Molnar <mingo@elte.hu> wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Mon, 2012-02-27 at 08:39 +0100, Ingo Molnar wrote:
>> > So either you do an obviously trivial patch that only adds 
>> > CFI annotations and nothing else, or I can pull these bits 
>> > into tip:x86/debug, for a v3.4 merge.
>> 
>> I'm fine with waiting for v3.4 before these changes get in.
> 
> Jan, is that fine with you?

Yes, absolutely (I never expected these changes to be proposed for
inclusion in 3.3, given that I didn't have the time to get them done
and submitted before -rc4).

Jan

Re: [PATCH 0/2] [GIT PULL][v3.3] x86: Fix up CFI for the nested NMI

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 28.02.12 at 10:06, Ingo Molnar <mingo@elte.hu> wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> >> On Mon, 2012-02-27 at 08:39 +0100, Ingo Molnar wrote:
> >> > So either you do an obviously trivial patch that only adds 
> >> > CFI annotations and nothing else, or I can pull these bits 
> >> > into tip:x86/debug, for a v3.4 merge.
> >> 
> >> I'm fine with waiting for v3.4 before these changes get in.
> > 
> > Jan, is that fine with you?
> 
> Yes, absolutely (I never expected these changes to be proposed 
> for inclusion in 3.3, given that I didn't have the time to get 
> them done and submitted before -rc4).

Ok, pulled it into x86/asm for v3.4, thanks guys!

	Ingo