[3.12] x86/jump-label: Clean ups and safety checks
Subsystems (2)
Steven Rostedt
reviewerPeter,
These 4 patches were taken from the patchset that did the 2-5 byte jumps, but had nothing to do with the 2-5 byte jumps itself. They were clean ups and safety checks that were needed for the jump code. Although the 2-5 byte jump code is controversial, these cleanups are not.
The first change is to use the default nops instead of a jmp to next instruction in the jump label code.
The second is a boot time optimization that doesn't bother changing the nops if the default nop is the same as the ideal nop.
The third patch adds safety checks like the ftrace function code has. It makes sure the op code that is being changed is in fact the op code we expect it to be.
The last patch will output the problem code if it fails the safety check from patch three.
This is based off of tip/x86/jumplabel, which was equal to v3.11-rc1.
Please pull the latest tip/x86/jumplabel tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/x86/jumplabel
Head SHA1: fb40d7a8994a3cc7a1e1c1f3258ea8662a366916
Steven Rostedt (4):
x86/jump-label: Use best default nops for inital jump label calls x86/jump-label: Do not bother updating nops if they are correct x86/jump-label: Add safety checks to jump label conversions x86/jump-label: Show where and what was wrong on errors
----
arch/x86/include/asm/jump_label.h | 9 +++-- arch/x86/kernel/jump_label.c | 70 ++++++++++++++++++++++++++++++++++--- 2 files changed, 73 insertions(+), 6 deletions(-)
Steven Rostedt
reviewerPATCH 1/4 x86/jump-label: Use best default nops for inital jump label calls
From: Steven Rostedt <[email protected]>
As specified by H. Peter Anvin, the best nops for x86 without knowing the running computer is:
32bit:
0x3e, 0x8d, 0x74, 0x26, 0x00 also known as GENERIC_NOP5_ATOMIC
64bit:
0x0f, 0x1f, 0x44, 0x00, 0x00 also known as P6_NOP5_ATOMIC
Currently the default nop that is used by jump label is:
0xe9 0x00 0x00 0x00 0x00
Which is really a 5byte jump to the next position.
It's better to use a real nop than a jmp.
--- arch/x86/include/asm/jump_label.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
arch/x86/include/asm/jump_label.h +7 -2
@@ -3,18 +3,23 @@#ifdef __KERNEL__ +#include <linux/stringify.h> #include <linux/types.h> #include <asm/nops.h> #include <asm/asm.h> #define JUMP_LABEL_NOP_SIZE 5 -#define STATIC_KEY_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t" +#ifdef CONFIG_X86_64 +# define STATIC_KEY_INIT_NOP P6_NOP5_ATOMIC +#else +# define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC +#endif static __always_inline bool arch_static_branch(struct static_key *key) { asm goto("1:" -STATIC_KEY_INITIAL_NOP + ".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"".pushsection __jump_table, \"aw\" \n\t" _ASM_ALIGN"\n\t" _ASM_PTR"1b, %l[l_yes], %c0 \n\t"
Signature · 1 line
1.7.10.4
Steven Rostedt
reviewerPATCH 2/4 x86/jump-label: Do not bother updating nops if they are correct
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
On boot up, the jump label init function scans all the jump label locations and converts them to the best nop for the machine. If the nop is already the ideal nop, do not bother with changing it.
--- arch/x86/kernel/jump_label.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
arch/x86/kernel/jump_label.c +24 -1
@@ -50,10 +50,33 @@ void arch_jump_label_transform(struct jump_entry *entry,put_online_cpus (); } +static enum { +JL_STATE_START , +JL_STATE_NO_UPDATE , +JL_STATE_UPDATE , +} jlstate __initdata_or_module = JL_STATE_START; + __init_or_module void arch_jump_label_transform_static(struct jump_entry *entry,enum jump_label_type type) { -__jump_label_transform (entry, type, text_poke_early); +/* +* This function is called at boot up and when modules are +* first loaded. Check if the default nop, the one that is +* inserted at compile time, is the ideal nop. If it is, then +* we do not need to update the nop, and we can leave it as is. +* If it is not, then we need to update the nop to the ideal nop. +*/ +if (jlstate ==JL_STATE_START ) { +const unsigned char default_nop[] = {STATIC_KEY_INIT_NOP }; +const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5 ]; + +if (memcmp (ideal_nop, default_nop,5 ) !=0 ) + jlstate =JL_STATE_UPDATE ; +else + jlstate =JL_STATE_NO_UPDATE ; + } +if (jlstate ==JL_STATE_UPDATE ) +__jump_label_transform (entry, type, text_poke_early); }#endif
Signature · 1 line
1.7.10.4
Steven Rostedt
reviewerPATCH 3/4 x86/jump-label: Add safety checks to jump label conversions
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
As with all modifying of kernel text, we need to be very paranoid.
When converting the jump label locations to and from nops to jumps a check has been added to make sure what we are replacing is what we expect, otherwise we bug.
--- arch/x86/kernel/jump_label.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)
arch/x86/kernel/jump_label.c +28 -4
@@ -26,16 +26,40 @@ union jump_code_union {static void __jump_label_transform (struct jump_entry *entry,enum jump_label_type type, -void *(*poker)(void *,const void *,size_t )) +void *(*poker)(void *,const void *,size_t ), +int init) {union jump_code_union code; +const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5 ];if (type ==JUMP_LABEL_ENABLE ) { +/* +* We are enabling this jump label. If it is not a nop +* then something must have gone wrong. +*/ +BUG_ON (memcmp ((void *)entry->code, ideal_nop,5 ) !=0 ); + code.jump =0xe9 ; code.offset = entry->target - (entry->code +JUMP_LABEL_NOP_SIZE ); - }else + }else { +/* +* We are disabling this jump label. If it is not what +* we think it is, then something must have gone wrong. +* If this is the first initialization call, then we +* are converting the default nop to the ideal nop. +*/ +if (init) { +const unsigned char default_nop[] = {STATIC_KEY_INIT_NOP }; +BUG_ON (memcmp ((void *)entry->code, default_nop,5 ) !=0 ); + }else { + code.jump =0xe9 ; + code.offset = entry->target - + (entry->code +JUMP_LABEL_NOP_SIZE ); +BUG_ON (memcmp ((void *)entry->code, &code,5 ) !=0 ); + }memcpy (&code, ideal_nops[NOP_ATOMIC5 ],JUMP_LABEL_NOP_SIZE ); + } (*poker)((void *)entry->code, &code,JUMP_LABEL_NOP_SIZE ); }
@@ -45,7 +69,7 @@ void arch_jump_label_transform(struct jump_entry *entry, {get_online_cpus ();mutex_lock (&text_mutex); -__jump_label_transform (entry, type, text_poke_smp); +__jump_label_transform (entry, type, text_poke_smp,0 );mutex_unlock (&text_mutex);put_online_cpus (); }
@@ -76,7 +100,7 @@ __init_or_module void arch_jump_label_transform_static(struct jump_entry *entry, jlstate = JL_STATE_NO_UPDATE; }if (jlstate ==JL_STATE_UPDATE ) -__jump_label_transform (entry, type, text_poke_early); +__jump_label_transform (entry, type, text_poke_early,1 ); }#endif
Signature · 1 line
1.7.10.4
Steven Rostedt
reviewerPATCH 4/4 x86/jump-label: Show where and what was wrong on errors
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
When modifying text sections for jump labels, a paranoid check is performed. If the check fails, the system "bugs". But why it failed is not shown.
The BUG_ON()s in the jump label update code is replaced with bug_at(ip). This is a function that will show what pointer failed, and what was at the location of the failure that made jump label panic.
--- arch/x86/kernel/jump_label.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
arch/x86/kernel/jump_label.c +18 -3
@@ -24,6 +24,18 @@ union jump_code_union { } __attribute__((packed)); }; +static void bug_at (unsigned char *ip,int line) +{ +/* +* The location is not an op that we were expecting. +* Something went wrong. Crash the box, as something could be +* corrupting the kernel. +*/ +pr_warning ("Unexpected op at %pS [%p] (%02x %02x %02x %02x %02x) %s:%d\n" , + ip, ip, ip[0 ], ip[1 ], ip[2 ], ip[3 ], ip[4 ], __FILE__, line); +BUG (); +} + static void __jump_label_transform(struct jump_entry *entry, enum jump_label_type type, void *(*poker)(void *, const void *, size_t),
@@ -37,7 +49,8 @@ static void __jump_label_transform(struct jump_entry *entry,* We are enabling this jump label. If it is not a nop * then something must have gone wrong. */ -BUG_ON (memcmp ((void *)entry->code, ideal_nop,5 ) !=0 ); +if (unlikely (memcmp ((void *)entry->code, ideal_nop,5 ) !=0 )) +bug_at ((void *)entry->code, __LINE__); code.jump =0xe9 ; code.offset = entry->target -
@@ -51,12 +64,14 @@ static void __jump_label_transform(struct jump_entry *entry,*/ if (init) {const unsigned char default_nop[] = {STATIC_KEY_INIT_NOP }; -BUG_ON (memcmp ((void *)entry->code, default_nop,5 ) !=0 ); +if (unlikely (memcmp ((void *)entry->code, default_nop,5 ) !=0 )) +bug_at ((void *)entry->code, __LINE__); }else { code.jump =0xe9 ; code.offset = entry->target - (entry->code +JUMP_LABEL_NOP_SIZE ); -BUG_ON (memcmp ((void *)entry->code, &code,5 ) !=0 ); +if (unlikely (memcmp ((void *)entry->code, &code,5 ) !=0 )) +bug_at ((void *)entry->code, __LINE__); }memcpy (&code, ideal_nops[NOP_ATOMIC5 ],JUMP_LABEL_NOP_SIZE ); }
Signature · 1 line
1.7.10.4
Borislav Petkov
maintainerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 23 lines · Steven Rostedt
From: Steven Rostedt <[email protected]>
When modifying text sections for jump labels, a paranoid check is performed. If the check fails, the system "bugs". But why it failed is not shown.
The BUG_ON()s in the jump label update code is replaced with
bug_at(ip). This is a function that will show what pointer failed, and what was at the location of the failure that made jump label panic.Signed-off-by Steven Rostedt <[email protected]>--- arch/x86/kernel/jump_label.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)arch/x86/kernel/jump_label.c +0
-0@@ -24,6 +24,18 @@ union jump_code_union { } __attribute__((packed)); };
arch/x86/kernel/jump_label.c union jump_code_union { · line 36
Quoted 10 lines
arch/x86/kernel/jump_label.c +10
-0+static void bug_at (unsigned char *ip,int line) +{ +/* +* The location is not an op that we were expecting. +* Something went wrong. Crash the box, as something could be +* corrupting the kernel. +*/ +pr_warning ("Unexpected op at %pS [%p] (%02x %02x %02x %02x %02x) %s:%d\n" , + ip, ip, ip[0 ], ip[1 ], ip[2 ], ip[3 ], ip[4 ], __FILE__, line); +BUG ();
Why not simply
panic("Unexpected...")?
Besides, BUG can be disabled in CONFIG_EXPERT.
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--Steven Rostedt
reviewerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 14 lines · Borislav Petkov
arch/x86/kernel/jump_label.c union jump_code_union { · line 36
Quoted 11 lines
arch/x86/kernel/jump_label.c +10
-0+static void bug_at (unsigned char *ip,int line) +{ +/* +* The location is not an op that we were expecting. +* Something went wrong. Crash the box, as something could be +* corrupting the kernel. +*/ +pr_warning ("Unexpected op at %pS [%p] (%02x %02x %02x %02x %02x) %s:%d\n" , + ip, ip, ip[0 ], ip[1 ], ip[2 ], ip[3 ], ip[4 ], __FILE__, line); +BUG ();Why not simply
Quoted 3 lines
panic("Unexpected...")?
Because the patch replaced BUG().
Quoted 3 lines
Besides, BUG can be disabled in
CONFIG_EXPERT.
Right, and this code keeps the same logic as it was before. If it was disabled by CONFIG_EXPERT, it stays disabled, but at least you get to see a warning that your kernel may be corrupt now :-)
-- Steve
Borislav Petkov
maintainerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 3 lines · Steven Rostedt
Right, and this code keeps the same logic as it was before. If it was disabled by
CONFIG_EXPERT, it stays disabled, but at least you get to see a warning that your kernel may be corrupt now :-)
Don't we really want to panic instead of running a corrupt kernel? IOW, to change the logic to panic unconditionally because the image in memory has been violated and not in a good way, at that :-)
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--Steven Rostedt
reviewerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 8 lines · Borislav Petkov
Quoted 3 lines · Steven Rostedt
Right, and this code keeps the same logic as it was before. If it was disabled by
CONFIG_EXPERT, it stays disabled, but at least you get to see a warning that your kernel may be corrupt now :-)Don't we really want to panic instead of running a corrupt kernel? IOW, to change the logic to panic unconditionally because the image in memory has been violated and not in a good way, at that :-)
Well, there's lots of places that use BUG() for a corrupt kernel. If you are stupid enough to disable it, you get what you asked for.
-- Steve
H. Peter Anvin
reviewerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 6 lines · Steven Rostedt
Quoted 5 lines · Borislav Petkov
Quoted 3 lines · Steven Rostedt
Right, and this code keeps the same logic as it was before. If it was disabled by
CONFIG_EXPERT, it stays disabled, but at least you get to see a warning that your kernel may be corrupt now :-)
Quoted 7 lines
Quoted 3 lines
Don't we really want to panic instead of running a corrupt kernel? IOW, to change the logic to panic unconditionally because the image in memory has been violated and not in a good way, at that :-)
Well, there's lots of places that use
BUG()for a corrupt kernel. If you are stupid enough to disable it, you get what you asked for.
A bigger issue is probably if panic-on-bug should be the default, with !panic being an opt-in debugging option.
-hpa
Borislav Petkov
maintainerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 2 lines · H. Peter Anvin
A bigger issue is probably if panic-on-bug should be the default, with !panic being an opt-in debugging option.
Yes, it might make sense although embedded wants to disable CONFIG_BUG on systems which cannot report errors:
│ CONFIG_BUG: │ │ Disabling this option eliminates support for BUG and WARN, reducing │ the size of your kernel image and potentially quietly ignoring │ numerous fatal conditions. You should only consider disabling this │ option for embedded systems with no facilities for reporting errors. │ Just say Y.
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--H. Peter Anvin
reviewerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 7 lines · Borislav Petkov
Quoted 2 lines · H. Peter Anvin
A bigger issue is probably if panic-on-bug should be the default, with !panic being an opt-in debugging option.
Yes, it might make sense although embedded wants to disable
CONFIG_BUGon systems which cannot report errors:
There is another thread going on to convert BUG to trap even on machines where CONFIG_BUG is disabled, i.e. CONFIG_BUG would control the metadata only. With the configuration option off, you would get the address of the failure only.
-hpa
Peter Zijlstra
maintainerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
In reply to Borislav PetkovQuoted 1 line · Borislav Petkov
Besides, BUG can be disabled in
CONFIG_EXPERT.
There was some email on this subject a while ago; disabling BUG() is very risky and can cause all kinds of horrid. IIRC the consensus back then was to remove this 'feature' and have BUG always at least lock up the machine hard.
H. Peter Anvin
reviewerRe: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 8 lines · Peter Zijlstra
Quoted 1 line · Borislav Petkov
Besides, BUG can be disabled in
CONFIG_EXPERT.There was some email on this subject a while ago; disabling
BUG()is very risky and can cause all kinds of horrid. IIRC the consensus back then was to remove this 'feature' and have BUG always at least lock up the machine hard.
Yes, always trap but leave the metadata as optional.
-hpa
Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors
Quoted 10 lines · H. Peter Anvin
Quoted 7 lines · Peter Zijlstra
Quoted 1 line · Borislav Petkov
Besides, BUG can be disabled in
CONFIG_EXPERT.There was some email on this subject a while ago; disabling
BUG()is very risky and can cause all kinds of horrid. IIRC the consensus back then was to remove this 'feature' and have BUG always at least lock up the machine hard.Yes, always trap but leave the metadata as optional.
I think those patches are going forward, so we can just regard BUG() as always existing from a program flow POV.
Thanks,
Ingo