Skip to contentSkip to conversation

[3.12] x86/jump-label: Clean ups and safety checks

GIT PULL In review Steven Rostedt 5 participants 15 messages Aug 7, 2013 – Aug 12, 2013

Subsystems (2)
lore
Share & tools
lore

Steven Rostedt

author reviewer
#0 lore raw

Peter,

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(-)
author reviewer
#1 lore raw

PATCH 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.

Cc H. Peter Anvin <[email protected]>
Cc Jason Baron <[email protected]>
---
 arch/x86/include/asm/jump_label.h |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
arch/x86/include/asm/jump_label.h +7 -2
index 3a16c14..64507f3 100644
@@ -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
author reviewer
#2 lore raw

PATCH 2/4 x86/jump-label: Do not bother updating nops if they are correct

In reply to Steven Rostedt

From: 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.

Cc Jason Baron <[email protected]>
Cc H. Peter Anvin <[email protected]>
---
 arch/x86/kernel/jump_label.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)
arch/x86/kernel/jump_label.c +24 -1
index 2889b3d..ae3d8fb 100644
@@ -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
author reviewer
#3 lore raw

PATCH 3/4 x86/jump-label: Add safety checks to jump label conversions

In reply to Steven Rostedt

From: 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.

Cc H. Peter Anvin <[email protected]>
Cc Jason Baron <[email protected]>
---
 arch/x86/kernel/jump_label.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)
arch/x86/kernel/jump_label.c +28 -4
index ae3d8fb..24cf2b2 100644
@@ -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
author reviewer
#4 lore raw

PATCH 4/4 x86/jump-label: Show where and what was wrong on errors

In reply to 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.

---
 arch/x86/kernel/jump_label.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)
arch/x86/kernel/jump_label.c +18 -3
index 24cf2b2..912a528 100644
@@ -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
maintainer
#5 lore raw

Re: [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.

---
 arch/x86/kernel/jump_label.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)
arch/x86/kernel/jump_label.c +0 -0
index 24cf2b2..912a528 100644
@@ -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.
--
author reviewer
#6 lore raw

Re: [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

maintainer
#7 lore raw

Re: [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.
--
author reviewer
#8 lore raw

Re: [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

#9 lore raw

Re: [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

maintainer
#10 lore raw

Re: [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.
--
#11 lore raw

Re: [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_BUG on 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

maintainer
#12 lore raw

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

In reply to Borislav Petkov
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.

#13 lore raw

Re: [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

#14 lore raw

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