x86: Fix up CFI for the nested NMI
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(-)
PATCH 1/2 x86-64: Fix CFI annotations for NMI nesting code
From: Jan Beulich <[email protected]>
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.
--- arch/x86/kernel/entry_64.S | 52 +++++++++++++++++++++++-------------------- 1 files changed, 28 insertions(+), 24 deletions(-)
arch/x86/kernel/entry_64.S +28 -24
@@ -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
Signature · 1 line
1.7.8.3
PATCH 2/2 x86: Fix the NMI nesting comments
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
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.
--- arch/x86/kernel/entry_64.S | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)
arch/x86/kernel/entry_64.S +8 -6
@@ -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
Signature · 1 line
1.7.8.3
Quoted 28 lines · Steven Rostedt
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/urgentHead SHA1:
79fb4ad63e8266ffac1f69bbb45a6f86570493e7Jan Beulich (1):
x86-64: Fix CFI annotations for NMI nesting code
Quoted 6 lines
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
Hi Ingo,
Quoted 30 lines · Ingo Molnar
Quoted 28 lines · Steven Rostedt
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/urgentHead SHA1:
79fb4ad63e8266ffac1f69bbb45a6f86570493e7Jan Beulich (1):
x86-64: Fix CFI annotations for NMI nesting code
Quoted 8 lines
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 4 lines
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
Quoted 36 lines · Steven Rostedt
Hi Ingo,
Quoted 33 lines · Ingo Molnar
Quoted 31 lines · Steven Rostedt
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/urgentHead SHA1:
79fb4ad63e8266ffac1f69bbb45a6f86570493e7Jan Beulich (1):
x86-64: Fix CFI annotations for NMI nesting codeSteven Rostedt (1):
x86: Fix the NMI nesting comments
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
----
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.
Quoted 15 lines
Quoted 2 lines
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.
Quoted 1 line
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
[...] 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.
Quoted 5 lines
Quoted 3 lines
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
Quoted 10 lines
Quoted 1 line
Quoted 1 line · Ingo Molnar
Quoted 6 lines · Steven Rostedt
Quoted 3 lines · Ingo Molnar
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
Quoted 7 lines · Jan Beulich
Quoted 7 lines
Quoted 1 line
Quoted 1 line · Ingo Molnar
Quoted 5 lines · Steven Rostedt
Quoted 3 lines · Ingo Molnar
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.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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