Immediate values for fast branches

i386 optimization of the immediate values which uses a movl with code patching to set/unset the value used to populate the register used for the branch test.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 arch/i386/kernel/Makefile    |    1 
 arch/i386/kernel/immediate.c |  177 +++++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/immediate.h |   72 +++++++++++++++++
 3 files changed, 249 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/include/asm-i386/immediate.h
===================================================================

include/asm-i386/immediate.h +22 -1

@@ -1 +1,71 @@
-#include <asm-generic/immediate.h>
+#ifndef _ASM_I386_IMMEDIATE_H
+#define _ASM_I386_IMMEDIATE_H
+
+/*
+ * Immediate values. i386 architecture optimizations.
+ *
+ * (C) Copyright 2006 Mathieu Desnoyers <[email protected]>
+ *
+ * This file is released under the GPLv2.
+ * See the file COPYING for more details.
+ */
+
+#define IF_DEFAULT (IF_OPTIMIZED | IF_LOCKDEP)
+
+/*
+ * Optimized version of the immediate. Passing the flags as a pointer to
+ * the inline assembly to trick it into putting the flags value as third
+ * parameter in the structure.
+ */
+#define immediate_optimized(flags, var)					\
+	({								\
+		int condition;						\

include/asm-i386/immediate.h +39 -0

+	})
+
+/*
+ * immediate macro selecting the generic or optimized version of immediate,
+ * depending on the flags specified. It is a macro because we need to pass the
+ * name to immediate_optimized() and immediate_generic() so they can declare a
+ * static variable with it.
+ */
+#define _immediate(flags, var)						\
+({									\
+	(((flags) & IF_LOCKDEP) && ((flags) & IF_OPTIMIZED)) ?		\
+		immediate_optimized(flags, var) :			\
+		immediate_generic(flags, var);				\
+})
+
+/* immediate with default behavior */
+#define immediate(var)	_immediate(IF_DEFAULT, var)
+
+/*
+ * Architecture dependant immediate information, used internally for immediate
+ * activation.
+ */
+
+/*
+ * Offset of the immediate value from the start of the movl instruction, in
+ * bytes. We point to the first lower byte of the 4 bytes immediate value. Only
+ * changing one byte makes sure we do an atomic memory write, independently of
+ * the alignment of the 4 bytes in the load immediate instruction.
+ */
+#define IMMEDIATE_OPTIMIZED_ENABLE_IMMEDIATE_OFFSET 1
+#define IMMEDIATE_OPTIMIZED_ENABLE_TYPE unsigned char
+/* Dereference enable as lvalue from a pointer to its instruction */
+#define IMMEDIATE_OPTIMIZED_ENABLE(a)					\
+	(*(IMMEDIATE_OPTIMIZED_ENABLE_TYPE*)				\
+		((char*)(a)+IMMEDIATE_OPTIMIZED_ENABLE_IMMEDIATE_OFFSET))
+
+extern int immediate_optimized_set_enable(void *address, char enable);
+
+#endif /* _ASM_I386_IMMEDIATE_H */

Index: linux-2.6-lttng/arch/i386/kernel/Makefile
===================================================================

arch/i386/kernel/Makefile +1 -0

@@ -35,6 +35,7 @@
 obj-y				+= sysenter.o vsyscall.o
 obj-$(CONFIG_ACPI_SRAT) 	+= srat.o
 obj-$(CONFIG_EFI) 		+= efi.o efi_stub.o
+obj-$(CONFIG_IMMEDIATE)		+= immediate.o
 obj-$(CONFIG_DOUBLEFAULT) 	+= doublefault.o
 obj-$(CONFIG_SERIAL_8250)	+= legacy_serial.o
 obj-$(CONFIG_VM86)		+= vm86.o

Index: linux-2.6-lttng/arch/i386/kernel/immediate.c
===================================================================

arch/i386/kernel/immediate.c new file +55 -0

@@ -0,0 +1,177 @@
+/*
+ * Immediate Value - i386 architecture specific code.
+ *
+ * Rationale
+ *
+ * Required because of :
+ * - Erratum 49 fix for Intel PIII.
+ * - Still present on newer processors : Intel Core 2 Duo Processor for Intel
+ *   Centrino Duo Processor Technology Specification Update, AH33.
+ *   Unsynchronized Cross-Modifying Code Operations Can Cause Unexpected
+ *   Instruction Execution Results.
+ *
+ * Permits immediate value modification by XMC with correct serialization.
+ *
+ * Reentrant for NMI and trap handler instrumentation. Permits XMC to a
+ * location that has preemption enabled because it involves no temporary or
+ * reused data structure.
+ *
+ * Quoting Richard J Moore, source of the information motivating this
+ * implementation which differs from the one proposed by Intel which is not
+ * suitable for kernel context (does not support NMI and would require disabling
+ * interrupts on every CPU for a long period) :
+ *
+ * "There is another issue to consider when looking into using probes other
+ * then int3:
+ *
+ * Intel erratum 54 - Unsynchronized Cross-modifying code - refers to the
+ * practice of modifying code on one processor where another has prefetched
+ * the unmodified version of the code. Intel states that unpredictable general
+ * protection faults may result if a synchronizing instruction (iret, int,
+ * int3, cpuid, etc ) is not executed on the second processor before it
+ * executes the pre-fetched out-of-date copy of the instruction.
+ *
+ * When we became aware of this I had a long discussion with Intel's
+ * microarchitecture guys. It turns out that the reason for this erratum
+ * (which incidentally Intel does not intend to fix) is because the trace
+ * cache - the stream of micorops resulting from instruction interpretation -
+ * cannot guaranteed to be valid. Reading between the lines I assume this
+ * issue arises because of optimization done in the trace cache, where it is
+ * no longer possible to identify the original instruction boundaries. If the
+ * CPU discoverers that the trace cache has been invalidated because of
+ * unsynchronized cross-modification then instruction execution will be
+ * aborted with a GPF. Further discussion with Intel revealed that replacing
+ * the first opcode byte with an int3 would not be subject to this erratum.
+ *
+ * So, is cmpxchg reliable? One has to guarantee more than mere atomicity."
+ *
+ * Overall design
+ *
+ * The algorithm proposed by Intel applies not so well in kernel context: it
+ * would imply disabling interrupts and looping on every CPUs while modifying
+ * the code and would not support instrumentation of code called from interrupt
+ * sources that cannot be disabled.
+ *
+ * Therefore, we use a different algorithm to respect Intel's erratum (see the

arch/i386/kernel/immediate.c new file +35 -0

+ *
+ * Mathieu Desnoyers <[email protected]>
+ */
+
+#include <linux/notifier.h>
+#include <linux/preempt.h>
+#include <linux/smp.h>
+#include <linux/notifier.h>
+#include <linux/module.h>
+#include <linux/immediate.h>
+#include <linux/kdebug.h>
+
+#include <asm/cacheflush.h>
+
+#define BREAKPOINT_INSTRUCTION  0xcc
+#define BREAKPOINT_INS_LEN 1
+
+static long target_eip;
+
+static void immediate_synchronize_core(void *info)
+{
+	sync_core();	/* use cpuid to stop speculative execution */
+}
+
+/*
+ * The eip value points right after the breakpoint instruction, in the second
+ * byte of the movb. Incrementing it of 1 byte makes the code resume right after
+ * the movb instruction, effectively skipping this instruction.
+ *
+ * We simply skip the 2 bytes load immediate here, leaving the register in an
+ * undefined state. We don't care about the content (0 or !0), because we are
+ * changing the value 0->1 or 1->0. This small window of undefined value
+ * doesn't matter.
+ */
+static int immediate_notifier(struct notifier_block *nb,

arch/i386/kernel/immediate.c new file +19 -0

+		return NOTIFY_STOP;
+	}
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block immediate_notify = {
+	.notifier_call = immediate_notifier,
+	.priority = 0x7fffffff,	/* we need to be notified first */
+};
+
+/*
+ * The address is not aligned. We can only change 1 byte of the value
+ * atomically.
+ * Must be called with immediate_mutex held.
+ */
+int immediate_optimized_set_enable(void *address, char enable)
+{
+	char saved_byte;
+	int ret;

arch/i386/kernel/immediate.c new file +38 -0

+	target_eip = (long)address + BREAKPOINT_INS_LEN;
+	/* register_die_notifier has memory barriers */
+	register_die_notifier(&immediate_notify);
+	saved_byte = *dest;
+	*dest = BREAKPOINT_INSTRUCTION;
+	wmb();
+	/*
+	 * Execute serializing instruction on each CPU.
+	 * Acts as a memory barrier.
+	 */
+	ret = on_each_cpu(immediate_synchronize_core, NULL, 1, 1);
+	BUG_ON(ret != 0);
+
+	dest[1] = enable;
+	wmb();
+	*dest = saved_byte;
+		/*
+		 * Wait for all int3 handlers to end
+		 * (interrupts are disabled in int3).
+		 * This CPU is clearly not in a int3 handler,
+		 * because int3 handler is not preemptible and
+		 * there cannot be any more int3 handler called
+		 * for this site, because we placed the original
+		 * instruction back.
+		 * synchronize_sched has memory barriers.
+		 */
+	synchronize_sched();
+	unregister_die_notifier(&immediate_notify);
+	/* unregister_die_notifier has memory barriers */
+	target_eip = 0;
+#if defined(CONFIG_DEBUG_RODATA)
+	/* Set this page back to RX */
+	change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_RX);
+	global_flush_tlb();
+#endif
+	return 0;
+}
+EXPORT_SYMBOL_GPL(immediate_optimized_set_enable);

Quoted 40 lines · Mathieu Desnoyers

i386 optimization of the immediate values which uses a movl with code patching to set/unset the value used to populate the register used for the branch test.

Signed-off-by Mathieu Desnoyers <[email protected]>

---
 arch/i386/kernel/Makefile    |    1 
 arch/i386/kernel/immediate.c |  177 +++++++++++++++++++++++++++++++++++++++++++
 include/asm-i386/immediate.h |   72 +++++++++++++++++
 3 files changed, 249 insertions(+), 1 deletion(-)

Index: linux-2.6-lttng/include/asm-i386/immediate.h
===================================================================

include/asm-i386/immediate.h +24 -1

@@ -1 +1,71 @@
-#include <asm-generic/immediate.h>
+#ifndef _ASM_I386_IMMEDIATE_H
+#define _ASM_I386_IMMEDIATE_H
+
+/*
+ * Immediate values. i386 architecture optimizations.
+ *
+ * (C) Copyright 2006 Mathieu Desnoyers <[email protected]>
+ *
+ * This file is released under the GPLv2.
+ * See the file COPYING for more details.
+ */
+
+#define IF_DEFAULT (IF_OPTIMIZED | IF_LOCKDEP)
+
+/*
+ * Optimized version of the immediate. Passing the flags as a pointer to
+ * the inline assembly to trick it into putting the flags value as third
+ * parameter in the structure.
+ */
+#define immediate_optimized(flags, var)					\
+	({								\
+		int condition;						\
+		asm (	".section __immediate, \"a\", @progbits;\n\t"	\
+					".long %1, 0f, %2;\n\t"		\

include/asm-i386/immediate.h line 32

Quoted 8 lines

include/asm-i386/immediate.h +8 -0

+					".previous;\n\t"		\
+					"0:\n\t"			\
+					"movl %3,%0;\n\t"		\
+				: "=r" (condition)			\
+				: "m" (var),				\
+				  "m" (*(char*)flags),			\
+				  "i" (0));				\
+		(condition);						\

Unnecessary parens.

Quoted 115 lines

include/asm-i386/immediate.h +39 -0

+	})
+
+/*
+ * immediate macro selecting the generic or optimized version of immediate,
+ * depending on the flags specified. It is a macro because we need to pass the
+ * name to immediate_optimized() and immediate_generic() so they can declare a
+ * static variable with it.
+ */
+#define _immediate(flags, var)						\
+({									\
+	(((flags) & IF_LOCKDEP) && ((flags) & IF_OPTIMIZED)) ?		\
+		immediate_optimized(flags, var) :			\
+		immediate_generic(flags, var);				\
+})
+
+/* immediate with default behavior */
+#define immediate(var)	_immediate(IF_DEFAULT, var)
+
+/*
+ * Architecture dependant immediate information, used internally for immediate
+ * activation.
+ */
+
+/*
+ * Offset of the immediate value from the start of the movl instruction, in
+ * bytes. We point to the first lower byte of the 4 bytes immediate value. Only
+ * changing one byte makes sure we do an atomic memory write, independently of
+ * the alignment of the 4 bytes in the load immediate instruction.
+ */
+#define IMMEDIATE_OPTIMIZED_ENABLE_IMMEDIATE_OFFSET 1
+#define IMMEDIATE_OPTIMIZED_ENABLE_TYPE unsigned char
+/* Dereference enable as lvalue from a pointer to its instruction */
+#define IMMEDIATE_OPTIMIZED_ENABLE(a)					\
+	(*(IMMEDIATE_OPTIMIZED_ENABLE_TYPE*)				\
+		((char*)(a)+IMMEDIATE_OPTIMIZED_ENABLE_IMMEDIATE_OFFSET))
+
+extern int immediate_optimized_set_enable(void *address, char enable);
+
+#endif /* _ASM_I386_IMMEDIATE_H */

Index: linux-2.6-lttng/arch/i386/kernel/Makefile
===================================================================

arch/i386/kernel/Makefile +1 -0

@@ -35,6 +35,7 @@
 obj-y				+= sysenter.o vsyscall.o
 obj-$(CONFIG_ACPI_SRAT) 	+= srat.o
 obj-$(CONFIG_EFI) 		+= efi.o efi_stub.o
+obj-$(CONFIG_IMMEDIATE)		+= immediate.o
 obj-$(CONFIG_DOUBLEFAULT) 	+= doublefault.o
 obj-$(CONFIG_SERIAL_8250)	+= legacy_serial.o
 obj-$(CONFIG_VM86)		+= vm86.o

Index: linux-2.6-lttng/arch/i386/kernel/immediate.c
===================================================================

arch/i386/kernel/immediate.c new file +59 -0

@@ -0,0 +1,177 @@
+/*
+ * Immediate Value - i386 architecture specific code.
+ *
+ * Rationale
+ *
+ * Required because of :
+ * - Erratum 49 fix for Intel PIII.
+ * - Still present on newer processors : Intel Core 2 Duo Processor for Intel
+ *   Centrino Duo Processor Technology Specification Update, AH33.
+ *   Unsynchronized Cross-Modifying Code Operations Can Cause Unexpected
+ *   Instruction Execution Results.
+ *
+ * Permits immediate value modification by XMC with correct serialization.
+ *
+ * Reentrant for NMI and trap handler instrumentation. Permits XMC to a
+ * location that has preemption enabled because it involves no temporary or
+ * reused data structure.
+ *
+ * Quoting Richard J Moore, source of the information motivating this
+ * implementation which differs from the one proposed by Intel which is not
+ * suitable for kernel context (does not support NMI and would require disabling
+ * interrupts on every CPU for a long period) :
+ *
+ * "There is another issue to consider when looking into using probes other
+ * then int3:
+ *
+ * Intel erratum 54 - Unsynchronized Cross-modifying code - refers to the
+ * practice of modifying code on one processor where another has prefetched
+ * the unmodified version of the code. Intel states that unpredictable general
+ * protection faults may result if a synchronizing instruction (iret, int,
+ * int3, cpuid, etc ) is not executed on the second processor before it
+ * executes the pre-fetched out-of-date copy of the instruction.
+ *
+ * When we became aware of this I had a long discussion with Intel's
+ * microarchitecture guys. It turns out that the reason for this erratum
+ * (which incidentally Intel does not intend to fix) is because the trace
+ * cache - the stream of micorops resulting from instruction interpretation -
+ * cannot guaranteed to be valid. Reading between the lines I assume this
+ * issue arises because of optimization done in the trace cache, where it is
+ * no longer possible to identify the original instruction boundaries. If the
+ * CPU discoverers that the trace cache has been invalidated because of
+ * unsynchronized cross-modification then instruction execution will be
+ * aborted with a GPF. Further discussion with Intel revealed that replacing
+ * the first opcode byte with an int3 would not be subject to this erratum.
+ *
+ * So, is cmpxchg reliable? One has to guarantee more than mere atomicity."
+ *
+ * Overall design
+ *
+ * The algorithm proposed by Intel applies not so well in kernel context: it
+ * would imply disabling interrupts and looping on every CPUs while modifying
+ * the code and would not support instrumentation of code called from interrupt
+ * sources that cannot be disabled.
+ *
+ * Therefore, we use a different algorithm to respect Intel's erratum (see the
+ * quoted discussion above). We make sure that no CPU sees an out-of-date copy
+ * of a pre-fetched instruction by 1 - using a breakpoint, which skips the
+ * instruction that is going to be modified, 2 - issuing an IPI to every CPU to
+ * execute a sync_core(), to make sure that even when the breakpoint is removed,

arch/i386/kernel/immediate.c line 65

Quoted 6 lines

arch/i386/kernel/immediate.c new file +6 -0

+ * no cpu could possibly still have the out-of-date copy of the instruction,
+ * modify the now unused 2nd byte of the instruction, and then put back the
+ * original 1st byte of the instruction.
+ *
+ * It has exactly the same intent as the algorithm proposed by Intel, but
+ * it has less side-effects, scales better and supports NMI, SMI and MCE.

Algorithm looks plausible, was it really tested?

Quoted 35 lines

arch/i386/kernel/immediate.c new file +35 -0

+ *
+ * Mathieu Desnoyers <[email protected]>
+ */
+
+#include <linux/notifier.h>
+#include <linux/preempt.h>
+#include <linux/smp.h>
+#include <linux/notifier.h>
+#include <linux/module.h>
+#include <linux/immediate.h>
+#include <linux/kdebug.h>
+
+#include <asm/cacheflush.h>
+
+#define BREAKPOINT_INSTRUCTION  0xcc
+#define BREAKPOINT_INS_LEN 1
+
+static long target_eip;
+
+static void immediate_synchronize_core(void *info)
+{
+	sync_core();	/* use cpuid to stop speculative execution */
+}
+
+/*
+ * The eip value points right after the breakpoint instruction, in the second
+ * byte of the movb. Incrementing it of 1 byte makes the code resume right after
+ * the movb instruction, effectively skipping this instruction.
+ *
+ * We simply skip the 2 bytes load immediate here, leaving the register in an
+ * undefined state. We don't care about the content (0 or !0), because we are
+ * changing the value 0->1 or 1->0. This small window of undefined value
+ * doesn't matter.
+ */
+static int immediate_notifier(struct notifier_block *nb,

arch/i386/kernel/immediate.c line 110

Quoted 10 lines

arch/i386/kernel/immediate.c new file +10 -0

+	unsigned long val, void *data)
+{
+	enum die_val die_val = (enum die_val) val;
+	struct die_args *args = data;
+
+	if (!args->regs || user_mode_vm(args->regs))
+		return NOTIFY_DONE;
+
+	if (die_val == DIE_INT3	&& args->regs->eip == target_eip) {
+		args->regs->eip += 1; /* Skip the next byte of load immediate */

<nitpick>

		args->regs->eip++;

Quoted 24 lines

arch/i386/kernel/immediate.c new file +24 -0

+		return NOTIFY_STOP;
+	}
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block immediate_notify = {
+	.notifier_call = immediate_notifier,
+	.priority = 0x7fffffff,	/* we need to be notified first */
+};
+
+/*
+ * The address is not aligned. We can only change 1 byte of the value
+ * atomically.
+ * Must be called with immediate_mutex held.
+ */
+int immediate_optimized_set_enable(void *address, char enable)
+{
+	char saved_byte;
+	int ret;
+	char *dest = address;
+
+	if (!(enable ^ dest[1])) /* Must be a state change 0<->1 to execute */
+		return 0;
+

arch/i386/kernel/immediate.c line 139

Quoted 5 lines

arch/i386/kernel/immediate.c new file +5 -0

+#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
+	/* Make sure this page is writable */
+	change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
+	global_flush_tlb();
+#endif

Can't we have a macro or inline to do this, and the setting back to read-only? kprobes also has the same ugly #if blocks...

Hmm, what happens if you race with kprobes setting a probe on the same page? Couldn't it unexpectedly become read-only?

Quoted 39 lines

arch/i386/kernel/immediate.c new file +38 -0

+	target_eip = (long)address + BREAKPOINT_INS_LEN;
+	/* register_die_notifier has memory barriers */
+	register_die_notifier(&immediate_notify);
+	saved_byte = *dest;
+	*dest = BREAKPOINT_INSTRUCTION;
+	wmb();
+	/*
+	 * Execute serializing instruction on each CPU.
+	 * Acts as a memory barrier.
+	 */
+	ret = on_each_cpu(immediate_synchronize_core, NULL, 1, 1);
+	BUG_ON(ret != 0);
+
+	dest[1] = enable;
+	wmb();
+	*dest = saved_byte;
+		/*
+		 * Wait for all int3 handlers to end
+		 * (interrupts are disabled in int3).
+		 * This CPU is clearly not in a int3 handler,
+		 * because int3 handler is not preemptible and
+		 * there cannot be any more int3 handler called
+		 * for this site, because we placed the original
+		 * instruction back.
+		 * synchronize_sched has memory barriers.
+		 */
+	synchronize_sched();
+	unregister_die_notifier(&immediate_notify);
+	/* unregister_die_notifier has memory barriers */
+	target_eip = 0;
+#if defined(CONFIG_DEBUG_RODATA)
+	/* Set this page back to RX */
+	change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_RX);
+	global_flush_tlb();
+#endif
+	return 0;
+}
+EXPORT_SYMBOL_GPL(immediate_optimized_set_enable);

Quoted 19 lines

arch/i386/kernel/immediate.c new file +19 -0

+		return NOTIFY_STOP;
+	}
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block immediate_notify = {
+	.notifier_call = immediate_notifier,
+	.priority = 0x7fffffff,	/* we need to be notified first */
+};
+
+/*
+ * The address is not aligned. We can only change 1 byte of the value
+ * atomically.
+ * Must be called with immediate_mutex held.
+ */
+int immediate_optimized_set_enable(void *address, char enable)
+{
+	char saved_byte;
+	int ret;

arch/i386/kernel/immediate.c line 139

Quoted 10 lines

arch/i386/kernel/immediate.c new file +10 -0

+	char *dest = address;
+
+	if (!(enable ^ dest[1])) /* Must be a state change 0<->1 to execute */
+		return 0;
+
+#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
+	/* Make sure this page is writable */
+	change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
+	global_flush_tlb();
+#endif

Can't we have a macro or inline to do this, and the setting back to read-only? kprobes also has the same ugly #if blocks...

Hmm, what happens if you race with kprobes setting a probe on the same page? Couldn't it unexpectedly become read-only?

arch/i386/kernel/immediate.c line 139

Quoted 5 lines

arch/i386/kernel/immediate.c new file +5 -0

+#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
+ /* Make sure this page is writable */
+ change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
+ global_flush_tlb();
+#endif

Quoted 32 lines · * Chuck Ebbert ([email protected]) wrote:

Quoted 19 lines

arch/i386/kernel/immediate.c new file +19 -0

+		return NOTIFY_STOP;
+	}
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block immediate_notify = {
+	.notifier_call = immediate_notifier,
+	.priority = 0x7fffffff,	/* we need to be notified first */
+};
+
+/*
+ * The address is not aligned. We can only change 1 byte of the value
+ * atomically.
+ * Must be called with immediate_mutex held.
+ */
+int immediate_optimized_set_enable(void *address, char enable)
+{
+	char saved_byte;
+	int ret;

arch/i386/kernel/immediate.c line 139

Quoted 10 lines

arch/i386/kernel/immediate.c new file +10 -0

+	char *dest = address;
+
+	if (!(enable ^ dest[1])) /* Must be a state change 0<->1 to execute */
+		return 0;
+
+#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
+	/* Make sure this page is writable */
+	change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
+	global_flush_tlb();
+#endif

Can't we have a macro or inline to do this, and the setting back to read-only? kprobes also has the same ugly #if blocks...

Quoted 3 lines

Hmm, what happens if you race with kprobes setting a probe on the same page? Couldn't it unexpectedly become read-only?

Hi Chuck,

I am looking more closely at kprobes; a few comments while we are here:

1 - Why is kprobe_count an atomic_t variable instead of a simple integer? It is always protected by the kprobe_mutex anyway. All this synchronization seems redundant.

2 - I wonder where is the equivalent of my snippet in kprobes code:

Quoted 5 lines

arch/i386/kernel/immediate.c line 139

Quoted 5 lines

arch/i386/kernel/immediate.c new file +5 -0

+#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
+ /* Make sure this page is writable */
+ change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
+ global_flush_tlb();
+#endif

I fancy it's done by the kprobe_page_fault handler, but I do not see clearly how writing the breakpoint from arch_arm_kprobe() in non-writeable memory is done.

In any case, I would like not to use that kind of approach; I prefer to set the memory page to RWX before doing the memory write so I do not depend on the page fault handler (remember that I instrument the page fault handler itself...).

Maybe we could use a shared "text_mutex" between kprobes and immediate values to insure mutual exclusion for .text modification. However, we would still have the following coherency issue when an immediate value and a kprobe share the same address:

1- enable immediate value
2- put a kprobe at the immediate value load instruction address 3- disable immediate value
4- remove kprobe

The kprobe removal would put back the load immediate instruction and would not touch the loaded value (which is ok). However, the instruction copy kept by kprobes would not be in sync with the immediate value state:

Scenario 1: kprobes int3 handler first:

1- enable immediate value
2- put a kprobe at the immediate value load instruction address

-> int3 triggered
kprobe handler runs. Single-steps the "enabled" state.

3- disable immediate value

-> int3 triggered
kprobe handler runs. Single-steps the "enabled" state. This state is wrong.

4- remove kprobe

Scenario 2: immediate value int3 handler first:

1- enable immediate value
2- put a kprobe at the immediate value load instruction address

-> int3 triggered
kprobe handler runs. Single-steps the "enabled" state.

3- disable immediate value
-> int3 triggered (while we disable)
While we disable, the immediate value int3 handler is executed first. It would cause the kprobe handler not to be called, and no "missing" counter would be incremented.

kprobe handler runs. Single-steps the "enabled" state. This state is wrong.

4- remove kprobe

Since I don't want to depend on kprobes to put the breakpoint, because of its reentrancy limitations (see all the __probes functions), It would be good to figure out a mutual exclusion mechanism between immediate values and kprobes. Maybe we could forbid kprobes to insert probes on addresses present in the immediate values tables ? Or better: if we detect this scenario, we could put the kprobe breakpoint at the instruction following the "movl".

Quoted 3 lines

I fancy it's done by the kprobe_page_fault handler, but I do not see clearly how writing the breakpoint from arch_arm_kprobe() in non-writeable memory is done.

Looks like it's not merged yet:

http://lkml.org/lkml/2007/6/7/2

This needs to go in before 2.6.22-final

Quoted 9 lines · On Mon, 18 Jun 2007 14:44:57 -0400 Chuck Ebbert <[email protected]> wrote:

Quoted 3 lines

I fancy it's done by the kprobe_page_fault handler, but I do not see clearly how writing the breakpoint from arch_arm_kprobe() in non-writeable memory is done.

Looks like it's not merged yet:

http://lkml.org/lkml/2007/6/7/2

This needs to go in before 2.6.22-final

Andi, I'll include the below two patches in the next batch, OK?

From: "S. P. Prasanna" <[email protected]>

Fix the problem of page protection introduced by CONFIG_DEBUG_RODATA for x86_64 architecture. As per Andi Kleen's suggestion, the kernel text pages are marked writeable only for a short duration to insert or remove the breakpoints.

Signed-off-by Prasanna S P<[email protected]>

Acked-by Jim Keniston <[email protected]>

Cc Andi Kleen <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 arch/x86_64/kernel/kprobes.c |   26 ++++++++++++++++++++++++++
 arch/x86_64/mm/init.c        |    6 +++++-
 include/asm-x86_64/kprobes.h |   10 ++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff -puN arch/x86_64/kernel/kprobes.c~kprobes-x86_64-fix-for-mark-ro-data arch/x86_64/kernel/kprobes.c

arch/x86_64/kernel/kprobes.c renamed from arch/x86_64/kernel/kprobes.c~kprobes-x86_64-fix-for-mark-ro-data +26 -0

@@ -209,16 +209,42 @@ static void __kprobes arch_copy_kprobe(s
 
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
+	unsigned long addr = (unsigned long)p->addr;
+	int page_readonly = 0;
+
+	if (kernel_readonly_text(addr)) {
+		change_page_attr_addr(addr, 1, PAGE_KERNEL_EXEC);
+		global_flush_tlb();
+		page_readonly = 1;
+	}
 	*p->addr = BREAKPOINT_INSTRUCTION;
 	flush_icache_range((unsigned long) p->addr,
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
+	if (page_readonly) {
+		change_page_attr_addr(addr, 1, PAGE_KERNEL_RO);
+		global_flush_tlb();
+	}
 }
 
 void __kprobes arch_disarm_kprobe(struct kprobe *p)
 {
+	unsigned long addr = (unsigned long)p->addr;
+	int page_readonly = 0;
+
+	if (kernel_readonly_text(addr)) {
+		change_page_attr_addr(addr, 1, PAGE_KERNEL_EXEC);
+		global_flush_tlb();
+		page_readonly = 1;
+	}
+
 	*p->addr = p->opcode;
 	flush_icache_range((unsigned long) p->addr,
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
+
+	if (page_readonly) {
+		change_page_attr_addr(addr, 1, PAGE_KERNEL_RO);
+		global_flush_tlb();
+	}
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)

diff -puN arch/x86_64/mm/init.c~kprobes-x86_64-fix-for-mark-ro-data arch/x86_64/mm/init.c

arch/x86_64/mm/init.c renamed from arch/x86_64/mm/init.c~kprobes-x86_64-fix-for-mark-ro-data +5 -1

@@ -48,6 +48,7 @@
 #define Dprintk(x...)
 #endif
 
+int kernel_text_is_ro;
 const struct dma_mapping_ops* dma_ops;
 EXPORT_SYMBOL(dma_ops);
 

@@ -600,10 +601,13 @@ void mark_rodata_ro(void)
 {
 	unsigned long start = (unsigned long)_stext, end;
 
+	kernel_text_is_ro = 1;
 #ifdef CONFIG_HOTPLUG_CPU
 	/* It must still be possible to apply SMP alternatives. */
-	if (num_possible_cpus() > 1)
+	if (num_possible_cpus() > 1) {
 		start = (unsigned long)_etext;
+		kernel_text_is_ro = 0;
+	}
 #endif
 	end = (unsigned long)__end_rodata;
 	start = (start + PAGE_SIZE - 1) & PAGE_MASK;

diff -puN include/asm-x86_64/kprobes.h~kprobes-x86_64-fix-for-mark-ro-data include/asm-x86_64/kprobes.h

include/asm-x86_64/kprobes.h renamed from include/asm-x86_64/kprobes.h~kprobes-x86_64-fix-for-mark-ro-data +10 -0

@@ -26,6 +26,7 @@
 #include <linux/types.h>
 #include <linux/ptrace.h>
 #include <linux/percpu.h>
+#include <asm-generic/sections.h>
 
 #define  __ARCH_WANT_KPROBES_INSN_SLOT
 

@@ -88,4 +89,13 @@ extern int kprobe_handler(struct pt_regs
 
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);
+extern int kernel_text_is_ro;
+static inline int kernel_readonly_text(unsigned long address)
+{
+	if (kernel_text_is_ro && ((address >= (unsigned long)_stext)
+					&& (address < (unsigned long) _etext)))
+		return 1;
+
+	return 0;
+}
 #endif				/* _ASM_KPROBES_H */

_

From: "S. P. Prasanna" <[email protected]>

Fix the problem of page protection introduced by CONFIG_DEBUG_RODATA. CONFIG_DEBUG_RODATA marks the text pages as read-only, hence kprobes is unable to insert breakpoints in the kernel text. This patch overrides the page protection when adding or removing a probe for the i386 architecture.

Signed-off-by Prasanna S P<[email protected]>

Acked-by Jim Keniston <[email protected]>

Cc Andi Kleen <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 arch/i386/kernel/kprobes.c |   26 ++++++++++++++++++++++++++
 arch/i386/mm/init.c        |    2 ++
 include/asm-i386/kprobes.h |   12 ++++++++++++
 include/asm-i386/pgtable.h |    2 ++
 4 files changed, 42 insertions(+)

diff -puN arch/i386/kernel/kprobes.c~kprobes-i386-fix-for-mark-ro-data arch/i386/kernel/kprobes.c

arch/i386/kernel/kprobes.c renamed from arch/i386/kernel/kprobes.c~kprobes-i386-fix-for-mark-ro-data +2 -0

@@ -169,16 +169,42 @@ int __kprobes arch_prepare_kprobe(struct
 
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
+	unsigned long addr = (unsigned long) p->addr;
+	int page_readonly = 0;

arch/i386/kernel/kprobes.c renamed from arch/i386/kernel/kprobes.c~kprobes-i386-fix-for-mark-ro-data +12 -0

 void __kprobes arch_disarm_kprobe(struct kprobe *p)
 {
+	unsigned long addr = (unsigned long) p->addr;
+	int page_readonly = 0;
+
+	if (kernel_readonly_text(addr)) {
+		page_readonly = 1;
+		change_page_attr(virt_to_page(addr), 1, PAGE_KERNEL_RWX);
+		global_flush_tlb();
+	}
 	*p->addr = p->opcode;
 	flush_icache_range((unsigned long) p->addr,
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
+	if (page_readonly) {
+		change_page_attr(virt_to_page(addr), 1, PAGE_KERNEL_RX);
+		global_flush_tlb();
+	}
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)

diff -puN arch/i386/mm/init.c~kprobes-i386-fix-for-mark-ro-data arch/i386/mm/init.c

arch/i386/mm/init.c renamed from arch/i386/mm/init.c~kprobes-i386-fix-for-mark-ro-data +2 -0

@@ -45,6 +45,7 @@
 #include <asm/sections.h>
 #include <asm/paravirt.h>
 
+int kernel_text_is_ro;
 unsigned int __VMALLOC_RESERVE = 128 << 20;
 
 DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);

@@ -808,6 +809,7 @@ void mark_rodata_ro(void)
 		change_page_attr(virt_to_page(start),
 		                 size >> PAGE_SHIFT, PAGE_KERNEL_RX);
 		printk("Write protecting the kernel text: %luk\n", size >> 10);
+		kernel_text_is_ro = 1;
 	}
 
 	start += size;

diff -puN include/asm-i386/kprobes.h~kprobes-i386-fix-for-mark-ro-data include/asm-i386/kprobes.h

include/asm-i386/kprobes.h renamed from include/asm-i386/kprobes.h~kprobes-i386-fix-for-mark-ro-data +2 -0

@@ -26,6 +26,8 @@
  */
 #include <linux/types.h>
 #include <linux/ptrace.h>
+#include <linux/pfn.h>
+#include <asm-generic/sections.h>
 
 #define  __ARCH_WANT_KPROBES_INSN_SLOT
 

@@ -90,4 +92,14 @@ static inline void restore_interrupts(st
 
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);

include/asm-i386/kprobes.h renamed from include/asm-i386/kprobes.h~kprobes-i386-fix-for-mark-ro-data +0 -0

 #endif				/* _ASM_KPROBES_H */

diff -puN include/asm-i386/pgtable.h~kprobes-i386-fix-for-mark-ro-data include/asm-i386/pgtable.h

include/asm-i386/pgtable.h renamed from include/asm-i386/pgtable.h~kprobes-i386-fix-for-mark-ro-data +2 -0

@@ -159,6 +159,7 @@ void paging_init(void);
 extern unsigned long long __PAGE_KERNEL, __PAGE_KERNEL_EXEC;
 #define __PAGE_KERNEL_RO		(__PAGE_KERNEL & ~_PAGE_RW)
 #define __PAGE_KERNEL_RX		(__PAGE_KERNEL_EXEC & ~_PAGE_RW)
+#define __PAGE_KERNEL_RWX		(__PAGE_KERNEL_EXEC | _PAGE_RW)
 #define __PAGE_KERNEL_NOCACHE		(__PAGE_KERNEL | _PAGE_PCD)
 #define __PAGE_KERNEL_LARGE		(__PAGE_KERNEL | _PAGE_PSE)
 #define __PAGE_KERNEL_LARGE_EXEC	(__PAGE_KERNEL_EXEC | _PAGE_PSE)

@@ -167,6 +168,7 @@ extern unsigned long long __PAGE_KERNEL,
 #define PAGE_KERNEL_RO		__pgprot(__PAGE_KERNEL_RO)
 #define PAGE_KERNEL_EXEC	__pgprot(__PAGE_KERNEL_EXEC)
 #define PAGE_KERNEL_RX		__pgprot(__PAGE_KERNEL_RX)
+#define PAGE_KERNEL_RWX		__pgprot(__PAGE_KERNEL_RWX)
 #define PAGE_KERNEL_NOCACHE	__pgprot(__PAGE_KERNEL_NOCACHE)
 #define PAGE_KERNEL_LARGE	__pgprot(__PAGE_KERNEL_LARGE)
 #define PAGE_KERNEL_LARGE_EXEC	__pgprot(__PAGE_KERNEL_LARGE_EXEC)

_

Quoted 4 lines · On Mon, 18 Jun 2007 14:44:57 -0400 Chuck Ebbert <[email protected]> wrote:

Quoted 3 lines

I fancy it's done by the kprobe_page_fault handler, but I do not see clearly how writing the breakpoint from arch_arm_kprobe() in non-writeable memory is done.

Quoted 5 lines

Looks like it's not merged yet:

http://lkml.org/lkml/2007/6/7/2

This needs to go in before 2.6.22-final

Andi, I'll include the below two patches in the next batch, OK?

+extern int kernel_text_is_ro;

But for 2.6.22 as a quick fix it might be better to just make KPROBES dependent on !DEBUG_RODATA. That would be a one liner.

Quoted 7 lines · On Monday 18 June 2007 20:56:32 Andrew Morton wrote:

Quoted 4 lines · On Mon, 18 Jun 2007 14:44:57 -0400 Chuck Ebbert <[email protected]> wrote:

Quoted 3 lines

I fancy it's done by the kprobe_page_fault handler, but I do not see clearly how writing the breakpoint from arch_arm_kprobe() in non-writeable memory is done.

Quoted 7 lines

Quoted 5 lines

Looks like it's not merged yet:

http://lkml.org/lkml/2007/6/7/2

This needs to go in before 2.6.22-final

Andi, I'll include the below two patches in the next batch, OK?

It won't work reliably unless some of the c_p_a() fixes get in first.

Quoted 4 lines

+extern int kernel_text_is_ro;

No externs in .c files

I also don't like kernel_text_is_read_only() much, it would be better to just lookup_address() it and check the write flag.

But for 2.6.22 as a quick fix it might be better to just make KPROBES dependent on !DEBUG_RODATA. That would be a one liner.

This patch is a quick fix for x86_64 to enable kprobes only if DEBUG_RODATA is not configured. Since DEBUG_RODATA markes the kernel text pages as read-only.

Quoted 2 lines · S. P. Prasanna

This patch is a quick fix for x86_64 to enable kprobes only if DEBUG_RODATA is not configured. Since DEBUG_RODATA markes the kernel text pages as read-only.

it does??

I don't seem to be able to find this in the source code.....

Quoted 5 lines · * Arjan van de Ven ([email protected]) wrote:

Quoted 2 lines · S. P. Prasanna

This patch is a quick fix for x86_64 to enable kprobes only if DEBUG_RODATA is not configured. Since DEBUG_RODATA markes the kernel text pages as read-only.

Quoted 4 lines

it does??

I don't seem to be able to find this in the source code.....

See arch/x86_64/mm/init.c:mark_rodata_ro().

Quoted 6 lines · Mathieu Desnoyers

Quoted 5 lines · * Arjan van de Ven ([email protected]) wrote:

Quoted 2 lines · S. P. Prasanna

This patch is a quick fix for x86_64 to enable kprobes only if DEBUG_RODATA is not configured. Since DEBUG_RODATA markes the kernel text pages as read-only.

Quoted 6 lines

Quoted 4 lines

it does??

I don't seem to be able to find this in the source code.....

See arch/x86_64/mm/init.c:mark_rodata_ro().

eh woops

PATCH] x86: tighten kernel image page access rights author
Jan Beulich <[email protected]>

Wed, 2 May 2007 17:27:10 +0000
(19:27 +0200)
committer
Andi Kleen <[email protected]>

Wed, 2 May 2007 17:27:10 +0000
(19:27 +0200)
commit
6fb14755a676282a4e6caa05a08c92db8e45cfff

changed it to include text (even though Andi vetoed that before when I asked for it on grounds of breaking kprobes)... sounds this really wants to be a 2nd config option to seperatedly do code and data.

Please find the quick fix as per your suggestion below.