[PATCH 0/7] [GIT PULL] jump label: fixes and work arounds

[PATCH 0/7] [GIT PULL] jump label: fixes and work arounds

[Steven Rostedt]

Ingo,

Please pull the latest tip/perf/jump-label-2 tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/perf/jump-label-2


David Miller (1):
      jump_label: Fix unaligned traps on sparc.

H. Peter Anvin (1):
      x86, ftrace: Use safe noops, drop trap test

Jason Baron (2):
      jump label: Fix module __init section race
      jump label: Fix deadlock b/w jump_label_mutex vs. text_mutex

Steven Rostedt (3):
      jump label: Fix error with preempt disable holding mutex
      jump label: Make arch_jump_label_text_poke_early() optional
      jump label: Add work around to i386 gcc asm goto bug

----
 arch/Kconfig                        |   14 ++++++
 arch/sparc/include/asm/jump_label.h |    1 +
 arch/x86/Makefile_32.cpu            |   13 +++++-
 arch/x86/kernel/alternative.c       |   69 +++++++------------------------
 include/linux/jump_label.h          |    7 +++-
 kernel/jump_label.c                 |   77 ++++++++++++++++++++++++++++++-----
 kernel/kprobes.c                    |   26 +++++++-----
 7 files changed, 130 insertions(+), 77 deletions(-)

[PATCH 1/7] jump label: Fix module __init section race

[Steven Rostedt]

[-- Attachment #1: 0001-jump-label-Fix-module-__init-section-race.patch --]
[-- Type: text/plain, Size: 3257 bytes --]

From: Jason Baron <jbaron@redhat.com>

Jump label uses is_module_text_address() to ensure that the module
__init sections are valid before updating them. However, between the
check for a valid module __init section and the subsequent jump
label update, the module's __init section could be freed out from under
us.

We fix this potential race by adding a notifier callback to the
MODULE_STATE_LIVE state. This notifier is called *after* the __init
section has been run but before it is going to be freed. In the
callback, the jump label code zeros the key value for any __init jump
code within the module, and we add a check for a non-zero key value when
we update jump labels. In this way we require no additional data
structures.

Thanks to Mathieu Desnoyers for pointing out this race condition.

Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Jason Baron <jbaron@redhat.com>
LKML-Reference: <c6f037b7598777668025ceedd9294212fd95fa34.1285965957.git.jbaron@redhat.com>

[ Renamed remove_module_init() to remove_jump_label_module_init()
  as suggested by Masami Hiramatsu. ]

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/jump_label.c |   41 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 7be868b..be9e105 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -168,7 +168,8 @@ void jump_label_update(unsigned long key, enum jump_label_type type)
 			count = e_module->nr_entries;
 			iter = e_module->table;
 			while (count--) {
-				if (kernel_text_address(iter->code))
+				if (iter->key &&
+						kernel_text_address(iter->code))
 					arch_jump_label_transform(iter, type);
 				iter++;
 			}
@@ -366,6 +367,39 @@ static void remove_jump_label_module(struct module *mod)
 	}
 }
 
+static void remove_jump_label_module_init(struct module *mod)
+{
+	struct hlist_head *head;
+	struct hlist_node *node, *node_next, *module_node, *module_node_next;
+	struct jump_label_entry *e;
+	struct jump_label_module_entry *e_module;
+	struct jump_entry *iter;
+	int i, count;
+
+	/* if the module doesn't have jump label entries, just return */
+	if (!mod->num_jump_entries)
+		return;
+
+	for (i = 0; i < JUMP_LABEL_TABLE_SIZE; i++) {
+		head = &jump_label_table[i];
+		hlist_for_each_entry_safe(e, node, node_next, head, hlist) {
+			hlist_for_each_entry_safe(e_module, module_node,
+						  module_node_next,
+						  &(e->modules), hlist) {
+				if (e_module->mod != mod)
+					continue;
+				count = e_module->nr_entries;
+				iter = e_module->table;
+				while (count--) {
+					if (within_module_init(iter->code, mod))
+						iter->key = 0;
+					iter++;
+				}
+			}
+		}
+	}
+}
+
 static int
 jump_label_module_notify(struct notifier_block *self, unsigned long val,
 			 void *data)
@@ -386,6 +420,11 @@ jump_label_module_notify(struct notifier_block *self, unsigned long val,
 		remove_jump_label_module(mod);
 		mutex_unlock(&jump_label_mutex);
 		break;
+	case MODULE_STATE_LIVE:
+		mutex_lock(&jump_label_mutex);
+		remove_jump_label_module_init(mod);
+		mutex_unlock(&jump_label_mutex);
+		break;
 	}
 	return ret;
 }
-- 
1.7.1

[PATCH 2/7] jump label: Fix deadlock b/w jump_label_mutex vs. text_mutex

[Steven Rostedt]

[-- Attachment #1: 0002-jump-label-Fix-deadlock-b-w-jump_label_mutex-vs.-tex.patch --]
[-- Type: text/plain, Size: 5944 bytes --]

From: Jason Baron <jbaron@redhat.com>

register_kprobe() downs the 'text_mutex' and then calls
jump_label_text_reserved(), which downs the 'jump_label_mutex'.
However, the jump label code takes those mutexes in the reverse
order.

Fix by requiring the caller of jump_label_text_reserved() to do
the jump label locking via the newly added: jump_label_lock(),
jump_label_unlock(). Currently, kprobes is the only user
of jump_label_text_reserved().

Reported-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Jason Baron <jbaron@redhat.com>
LKML-Reference: <759032c48d5e30c27f0bba003d09bffa8e9f28bb.1285965957.git.jbaron@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/jump_label.h |    5 +++++
 kernel/jump_label.c        |   33 +++++++++++++++++++++------------
 kernel/kprobes.c           |    6 ++++++
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index b67cb18..1947a12 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -18,6 +18,8 @@ struct module;
 extern struct jump_entry __start___jump_table[];
 extern struct jump_entry __stop___jump_table[];
 
+extern void jump_label_lock(void);
+extern void jump_label_unlock(void);
 extern void arch_jump_label_transform(struct jump_entry *entry,
 				 enum jump_label_type type);
 extern void arch_jump_label_text_poke_early(jump_label_t addr);
@@ -59,6 +61,9 @@ static inline int jump_label_text_reserved(void *start, void *end)
 	return 0;
 }
 
+static inline void jump_label_lock(void) {}
+static inline void jump_label_unlock(void) {}
+
 #endif
 
 #define COND_STMT(key, stmt)					\
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index be9e105..12cce78 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -39,6 +39,16 @@ struct jump_label_module_entry {
 	struct module *mod;
 };
 
+void jump_label_lock(void)
+{
+	mutex_lock(&jump_label_mutex);
+}
+
+void jump_label_unlock(void)
+{
+	mutex_unlock(&jump_label_mutex);
+}
+
 static int jump_label_cmp(const void *a, const void *b)
 {
 	const struct jump_entry *jea = a;
@@ -152,7 +162,7 @@ void jump_label_update(unsigned long key, enum jump_label_type type)
 	struct jump_label_module_entry *e_module;
 	int count;
 
-	mutex_lock(&jump_label_mutex);
+	jump_label_lock();
 	entry = get_jump_label_entry((jump_label_t)key);
 	if (entry) {
 		count = entry->nr_entries;
@@ -175,7 +185,7 @@ void jump_label_update(unsigned long key, enum jump_label_type type)
 			}
 		}
 	}
-	mutex_unlock(&jump_label_mutex);
+	jump_label_unlock();
 }
 
 static int addr_conflict(struct jump_entry *entry, void *start, void *end)
@@ -232,6 +242,7 @@ out:
  * overlaps with any of the jump label patch addresses. Code
  * that wants to modify kernel text should first verify that
  * it does not overlap with any of the jump label addresses.
+ * Caller must hold jump_label_mutex.
  *
  * returns 1 if there is an overlap, 0 otherwise
  */
@@ -242,7 +253,6 @@ int jump_label_text_reserved(void *start, void *end)
 	struct jump_entry *iter_stop = __start___jump_table;
 	int conflict = 0;
 
-	mutex_lock(&jump_label_mutex);
 	iter = iter_start;
 	while (iter < iter_stop) {
 		if (addr_conflict(iter, start, end)) {
@@ -257,7 +267,6 @@ int jump_label_text_reserved(void *start, void *end)
 	conflict = module_conflict(start, end);
 #endif
 out:
-	mutex_unlock(&jump_label_mutex);
 	return conflict;
 }
 
@@ -268,7 +277,7 @@ static __init int init_jump_label(void)
 	struct jump_entry *iter_stop = __stop___jump_table;
 	struct jump_entry *iter;
 
-	mutex_lock(&jump_label_mutex);
+	jump_label_lock();
 	ret = build_jump_label_hashtable(__start___jump_table,
 					 __stop___jump_table);
 	iter = iter_start;
@@ -276,7 +285,7 @@ static __init int init_jump_label(void)
 		arch_jump_label_text_poke_early(iter->code);
 		iter++;
 	}
-	mutex_unlock(&jump_label_mutex);
+	jump_label_unlock();
 	return ret;
 }
 early_initcall(init_jump_label);
@@ -409,21 +418,21 @@ jump_label_module_notify(struct notifier_block *self, unsigned long val,
 
 	switch (val) {
 	case MODULE_STATE_COMING:
-		mutex_lock(&jump_label_mutex);
+		jump_label_lock();
 		ret = add_jump_label_module(mod);
 		if (ret)
 			remove_jump_label_module(mod);
-		mutex_unlock(&jump_label_mutex);
+		jump_label_unlock();
 		break;
 	case MODULE_STATE_GOING:
-		mutex_lock(&jump_label_mutex);
+		jump_label_lock();
 		remove_jump_label_module(mod);
-		mutex_unlock(&jump_label_mutex);
+		jump_label_unlock();
 		break;
 	case MODULE_STATE_LIVE:
-		mutex_lock(&jump_label_mutex);
+		jump_label_lock();
 		remove_jump_label_module_init(mod);
-		mutex_unlock(&jump_label_mutex);
+		jump_label_unlock();
 		break;
 	}
 	return ret;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 99865c3..9437e14 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1146,13 +1146,16 @@ int __kprobes register_kprobe(struct kprobe *p)
 		return ret;
 
 	preempt_disable();
+	jump_label_lock();
 	if (!kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr) ||
 	    ftrace_text_reserved(p->addr, p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr)) {
 		preempt_enable();
+		jump_label_unlock();
 		return -EINVAL;
 	}
+	jump_label_unlock();
 
 	/* User can pass only KPROBE_FLAG_DISABLED to register_kprobe */
 	p->flags &= KPROBE_FLAG_DISABLED;
@@ -1187,6 +1190,8 @@ int __kprobes register_kprobe(struct kprobe *p)
 	INIT_LIST_HEAD(&p->list);
 	mutex_lock(&kprobe_mutex);
 
+	jump_label_lock(); /* needed to call jump_label_text_reserved() */
+
 	get_online_cpus();	/* For avoiding text_mutex deadlock. */
 	mutex_lock(&text_mutex);
 
@@ -1214,6 +1219,7 @@ int __kprobes register_kprobe(struct kprobe *p)
 out:
 	mutex_unlock(&text_mutex);
 	put_online_cpus();
+	jump_label_unlock();
 	mutex_unlock(&kprobe_mutex);
 
 	if (probed_mod)
-- 
1.7.1

[PATCH 3/7] jump label: Fix error with preempt disable holding mutex

[Steven Rostedt]

[-- Attachment #1: 0003-jump-label-Fix-error-with-preempt-disable-holding-mu.patch --]
[-- Type: text/plain, Size: 2548 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Kprobes and jump label were having a race between mutexes that
was fixed by reordering the jump label. But this reordering
moved the jump label mutex into a preempt disable location.

This patch does a little fiddling to move the grabbing of
the jump label mutex from inside the preempt disable section
and still keep the order correct between the mutex and the
kprobes lock.

Reported-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jason Baron <jbaron@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/kprobes.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9437e14..9737a76 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1145,17 +1145,13 @@ int __kprobes register_kprobe(struct kprobe *p)
 	if (ret)
 		return ret;
 
-	preempt_disable();
 	jump_label_lock();
+	preempt_disable();
 	if (!kernel_text_address((unsigned long) p->addr) ||
 	    in_kprobes_functions((unsigned long) p->addr) ||
 	    ftrace_text_reserved(p->addr, p->addr) ||
-	    jump_label_text_reserved(p->addr, p->addr)) {
-		preempt_enable();
-		jump_label_unlock();
-		return -EINVAL;
-	}
-	jump_label_unlock();
+	    jump_label_text_reserved(p->addr, p->addr))
+		goto fail_with_jump_label;
 
 	/* User can pass only KPROBE_FLAG_DISABLED to register_kprobe */
 	p->flags &= KPROBE_FLAG_DISABLED;
@@ -1169,10 +1165,9 @@ int __kprobes register_kprobe(struct kprobe *p)
 		 * We must hold a refcount of the probed module while updating
 		 * its code to prohibit unexpected unloading.
 		 */
-		if (unlikely(!try_module_get(probed_mod))) {
-			preempt_enable();
-			return -EINVAL;
-		}
+		if (unlikely(!try_module_get(probed_mod)))
+			goto fail_with_jump_label;
+
 		/*
 		 * If the module freed .init.text, we couldn't insert
 		 * kprobes in there.
@@ -1180,11 +1175,11 @@ int __kprobes register_kprobe(struct kprobe *p)
 		if (within_module_init((unsigned long)p->addr, probed_mod) &&
 		    probed_mod->state != MODULE_STATE_COMING) {
 			module_put(probed_mod);
-			preempt_enable();
-			return -EINVAL;
+			goto fail_with_jump_label;
 		}
 	}
 	preempt_enable();
+	jump_label_unlock();
 
 	p->nmissed = 0;
 	INIT_LIST_HEAD(&p->list);
@@ -1226,6 +1221,11 @@ out:
 		module_put(probed_mod);
 
 	return ret;
+
+fail_with_jump_label:
+	preempt_enable();
+	jump_label_unlock();
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(register_kprobe);
 
-- 
1.7.1

[PATCH 4/7] jump label: Make arch_jump_label_text_poke_early() optional

[Steven Rostedt]

[-- Attachment #1: 0004-jump-label-Make-arch_jump_label_text_poke_early-opti.patch --]
[-- Type: text/plain, Size: 1018 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Some archs do not need to do anything special for jump labels on
startup (like MIPS).  This patch adds a weak function stub for
arch_jump_label_text_poke_early();

Cc: Jason Baron <jbaron@redhat.com>
Cc: David Miller <davem@davemloft.net>
Cc: David Daney <ddaney@caviumnetworks.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <1286218615-24011-2-git-send-email-ddaney@caviumnetworks.com>
LKML-Reference: <20101015201037.703989993@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/jump_label.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 12cce78..3b79bd9 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -270,6 +270,13 @@ out:
 	return conflict;
 }
 
+/*
+ * Not all archs need this.
+ */
+void __weak arch_jump_label_text_poke_early(jump_label_t addr)
+{
+}
+
 static __init int init_jump_label(void)
 {
 	int ret;
-- 
1.7.1

[PATCH 5/7] jump_label: Fix unaligned traps on sparc.

[Steven Rostedt]

[-- Attachment #1: 0005-jump_label-Fix-unaligned-traps-on-sparc.patch --]
[-- Type: text/plain, Size: 1049 bytes --]

From: David Miller <davem@davemloft.net>

The vmlinux.lds.h knobs to emit the __jump_table section in the main
kernel image takes care to align the section, but this doesn't help
for the __jump_table section that gets emitted into modules.

Fix the resulting lack of section alignment by explicitly specifying
it in the assembler.

Signed-off-by: David S. Miller <davem@davemloft.net>
LKML-Reference: <20101023.110624.226758370.davem@davemloft.net>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/sparc/include/asm/jump_label.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/sparc/include/asm/jump_label.h b/arch/sparc/include/asm/jump_label.h
index 62e66d7..d95cf44 100644
--- a/arch/sparc/include/asm/jump_label.h
+++ b/arch/sparc/include/asm/jump_label.h
@@ -14,6 +14,7 @@
 			 "nop\n\t"				\
 			 "nop\n\t"				\
 			 ".pushsection __jump_table,  \"a\"\n\t"\
+			 ".align 4\n\t"				\
 			 ".word 1b, %l[" #label "], %c0\n\t"	\
 			 ".popsection \n\t"			\
 			 : :  "i" (key) :  : label);\
-- 
1.7.1

[PATCH 6/7] x86, ftrace: Use safe noops, drop trap test

[Steven Rostedt]

[-- Attachment #1: 0006-x86-ftrace-Use-safe-noops-drop-trap-test.patch --]
[-- Type: text/plain, Size: 3371 bytes --]

From: H. Peter Anvin <hpa@zytor.com>

Always use a safe 5-byte noop sequence.  Drop the trap test, since it
is known to return false negatives on some virtualization platforms on
32 bits.  The resulting code is both simpler and safer.

Cc: Daniel Drake <dsd@laptop.org>
Cc: Jason Baron <jbaron@redhat.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/alternative.c |   69 +++++++++--------------------------------
 1 files changed, 15 insertions(+), 54 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index a36bb90..0b30214 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -644,65 +644,26 @@ void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len)
 
 #if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
 
-unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
+#ifdef CONFIG_X86_64
+unsigned char ideal_nop5[5] = { 0x66, 0x66, 0x66, 0x66, 0x90 };
+#else
+unsigned char ideal_nop5[5] = { 0x3e, 0x8d, 0x74, 0x26, 0x00 };
+#endif
 
 void __init arch_init_ideal_nop5(void)
 {
-	extern const unsigned char ftrace_test_p6nop[];
-	extern const unsigned char ftrace_test_nop5[];
-	extern const unsigned char ftrace_test_jmp[];
-	int faulted = 0;
-
 	/*
-	 * There is no good nop for all x86 archs.
-	 * We will default to using the P6_NOP5, but first we
-	 * will test to make sure that the nop will actually
-	 * work on this CPU. If it faults, we will then
-	 * go to a lesser efficient 5 byte nop. If that fails
-	 * we then just use a jmp as our nop. This isn't the most
-	 * efficient nop, but we can not use a multi part nop
-	 * since we would then risk being preempted in the middle
-	 * of that nop, and if we enabled tracing then, it might
-	 * cause a system crash.
+	 * There is no good nop for all x86 archs.  This selection
+	 * algorithm should be unified with the one in find_nop_table(),
+	 * but this should be good enough for now.
 	 *
-	 * TODO: check the cpuid to determine the best nop.
+	 * For cases other than the ones below, use the safe (as in
+	 * always functional) defaults above.
 	 */
-	asm volatile (
-		"ftrace_test_jmp:"
-		"jmp ftrace_test_p6nop\n"
-		"nop\n"
-		"nop\n"
-		"nop\n"  /* 2 byte jmp + 3 bytes */
-		"ftrace_test_p6nop:"
-		P6_NOP5
-		"jmp 1f\n"
-		"ftrace_test_nop5:"
-		".byte 0x66,0x66,0x66,0x66,0x90\n"
-		"1:"
-		".section .fixup, \"ax\"\n"
-		"2:	movl $1, %0\n"
-		"	jmp ftrace_test_nop5\n"
-		"3:	movl $2, %0\n"
-		"	jmp 1b\n"
-		".previous\n"
-		_ASM_EXTABLE(ftrace_test_p6nop, 2b)
-		_ASM_EXTABLE(ftrace_test_nop5, 3b)
-		: "=r"(faulted) : "0" (faulted));
-
-	switch (faulted) {
-	case 0:
-		pr_info("converting mcount calls to 0f 1f 44 00 00\n");
-		memcpy(ideal_nop5, ftrace_test_p6nop, IDEAL_NOP_SIZE_5);
-		break;
-	case 1:
-		pr_info("converting mcount calls to 66 66 66 66 90\n");
-		memcpy(ideal_nop5, ftrace_test_nop5, IDEAL_NOP_SIZE_5);
-		break;
-	case 2:
-		pr_info("converting mcount calls to jmp . + 5\n");
-		memcpy(ideal_nop5, ftrace_test_jmp, IDEAL_NOP_SIZE_5);
-		break;
-	}
-
+#ifdef CONFIG_X86_64
+	/* Don't use these on 32 bits due to broken virtualizers */
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+		memcpy(ideal_nop5, p6_nops[5], 5);
+#endif
 }
 #endif
-- 
1.7.1

Re: [PATCH 6/7] x86, ftrace: Use safe noops, drop trap test

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> From: H. Peter Anvin <hpa@zytor.com>
> 
> Always use a safe 5-byte noop sequence.  Drop the trap test, since it
> is known to return false negatives on some virtualization platforms on
> 32 bits.  The resulting code is both simpler and safer.
> 
> Cc: Daniel Drake <dsd@laptop.org>
> Cc: Jason Baron <jbaron@redhat.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Looks good to me.

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/alternative.c |   69 +++++++++--------------------------------
>  1 files changed, 15 insertions(+), 54 deletions(-)
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index a36bb90..0b30214 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -644,65 +644,26 @@ void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len)
>  
>  #if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
>  
> -unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
> +#ifdef CONFIG_X86_64
> +unsigned char ideal_nop5[5] = { 0x66, 0x66, 0x66, 0x66, 0x90 };
> +#else
> +unsigned char ideal_nop5[5] = { 0x3e, 0x8d, 0x74, 0x26, 0x00 };
> +#endif
>  
>  void __init arch_init_ideal_nop5(void)
>  {
> -	extern const unsigned char ftrace_test_p6nop[];
> -	extern const unsigned char ftrace_test_nop5[];
> -	extern const unsigned char ftrace_test_jmp[];
> -	int faulted = 0;
> -
>  	/*
> -	 * There is no good nop for all x86 archs.
> -	 * We will default to using the P6_NOP5, but first we
> -	 * will test to make sure that the nop will actually
> -	 * work on this CPU. If it faults, we will then
> -	 * go to a lesser efficient 5 byte nop. If that fails
> -	 * we then just use a jmp as our nop. This isn't the most
> -	 * efficient nop, but we can not use a multi part nop
> -	 * since we would then risk being preempted in the middle
> -	 * of that nop, and if we enabled tracing then, it might
> -	 * cause a system crash.
> +	 * There is no good nop for all x86 archs.  This selection
> +	 * algorithm should be unified with the one in find_nop_table(),
> +	 * but this should be good enough for now.
>  	 *
> -	 * TODO: check the cpuid to determine the best nop.
> +	 * For cases other than the ones below, use the safe (as in
> +	 * always functional) defaults above.
>  	 */
> -	asm volatile (
> -		"ftrace_test_jmp:"
> -		"jmp ftrace_test_p6nop\n"
> -		"nop\n"
> -		"nop\n"
> -		"nop\n"  /* 2 byte jmp + 3 bytes */
> -		"ftrace_test_p6nop:"
> -		P6_NOP5
> -		"jmp 1f\n"
> -		"ftrace_test_nop5:"
> -		".byte 0x66,0x66,0x66,0x66,0x90\n"
> -		"1:"
> -		".section .fixup, \"ax\"\n"
> -		"2:	movl $1, %0\n"
> -		"	jmp ftrace_test_nop5\n"
> -		"3:	movl $2, %0\n"
> -		"	jmp 1b\n"
> -		".previous\n"
> -		_ASM_EXTABLE(ftrace_test_p6nop, 2b)
> -		_ASM_EXTABLE(ftrace_test_nop5, 3b)
> -		: "=r"(faulted) : "0" (faulted));
> -
> -	switch (faulted) {
> -	case 0:
> -		pr_info("converting mcount calls to 0f 1f 44 00 00\n");
> -		memcpy(ideal_nop5, ftrace_test_p6nop, IDEAL_NOP_SIZE_5);
> -		break;
> -	case 1:
> -		pr_info("converting mcount calls to 66 66 66 66 90\n");
> -		memcpy(ideal_nop5, ftrace_test_nop5, IDEAL_NOP_SIZE_5);
> -		break;
> -	case 2:
> -		pr_info("converting mcount calls to jmp . + 5\n");
> -		memcpy(ideal_nop5, ftrace_test_jmp, IDEAL_NOP_SIZE_5);
> -		break;
> -	}
> -
> +#ifdef CONFIG_X86_64
> +	/* Don't use these on 32 bits due to broken virtualizers */
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		memcpy(ideal_nop5, p6_nops[5], 5);
> +#endif
>  }
>  #endif
> -- 
> 1.7.1
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[PATCH 7/7] jump label: Add work around to i386 gcc asm goto bug

[Steven Rostedt]

[-- Attachment #1: 0007-jump-label-Add-work-around-to-i386-gcc-asm-goto-bug.patch --]
[-- Type: text/plain, Size: 4982 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

On i386 (not x86_64) early implementations of gcc would have a bug
with asm goto causing it to produce code like the following:

(This was noticed by Peter Zijlstra)

   56 pushl 0
   67 nopl         jmp 0x6f
      popl
      jmp 0x8c

   6f              mov
                   test
                   je 0x8c

   8c mov
      call *(%esp)

The jump added in the asm goto skipped over the popl that matched
the pushl 0, which lead up to a quick crash of the system when
the jump was enabled. The nopl is defined in the asm goto () statement
and when tracepoints are enabled, the nop changes to a jump to the label
that was specified by the asm goto. asm goto is suppose to tell gcc that
the code in the asm might jump to an external label. Here gcc obviously
fails to make that work.

The bug report for gcc is here:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226

The bug only appears on x86 when not compiled with
-maccumulate-outgoing-args. This option is always set on x86_64 and it
is also the work around for a function graph tracer i386 bug.
(See commit: 746357d6a526d6da9d89a2ec645b28406e959c2e)
This explains why the bug only showed up on i386 when function graph
tracer was not enabled.

This patch now adds a CONFIG_JUMP_LABEL option that is default
off instead of using jump labels by default. When jump labels are
enabled, the -maccumulate-outgoing-args will be used (causing a
slightly larger kernel image on i386). This option will exist
until we have a way to detect if the gcc compiler in use is safe
to use on all configurations without the work around.

Note, there exists such a test, but for now we will keep the enabling
of jump label as a manual option.

Archs that know the compiler is safe with asm goto, may choose to
select JUMP_LABEL and enable it by default.

Reported-by: Ingo Molnar <mingo@elte.hu>
Cause-discovered-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Baron <jbaron@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: David Daney <ddaney@caviumnetworks.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: David Miller <davem@davemloft.net>
Cc: Richard Henderson <rth@redhat.com>
LKML-Reference: <1288028746.3673.11.camel@laptop>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/Kconfig               |   14 ++++++++++++++
 arch/x86/Makefile_32.cpu   |   13 ++++++++++++-
 include/linux/jump_label.h |    2 +-
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 53d7f61..8bf0fa6 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -42,6 +42,20 @@ config KPROBES
 	  for kernel debugging, non-intrusive instrumentation and testing.
 	  If in doubt, say "N".
 
+config JUMP_LABEL
+       bool "Optimize trace point call sites"
+       depends on HAVE_ARCH_JUMP_LABEL
+       help
+         If it is detected that the compiler has support for "asm goto",
+	 the kernel will compile trace point locations with just a
+	 nop instruction. When trace points are enabled, the nop will
+	 be converted to a jump to the trace function. This technique
+	 lowers overhead and stress on the branch prediction of the
+	 processor.
+
+	 On i386, options added to the compiler flags may increase
+	 the size of the kernel slightly.
+
 config OPTPROBES
 	def_bool y
 	depends on KPROBES && HAVE_OPTPROBES
diff --git a/arch/x86/Makefile_32.cpu b/arch/x86/Makefile_32.cpu
index 1255d95..f2ee1ab 100644
--- a/arch/x86/Makefile_32.cpu
+++ b/arch/x86/Makefile_32.cpu
@@ -51,7 +51,18 @@ cflags-$(CONFIG_X86_GENERIC) 	+= $(call tune,generic,$(call tune,i686))
 # prologue (push %ebp, mov %esp, %ebp) which breaks the function graph
 # tracer assumptions. For i686, generic, core2 this is set by the
 # compiler anyway
-cflags-$(CONFIG_FUNCTION_GRAPH_TRACER) += $(call cc-option,-maccumulate-outgoing-args)
+ifeq ($(CONFIG_FUNCTION_GRAPH_TRACER), y)
+ADD_ACCUMULATE_OUTGOING_ARGS := y
+endif
+
+# Work around to a bug with asm goto with first implementations of it
+# in gcc causing gcc to mess up the push and pop of the stack in some
+# uses of asm goto.
+ifeq ($(CONFIG_JUMP_LABEL), y)
+ADD_ACCUMULATE_OUTGOING_ARGS := y
+endif
+
+cflags-$(ADD_ACCUMULATE_OUTGOING_ARGS) += $(call cc-option,-maccumulate-outgoing-args)
 
 # Bug fix for binutils: this option is required in order to keep
 # binutils from generating NOPL instructions against our will.
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 1947a12..7880f18 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -1,7 +1,7 @@
 #ifndef _LINUX_JUMP_LABEL_H
 #define _LINUX_JUMP_LABEL_H
 
-#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
 # include <asm/jump_label.h>
 # define HAVE_JUMP_LABEL
 #endif
-- 
1.7.1

Re: [PATCH 7/7] jump label: Add work around to i386 gcc asm goto bug

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> On i386 (not x86_64) early implementations of gcc would have a bug
> with asm goto causing it to produce code like the following:
> 
> (This was noticed by Peter Zijlstra)
> 
>    56 pushl 0
>    67 nopl         jmp 0x6f
>       popl
>       jmp 0x8c
> 
>    6f              mov
>                    test
>                    je 0x8c
> 
>    8c mov
>       call *(%esp)
> 
> The jump added in the asm goto skipped over the popl that matched
> the pushl 0, which lead up to a quick crash of the system when
> the jump was enabled. The nopl is defined in the asm goto () statement
> and when tracepoints are enabled, the nop changes to a jump to the label
> that was specified by the asm goto. asm goto is suppose to tell gcc that
> the code in the asm might jump to an external label. Here gcc obviously
> fails to make that work.
> 
> The bug report for gcc is here:
> 
>   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
> 
> The bug only appears on x86 when not compiled with
> -maccumulate-outgoing-args. This option is always set on x86_64 and it
> is also the work around for a function graph tracer i386 bug.
> (See commit: 746357d6a526d6da9d89a2ec645b28406e959c2e)
> This explains why the bug only showed up on i386 when function graph
> tracer was not enabled.
> 
> This patch now adds a CONFIG_JUMP_LABEL option that is default
> off instead of using jump labels by default. When jump labels are
> enabled, the -maccumulate-outgoing-args will be used (causing a
> slightly larger kernel image on i386). This option will exist
> until we have a way to detect if the gcc compiler in use is safe
> to use on all configurations without the work around.
> 
> Note, there exists such a test, but for now we will keep the enabling
> of jump label as a manual option.
> 
> Archs that know the compiler is safe with asm goto, may choose to
> select JUMP_LABEL and enable it by default.

Looks good. You can add my

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

if you feel like it.

Thanks,

Mathieu

> 
> Reported-by: Ingo Molnar <mingo@elte.hu>
> Cause-discovered-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Baron <jbaron@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: David Daney <ddaney@caviumnetworks.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: David Miller <davem@davemloft.net>
> Cc: Richard Henderson <rth@redhat.com>
> LKML-Reference: <1288028746.3673.11.camel@laptop>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  arch/Kconfig               |   14 ++++++++++++++
>  arch/x86/Makefile_32.cpu   |   13 ++++++++++++-
>  include/linux/jump_label.h |    2 +-
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 53d7f61..8bf0fa6 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -42,6 +42,20 @@ config KPROBES
>  	  for kernel debugging, non-intrusive instrumentation and testing.
>  	  If in doubt, say "N".
>  
> +config JUMP_LABEL
> +       bool "Optimize trace point call sites"
> +       depends on HAVE_ARCH_JUMP_LABEL
> +       help
> +         If it is detected that the compiler has support for "asm goto",
> +	 the kernel will compile trace point locations with just a
> +	 nop instruction. When trace points are enabled, the nop will
> +	 be converted to a jump to the trace function. This technique
> +	 lowers overhead and stress on the branch prediction of the
> +	 processor.
> +
> +	 On i386, options added to the compiler flags may increase
> +	 the size of the kernel slightly.
> +
>  config OPTPROBES
>  	def_bool y
>  	depends on KPROBES && HAVE_OPTPROBES
> diff --git a/arch/x86/Makefile_32.cpu b/arch/x86/Makefile_32.cpu
> index 1255d95..f2ee1ab 100644
> --- a/arch/x86/Makefile_32.cpu
> +++ b/arch/x86/Makefile_32.cpu
> @@ -51,7 +51,18 @@ cflags-$(CONFIG_X86_GENERIC) 	+= $(call tune,generic,$(call tune,i686))
>  # prologue (push %ebp, mov %esp, %ebp) which breaks the function graph
>  # tracer assumptions. For i686, generic, core2 this is set by the
>  # compiler anyway
> -cflags-$(CONFIG_FUNCTION_GRAPH_TRACER) += $(call cc-option,-maccumulate-outgoing-args)
> +ifeq ($(CONFIG_FUNCTION_GRAPH_TRACER), y)
> +ADD_ACCUMULATE_OUTGOING_ARGS := y
> +endif
> +
> +# Work around to a bug with asm goto with first implementations of it
> +# in gcc causing gcc to mess up the push and pop of the stack in some
> +# uses of asm goto.
> +ifeq ($(CONFIG_JUMP_LABEL), y)
> +ADD_ACCUMULATE_OUTGOING_ARGS := y
> +endif
> +
> +cflags-$(ADD_ACCUMULATE_OUTGOING_ARGS) += $(call cc-option,-maccumulate-outgoing-args)
>  
>  # Bug fix for binutils: this option is required in order to keep
>  # binutils from generating NOPL instructions against our will.
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 1947a12..7880f18 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -1,7 +1,7 @@
>  #ifndef _LINUX_JUMP_LABEL_H
>  #define _LINUX_JUMP_LABEL_H
>  
> -#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
>  # include <asm/jump_label.h>
>  # define HAVE_JUMP_LABEL
>  #endif
> -- 
> 1.7.1
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 0/7] [GIT PULL] jump label: fixes and work arounds

[Ingo Molnar]

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

> Ingo,
> 
> Please pull the latest tip/perf/jump-label-2 tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/perf/jump-label-2
> 
> 
> David Miller (1):
>       jump_label: Fix unaligned traps on sparc.
> 
> H. Peter Anvin (1):
>       x86, ftrace: Use safe noops, drop trap test
> 
> Jason Baron (2):
>       jump label: Fix module __init section race
>       jump label: Fix deadlock b/w jump_label_mutex vs. text_mutex
> 
> Steven Rostedt (3):
>       jump label: Fix error with preempt disable holding mutex
>       jump label: Make arch_jump_label_text_poke_early() optional
>       jump label: Add work around to i386 gcc asm goto bug
> 
> ----
>  arch/Kconfig                        |   14 ++++++
>  arch/sparc/include/asm/jump_label.h |    1 +
>  arch/x86/Makefile_32.cpu            |   13 +++++-
>  arch/x86/kernel/alternative.c       |   69 +++++++------------------------
>  include/linux/jump_label.h          |    7 +++-
>  kernel/jump_label.c                 |   77 ++++++++++++++++++++++++++++++-----
>  kernel/kprobes.c                    |   26 +++++++-----
>  7 files changed, 130 insertions(+), 77 deletions(-)

Pulled, thanks a lot Steve!

	Ingo