Linux Kernel Markers

Hi,

You will find, in the following posts, the latest revision of the Linux

Markers. Due to the need some tracing projects (LTTng, SystemTAP) has of

kind of mechanism, it could be nice to consider it for mainstream

The following patches apply on 2.6.20-rc1-git7.

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

Mathieu Desnoyers <[email protected]> wrote on 20/12/2006 23:52:16:

Quoted 3 lines

Hi,

You will find, in the following posts, the latest revision of the Linux

Kernel

Quoted 1 line

Markers. Due to the need some tracing projects (LTTng, SystemTAP) has of

this

Quoted 1 line

kind of mechanism, it could be nice to consider it for mainstream

inclusion.

Quoted 1 line

Quoted 3 lines

The following patches apply on 2.6.20-rc1-git7.

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

Mathiue, FWIW I like this idea. A few years ago I implemented something similar, but that had no explicit clients. Consequently I made my hooks code more generalized than is needed in practice. I do remember that Karim reworked the LTT instrumentation to use hooks and it worked fine.

You've got the same optimizations for x86 by modifying an instruction's immediate operand and thus avoiding a d-cache hit. The only real caveat is the need to avoid the unsynchronised cross modification erratum. Which means that all processors will need to issue a serializing operation before executing a Marker whose state is changed. How is that handled?

One additional thing we did, which might be useful at some future point, was adding a /proc interface. We reflected the current instrumentation though /proc and gave the status of each hook. We even talked about being able to enable or disabled instrumentation by writing to /proc but I don't think we ever implemented this.

It's high time we settled the issue of instrumentation. It gets my vote,

Good luck!

Richard

- -
Richard J Moore
IBM Linux Technology Centre

Quoted 6 lines

You've got the same optimizations for x86 by modifying an instruction's immediate operand and thus avoiding a d-cache hit. The only real caveat is the need to avoid the unsynchronised cross modification erratum. Which means that all processors will need to issue a serializing operation before executing a Marker whose state is changed. How is that handled?

Good catch. I thought that modifying only 1 byte would spare us from this errata, but looking at it in detail tells me than it's not the case.

I see three different ways to address the problem :

3 - First write an int3 instead of the instruction's first byte. The handler
would do the following :
int3_handler :
single-step the original instruction.
iret

Secondly, we call an IPI that does a smp_processor_id() on each CPU and wait for them to complete. It will make sure we execute a synchronizing instruction on every CPU even if we do not execute the trap handler.

Then, we write the new 2 bytes instruction atomically instead of the int3 and immediate value.

[...]
It would be nice to push the study of the kprobes debug trap handler so it can become possible to use it to put breakpoints in trap handlers. For now, kprobes refuses to insert breakpoints in __kprobes marked functions. However, as we instrument specific spots of the functions (not necessarily the function entry), it is sometimes correct to use kprobes on a marker within the function even if it is not correct to use it in the prologue. [...]

Hi Richard,

Quoted 11 lines · * Mathieu Desnoyers ([email protected]) wrote:

Quoted 6 lines

You've got the same optimizations for x86 by modifying an instruction's immediate operand and thus avoiding a d-cache hit. The only real caveat is the need to avoid the unsynchronised cross modification erratum. Which means that all processors will need to issue a serializing operation before executing a Marker whose state is changed. How is that handled?

Good catch. I thought that modifying only 1 byte would spare us from this errata, but looking at it in detail tells me than it's not the case.

I see three different ways to address the problem :

[...]

Quoted 6 lines

3 - First write an int3 instead of the instruction's first byte. The handler
would do the following :
int3_handler :
single-step the original instruction.
iret

Quoted 8 lines

Secondly, we call an IPI that does a smp_processor_id() on each CPU and wait for them to complete. It will make sure we execute a synchronizing instruction on every CPU even if we do not execute the trap handler.

Then, we write the new 2 bytes instruction atomically instead of the int3 and immediate value.

Here is the implementation of my proposal using a slightly enhanced kprobes. I add the ability to single step a different instruction than the original one, and then put the new instruction instead of the original one when removing the kprobe. It is an improvement on the djprobes design : AFAIK, djprobes required the int3 to be executed by _every_ CPU before the instruction could be replaced. It was problematic with rarely used code paths (error handling) and with thread CPU affinity. Comments are welcome.

I noticed that it restrains LTTng by removing the ability to probe do_general_protection, do_nmi, do_trap, do_debug and do_page_fault. hardirq on/off in lockdep.c must also be tweaked to allow local_irq_enable/disable usage within the debug trap handler.

It would be nice to push the study of the kprobes debug trap handler so it can become possible to use it to put breakpoints in trap handlers. For now, kprobes refuses to insert breakpoints in __kprobes marked functions. However, as we instrument specific spots of the functions (not necessarily the function entry), it is sometimes correct to use kprobes on a marker within the function even if it is not correct to use it in the prologue. Insight from the SystemTAP team would be welcome on this kprobe limitation.

Mathieu

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

arch/i386/kernel/kprobes.c +68 -0

@@ -31,6 +31,7 @@
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
 #include <linux/preempt.h>
+#include <linux/kallsyms.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>
 #include <asm/desc.h>

@@ -753,6 +754,73 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 	return 0;
 }
 
+static struct kprobe xmc_kp;
+DEFINE_MUTEX(kprobe_xmc_mutex);
+
+static int xmc_handler_pre(struct kprobe *p, struct pt_regs *regs)
+{
+	return 0;
+}
+
+static void xmc_handler_post(struct kprobe *p, struct pt_regs *regs,
+	unsigned long flags)
+{
+}
+
+static int xmc_handler_fault(struct kprobe *p, struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
+
+static void xmc_synchronize_cpu(void *info)
+{
+	smp_processor_id();
+}
+
+/* Think of it as a memcpy of new into address with safety with regard to 
+ * Intel PIII errata 49. Only valid for modifying a single instruction with
+ * an instruction of the same size or in smaller instructions of the total
+ * same size as the original instruction. */
+int arch_xmc(void *address, char *newi, int size)
+{
+	int ret = 0;
+	char *dest = (char*)address;
+	char str[KSYM_NAME_LEN + 1];
+	unsigned long sym_offs, sym_size;
+	char *modname;
+	const char *sym_name;
+
+	mutex_lock(&kprobe_xmc_mutex);
+	xmc_kp.pre_handler = xmc_handler_pre;
+	xmc_kp.post_handler = xmc_handler_post;
+	xmc_kp.fault_handler = xmc_handler_fault;
+	xmc_kp.addr = address;
+	xmc_kp.symbol_name = NULL;
+	xmc_kp.offset = 0;
+
+	ret = register_kprobe_swapi(&xmc_kp, newi, size);
+	if (ret < 0) {
+		sym_name = kallsyms_lookup((unsigned long)address,
+				&sym_size, &sym_offs,
+				&modname, str);
+		printk("register_kprobe failed for %p in %s:%s returned %d\n",
+			address, modname?modname:"kernel", sym_name, ret);
+		goto end;
+	}
+	/* Copy every bytes of the new instruction except the first one */
+	memcpy(dest+1, newi+1, size-1);
+	flush_icache_range(dest, size);
+	/* Execute serializing instruction on each CPU */
+	ret = on_each_cpu(xmc_synchronize_cpu, NULL, 1, 1);
+	BUG_ON(ret != 0);
+
+	unregister_kprobe(&xmc_kp);
+end:
+	mutex_unlock(&kprobe_xmc_mutex);
+
+	return ret;
+}
+
 int __init arch_init_kprobes(void)
 {
 	return 0;

include/asm-generic/marker.h +1 -1

@@ -18,7 +18,7 @@ struct __mark_marker_c {
 
 struct __mark_marker {
 	const struct __mark_marker_c *cmark;
-	volatile char *enable;
+	char *enable;
 } __attribute__((packed));
 
 #ifdef CONFIG_MARKERS

include/asm-i386/kprobes.h +3 -0

@@ -90,4 +90,7 @@ static inline void restore_interrupts(struct pt_regs *regs)
 
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);
+/* pIII code cross modification which follows erratum 49. */
+#define ARCH_HAS_XMC
+int arch_xmc(void *address, char *newi, int size);
 #endif				/* _ASM_KPROBES_H */

include/asm-i386/marker.h +1 -1

@@ -18,7 +18,7 @@ struct __mark_marker_c {
 
 struct __mark_marker {
 	struct __mark_marker_c *cmark;
-	volatile char *enable;
+	char *enable;
 } __attribute__((packed));
 
 #ifdef CONFIG_MARKERS

include/asm-powerpc/marker.h +1 -1

@@ -20,7 +20,7 @@ struct __mark_marker_c {
 
 struct __mark_marker {
 	struct __mark_marker_c *cmark;
-	volatile short *enable;
+	short *enable;
 } __attribute__((packed));
 
 #ifdef CONFIG_MARKERS

include/linux/kprobes.h +4 -0

@@ -189,6 +189,7 @@ static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
 }
 
 int register_kprobe(struct kprobe *p);
+int register_kprobe_swapi(struct kprobe *p, char *newi, int size);
 void unregister_kprobe(struct kprobe *p);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
 int longjmp_break_handler(struct kprobe *, struct pt_regs *);

@@ -203,6 +204,9 @@ struct kretprobe_instance *get_free_rp_inst(struct kretprobe *rp);
 void add_rp_inst(struct kretprobe_instance *ri);
 void kprobe_flush_task(struct task_struct *tk);
 void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
+/* Architecture specific code cross-modification. Valid for overwriting
+ * a single instruction. Safe for Intel PIII erratum 49. */
+int kprobe_xmc(void *address, char *newi, int size);
 #else /* CONFIG_KPROBES */
 
 #define __kprobes	/**/

include/linux/marker.h +1 -1

@@ -40,7 +40,7 @@
 
 typedef void marker_probe_func(const char *fmt, ...);
 
-#ifndef CONFIG_MARKERS_DISABLE_OPTIMIZATION
+#ifdef CONFIG_MARKERS_ENABLE_OPTIMIZATION
 #include <asm/marker.h>
 #else
 #include <asm-generic/marker.h>

kernel/Kconfig.marker +4 -3

@@ -8,9 +8,10 @@ config MARKERS
 	  Place an empty function call at each marker site. Can be
 	  dynamically changed for a probe function.
 
-config MARKERS_DISABLE_OPTIMIZATION
-	bool "Disable architecture specific marker optimization"
-	depends EMBEDDED
+config MARKERS_ENABLE_OPTIMIZATION
+	bool "Enable marker optimization (EXPERIMENTAL)"
+	depends MARKERS
+	select KPROBES
 	default n
 	help
 	  Disable code replacement jump optimisations. Especially useful if your

kernel/kprobes.c +44 -4

@@ -534,7 +534,7 @@ static int __kprobes in_kprobes_functions(unsigned long addr)
 }
 
 static int __kprobes __register_kprobe(struct kprobe *p,
-	unsigned long called_from)
+	unsigned long called_from, char *newi, int size)
 {
 	int ret = 0;
 	struct kprobe *old_p;

@@ -587,6 +587,10 @@ static int __kprobes __register_kprobe(struct kprobe *p,
 
 	if ((ret = arch_prepare_kprobe(p)) != 0)
 		goto out;
+	if (newi) {
+		memcpy(p->ainsn.insn, newi, size);
+		p->opcode = *newi;
+	}
 
 	INIT_HLIST_NODE(&p->hlist);
 	hlist_add_head_rcu(&p->hlist,

@@ -609,7 +613,14 @@ out:
 int __kprobes register_kprobe(struct kprobe *p)
 {
 	return __register_kprobe(p,
-		(unsigned long)__builtin_return_address(0));
+		(unsigned long)__builtin_return_address(0),
+		NULL, 0);
+}
+
+int __kprobes register_kprobe_swapi(struct kprobe *p, char *newi, int size)
+{
+	return __register_kprobe(p,
+		(unsigned long)__builtin_return_address(0), newi, size);
 }
 
 void __kprobes unregister_kprobe(struct kprobe *p)

@@ -699,7 +710,8 @@ int __kprobes register_jprobe(struct jprobe *jp)
 	jp->kp.break_handler = longjmp_break_handler;
 
 	return __register_kprobe(&jp->kp,
-		(unsigned long)__builtin_return_address(0));
+		(unsigned long)__builtin_return_address(0),
+		NULL, 0);
 }
 
 void __kprobes unregister_jprobe(struct jprobe *jp)

@@ -760,7 +772,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
 	rp->nmissed = 0;
 	/* Establish function entry probe point */
 	if ((ret = __register_kprobe(&rp->kp,
-		(unsigned long)__builtin_return_address(0))) != 0)
+		(unsigned long)__builtin_return_address(0),
+		NULL, 0)) != 0)
 		free_rp_inst(rp);
 	return ret;
 }

@@ -790,6 +803,33 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
 	free_rp_inst(rp);
 }
 
+#ifdef ARCH_HAS_XMC
+int kprobe_xmc(void *address, char *newi, int size)
+{
+	return arch_xmc(address, newi, size);
+}
+#else
+/* Limited XMC : only overwrite an instruction with an atomic operation
+ * (writing at most an aligned long). */
+int kprobe_xmc(void *address, char *newi, int size)
+{
+	if (size > sizeof(long)) {
+		printk("Limited XMC : cannot overwrite instructions bigger "\
+			"than %d. XMC of size %d fails.\n", sizeof(long),
+			size);
+		return -EPERM;
+	}
+	if (hweight32(size) != 1 || address % size != 0) {
+		printk("Limited XMC : cannot write %d bytes unaligned "\
+			"instruction. XMC fails.\n", size);
+		return -EPERM;
+	}
+	memcpy(address, newi, size);
+	flush_icache_range(address, size);
+}
+#endif /* HAS_ARCH_XMC */
+EXPORT_SYMBOL(kprobe_xmc);
+
 static int __init init_kprobes(void)
 {
 	int i, err = 0;

kernel/module.c +30 -28

@@ -40,6 +40,7 @@
 #include <linux/string.h>
 #include <linux/mutex.h>
 #include <linux/unwind.h>
+#include <linux/kprobes.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 #include <asm/cacheflush.h>

@@ -309,6 +310,26 @@ EXPORT_SYMBOL(__mark_empty_function);
 #define MARK_ENABLE_OFFSET(a) \
 	(typeof(a))((char*)a+MARK_ENABLE_IMMEDIATE_OFFSET)
 
+/* wmb() stay ordered. State will be ok for interrupts/other CPUs. */
+#ifdef MARK_POLYMORPHIC
+static int marker_set_enable(char *address, char enable)
+{
+	char newi[MARK_ENABLE_IMMEDIATE_OFFSET+1];
+
+	memcpy(newi, address, MARK_ENABLE_IMMEDIATE_OFFSET+1);
+	*MARK_ENABLE_OFFSET(&newi[0]) = enable;
+	return kprobe_xmc(address, newi, MARK_ENABLE_IMMEDIATE_OFFSET+1);
+}
+#else
+static int marker_set_enable(char *address, char enable)
+{
+	*MARK_ENABLE_OFFSET(address) = enable;
+	return 0;
+}
+#endif /* MARK_POLYMORPHIC */
+
+/* enable and function address are set out of order, and it's ok :
+ * the state is always coherent. */
 static int marker_set_probe_range(const char *name, 
 	const char *format,
 	marker_probe_func *probe,

@@ -336,13 +357,7 @@ static int marker_set_probe_range(const char *name,
 					*iter->cmark->call =
 						__mark_empty_function;
 				}
-				/* Can have many enables for one function */
-				*MARK_ENABLE_OFFSET(iter->enable) = 0;
-#ifdef MARK_POLYMORPHIC
-				flush_icache_range(
-					MARK_ENABLE_OFFSET(iter->enable),
-					sizeof(*(iter->enable)));
-#endif
+				marker_set_enable(iter->enable, 0);
 			} else {
 				if (*iter->cmark->call
 					!= __mark_empty_function) {

@@ -360,12 +375,7 @@ static int marker_set_probe_range(const char *name,
 					*iter->cmark->call = probe;
 				}
 				/* Can have many enables for one function */
-				*MARK_ENABLE_OFFSET(iter->enable) = 1;
-#ifdef MARK_POLYMORPHIC
-				flush_icache_range(
-					MARK_ENABLE_OFFSET(iter->enable),
-					sizeof(*(iter->enable)));
-#endif
+				marker_set_enable(iter->enable, 1);
 			}
 			found++;
 		}

@@ -382,12 +392,7 @@ static int marker_remove_probe_range(marker_probe_func *probe,
 
 	for (iter = begin; iter < end; iter++) {
 		if (*iter->cmark->call == probe) {
-			*MARK_ENABLE_OFFSET(iter->enable) = 0;
-#ifdef MARK_POLYMORPHIC
-				flush_icache_range(
-					MARK_ENABLE_OFFSET(iter->enable),
-					sizeof(*(iter->enable)));
-#endif
+			marker_set_enable(iter->enable, 0);
 			*iter->cmark->call = __mark_empty_function;
 			found++;
 		}

@@ -419,9 +424,8 @@ int marker_set_probe(const char *name, const char *format,
 {
 	struct module *mod;
 	int found = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&modlist_lock, flags);
+	mutex_lock(&module_mutex);
 	/* Core kernel markers */
 	found += marker_set_probe_range(name, format, probe,
 			__start___markers, __stop___markers);

@@ -431,7 +435,7 @@ int marker_set_probe(const char *name, const char *format,
 			found += marker_set_probe_range(name, format, probe,
 				mod->markers, mod->markers+mod->num_markers);
 	}
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	mutex_unlock(&module_mutex);
 	return found;
 }
 EXPORT_SYMBOL(marker_set_probe);

@@ -440,9 +444,8 @@ int marker_remove_probe(marker_probe_func *probe)
 {
 	struct module *mod;
 	int found = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&modlist_lock, flags);
+	mutex_lock(&module_mutex);
 	/* Core kernel markers */
 	found += marker_remove_probe_range(probe,
 			__start___markers, __stop___markers);

@@ -452,7 +455,7 @@ int marker_remove_probe(marker_probe_func *probe)
 			found += marker_remove_probe_range(probe,
 				mod->markers, mod->markers+mod->num_markers);
 	}
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	mutex_unlock(&module_mutex);
 	return found;
 }
 EXPORT_SYMBOL(marker_remove_probe);

@@ -461,9 +464,8 @@ int marker_list_probe(marker_probe_func *probe)
 {
 	struct module *mod;
 	int found = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&modlist_lock, flags);
+	mutex_lock(&module_mutex);
 	/* Core kernel markers */
 	printk("Listing kernel markers\n");
 	found += marker_list_probe_range(probe,

@@ -477,7 +479,7 @@ int marker_list_probe(marker_probe_func *probe)
 				mod->markers, mod->markers+mod->num_markers);
 		}
 	}
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	mutex_unlock(&module_mutex);
 	return found;
 }
 EXPORT_SYMBOL(marker_list_probe);

Signature · 7 lines

-- 
OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 
_______________________________________________
Ltt-dev mailing list
[email protected]
http://listserv.shafik.org/mailman/listinfo/ltt-dev

Hi,

I have looked at kprobes code and have some questions for you. I would really like to use it to patch dynamically my marker immediate value by doing code patching. Using an int3 seems like the right way to handle this wrt pIII erratum 49.

Everything is ok, except for a limitation important to the LTTng project :
kprobes cannot probe trap handlers. Looking at the code, I see that the kprobes trap notifier expects interrupts to be disabled when it is run. Looking a little deeper in the code, I notice that you use per-cpu data structures to keep the probe control information that is needed for single stepping, which clearly requires you to disable interrupts so no interrupt handler with a kprobe in it fires on top of the kprobe handler. It also forbids trap handler and NMI handler instrumentation, as traps can be triggered by the kprobes handler and NMIs can come at any point during execution.

From i386 point of view, your understanding is correct.

Would it be possible to put these data structures on the stack or on a separate stack accessible through thread_info instead ?

Quoted 465 lines

Hi Richard,

Quoted 11 lines · * Mathieu Desnoyers ([email protected]) wrote:

Quoted 6 lines

You've got the same optimizations for x86 by modifying an instruction's immediate operand and thus avoiding a d-cache hit. The only real caveat is the need to avoid the unsynchronised cross modification erratum. Which means that all processors will need to issue a serializing operation before executing a Marker whose state is changed. How is that handled?

Good catch. I thought that modifying only 1 byte would spare us from this errata, but looking at it in detail tells me than it's not the case.

I see three different ways to address the problem :

[...]

Quoted 6 lines

3 - First write an int3 instead of the instruction's first byte. The handler
would do the following :
int3_handler :
single-step the original instruction.
iret

Quoted 8 lines

Secondly, we call an IPI that does a smp_processor_id() on each CPU and wait for them to complete. It will make sure we execute a synchronizing instruction on every CPU even if we do not execute the trap handler.

Then, we write the new 2 bytes instruction atomically instead of the int3 and immediate value.

Here is the implementation of my proposal using a slightly enhanced kprobes. I add the ability to single step a different instruction than the original one, and then put the new instruction instead of the original one when removing the kprobe. It is an improvement on the djprobes design : AFAIK, djprobes required the int3 to be executed by _every_ CPU before the instruction could be replaced. It was problematic with rarely used code paths (error handling) and with thread CPU affinity. Comments are welcome.

I noticed that it restrains LTTng by removing the ability to probe do_general_protection, do_nmi, do_trap, do_debug and do_page_fault. hardirq on/off in lockdep.c must also be tweaked to allow local_irq_enable/disable usage within the debug trap handler.

It would be nice to push the study of the kprobes debug trap handler so it can become possible to use it to put breakpoints in trap handlers. For now, kprobes refuses to insert breakpoints in __kprobes marked functions. However, as we instrument specific spots of the functions (not necessarily the function entry), it is sometimes correct to use kprobes on a marker within the function even if it is not correct to use it in the prologue. Insight from the SystemTAP team would be welcome on this kprobe limitation.

Mathieu

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

arch/i386/kernel/kprobes.c +68 -0

@@ -31,6 +31,7 @@
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
 #include <linux/preempt.h>
+#include <linux/kallsyms.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>
 #include <asm/desc.h>

@@ -753,6 +754,73 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 	return 0;
 }
 
+static struct kprobe xmc_kp;
+DEFINE_MUTEX(kprobe_xmc_mutex);
+
+static int xmc_handler_pre(struct kprobe *p, struct pt_regs *regs)
+{
+	return 0;
+}
+
+static void xmc_handler_post(struct kprobe *p, struct pt_regs *regs,
+	unsigned long flags)
+{
+}
+
+static int xmc_handler_fault(struct kprobe *p, struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
+
+static void xmc_synchronize_cpu(void *info)
+{
+	smp_processor_id();
+}
+
+/* Think of it as a memcpy of new into address with safety with regard to
+ * Intel PIII errata 49. Only valid for modifying a single instruction with
+ * an instruction of the same size or in smaller instructions of the total
+ * same size as the original instruction. */
+int arch_xmc(void *address, char *newi, int size)
+{
+	int ret = 0;
+	char *dest = (char*)address;
+	char str[KSYM_NAME_LEN + 1];
+	unsigned long sym_offs, sym_size;
+	char *modname;
+	const char *sym_name;
+
+	mutex_lock(&kprobe_xmc_mutex);
+	xmc_kp.pre_handler = xmc_handler_pre;
+	xmc_kp.post_handler = xmc_handler_post;
+	xmc_kp.fault_handler = xmc_handler_fault;
+	xmc_kp.addr = address;
+	xmc_kp.symbol_name = NULL;
+	xmc_kp.offset = 0;
+
+	ret = register_kprobe_swapi(&xmc_kp, newi, size);
+	if (ret < 0) {
+		sym_name = kallsyms_lookup((unsigned long)address,
+				&sym_size, &sym_offs,
+				&modname, str);
+		printk("register_kprobe failed for %p in %s:%s returned %d\n",
+			address, modname?modname:"kernel", sym_name, ret);
+		goto end;
+	}
+	/* Copy every bytes of the new instruction except the first one */
+	memcpy(dest+1, newi+1, size-1);
+	flush_icache_range(dest, size);
+	/* Execute serializing instruction on each CPU */
+	ret = on_each_cpu(xmc_synchronize_cpu, NULL, 1, 1);
+	BUG_ON(ret != 0);
+
+	unregister_kprobe(&xmc_kp);
+end:
+	mutex_unlock(&kprobe_xmc_mutex);
+
+	return ret;
+}
+
 int __init arch_init_kprobes(void)
 {
 	return 0;

include/asm-generic/marker.h +1 -1

@@ -18,7 +18,7 @@ struct __mark_marker_c {
 
 struct __mark_marker {
 	const struct __mark_marker_c *cmark;
-	volatile char *enable;
+	char *enable;
 } __attribute__((packed));
 
 #ifdef CONFIG_MARKERS

include/asm-i386/kprobes.h +3 -0

@@ -90,4 +90,7 @@ static inline void restore_interrupts(struct pt_regs *regs)
 
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);
+/* pIII code cross modification which follows erratum 49. */
+#define ARCH_HAS_XMC
+int arch_xmc(void *address, char *newi, int size);
 #endif				/* _ASM_KPROBES_H */

include/asm-i386/marker.h +1 -1

@@ -18,7 +18,7 @@ struct __mark_marker_c {
 
 struct __mark_marker {
 	struct __mark_marker_c *cmark;
-	volatile char *enable;
+	char *enable;
 } __attribute__((packed));
 
 #ifdef CONFIG_MARKERS

include/asm-powerpc/marker.h +1 -1

@@ -20,7 +20,7 @@ struct __mark_marker_c {
 
 struct __mark_marker {
 	struct __mark_marker_c *cmark;
-	volatile short *enable;
+	short *enable;
 } __attribute__((packed));
 
 #ifdef CONFIG_MARKERS

include/linux/kprobes.h +4 -0

@@ -189,6 +189,7 @@ static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
 }
 
 int register_kprobe(struct kprobe *p);
+int register_kprobe_swapi(struct kprobe *p, char *newi, int size);
 void unregister_kprobe(struct kprobe *p);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
 int longjmp_break_handler(struct kprobe *, struct pt_regs *);

@@ -203,6 +204,9 @@ struct kretprobe_instance *get_free_rp_inst(struct kretprobe *rp);
 void add_rp_inst(struct kretprobe_instance *ri);
 void kprobe_flush_task(struct task_struct *tk);
 void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
+/* Architecture specific code cross-modification. Valid for overwriting
+ * a single instruction. Safe for Intel PIII erratum 49. */
+int kprobe_xmc(void *address, char *newi, int size);
 #else /* CONFIG_KPROBES */
 
 #define __kprobes	/**/

include/linux/marker.h +1 -1

@@ -40,7 +40,7 @@
 
 typedef void marker_probe_func(const char *fmt, ...);
 
-#ifndef CONFIG_MARKERS_DISABLE_OPTIMIZATION
+#ifdef CONFIG_MARKERS_ENABLE_OPTIMIZATION
 #include <asm/marker.h>
 #else
 #include <asm-generic/marker.h>

kernel/Kconfig.marker +4 -3

@@ -8,9 +8,10 @@ config MARKERS
 	  Place an empty function call at each marker site. Can be
 	  dynamically changed for a probe function.
 
-config MARKERS_DISABLE_OPTIMIZATION
-	bool "Disable architecture specific marker optimization"
-	depends EMBEDDED
+config MARKERS_ENABLE_OPTIMIZATION
+	bool "Enable marker optimization (EXPERIMENTAL)"
+	depends MARKERS
+	select KPROBES
 	default n
 	help
 	  Disable code replacement jump optimisations. Especially useful if your

kernel/kprobes.c +44 -4

@@ -534,7 +534,7 @@ static int __kprobes in_kprobes_functions(unsigned long addr)
 }
 
 static int __kprobes __register_kprobe(struct kprobe *p,
-	unsigned long called_from)
+	unsigned long called_from, char *newi, int size)
 {
 	int ret = 0;
 	struct kprobe *old_p;

@@ -587,6 +587,10 @@ static int __kprobes __register_kprobe(struct kprobe *p,
 
 	if ((ret = arch_prepare_kprobe(p)) != 0)
 		goto out;
+	if (newi) {
+		memcpy(p->ainsn.insn, newi, size);
+		p->opcode = *newi;
+	}
 
 	INIT_HLIST_NODE(&p->hlist);
 	hlist_add_head_rcu(&p->hlist,

@@ -609,7 +613,14 @@ out:
 int __kprobes register_kprobe(struct kprobe *p)
 {
 	return __register_kprobe(p,
-		(unsigned long)__builtin_return_address(0));
+		(unsigned long)__builtin_return_address(0),
+		NULL, 0);
+}
+
+int __kprobes register_kprobe_swapi(struct kprobe *p, char *newi, int size)
+{
+	return __register_kprobe(p,
+		(unsigned long)__builtin_return_address(0), newi, size);
 }
 
 void __kprobes unregister_kprobe(struct kprobe *p)

@@ -699,7 +710,8 @@ int __kprobes register_jprobe(struct jprobe *jp)
 	jp->kp.break_handler = longjmp_break_handler;
 
 	return __register_kprobe(&jp->kp,
-		(unsigned long)__builtin_return_address(0));
+		(unsigned long)__builtin_return_address(0),
+		NULL, 0);
 }
 
 void __kprobes unregister_jprobe(struct jprobe *jp)

@@ -760,7 +772,8 @@ int __kprobes register_kretprobe(struct kretprobe *rp)
 	rp->nmissed = 0;
 	/* Establish function entry probe point */
 	if ((ret = __register_kprobe(&rp->kp,
-		(unsigned long)__builtin_return_address(0))) != 0)
+		(unsigned long)__builtin_return_address(0),
+		NULL, 0)) != 0)
 		free_rp_inst(rp);
 	return ret;
 }

@@ -790,6 +803,33 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
 	free_rp_inst(rp);
 }
 
+#ifdef ARCH_HAS_XMC
+int kprobe_xmc(void *address, char *newi, int size)
+{
+	return arch_xmc(address, newi, size);
+}
+#else
+/* Limited XMC : only overwrite an instruction with an atomic operation
+ * (writing at most an aligned long). */
+int kprobe_xmc(void *address, char *newi, int size)
+{
+	if (size > sizeof(long)) {
+		printk("Limited XMC : cannot overwrite instructions bigger "\
+			"than %d. XMC of size %d fails.\n", sizeof(long),
+			size);
+		return -EPERM;
+	}
+	if (hweight32(size) != 1 || address % size != 0) {
+		printk("Limited XMC : cannot write %d bytes unaligned "\
+			"instruction. XMC fails.\n", size);
+		return -EPERM;
+	}
+	memcpy(address, newi, size);
+	flush_icache_range(address, size);
+}
+#endif /* HAS_ARCH_XMC */
+EXPORT_SYMBOL(kprobe_xmc);
+
 static int __init init_kprobes(void)
 {
 	int i, err = 0;

kernel/module.c +30 -29

@@ -40,6 +40,7 @@
 #include <linux/string.h>
 #include <linux/mutex.h>
 #include <linux/unwind.h>
+#include <linux/kprobes.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 #include <asm/cacheflush.h>

@@ -309,6 +310,26 @@ EXPORT_SYMBOL(__mark_empty_function);
 #define MARK_ENABLE_OFFSET(a) \
 	(typeof(a))((char*)a+MARK_ENABLE_IMMEDIATE_OFFSET)
 
+/* wmb() stay ordered. State will be ok for interrupts/other CPUs. */
+#ifdef MARK_POLYMORPHIC
+static int marker_set_enable(char *address, char enable)
+{
+	char newi[MARK_ENABLE_IMMEDIATE_OFFSET+1];
+
+	memcpy(newi, address, MARK_ENABLE_IMMEDIATE_OFFSET+1);
+	*MARK_ENABLE_OFFSET(&newi[0]) = enable;
+	return kprobe_xmc(address, newi, MARK_ENABLE_IMMEDIATE_OFFSET+1);
+}
+#else
+static int marker_set_enable(char *address, char enable)
+{
+	*MARK_ENABLE_OFFSET(address) = enable;
+	return 0;
+}
+#endif /* MARK_POLYMORPHIC */
+
+/* enable and function address are set out of order, and it's ok :
+ * the state is always coherent. */
 static int marker_set_probe_range(const char *name,
 	const char *format,
 	marker_probe_func *probe,

@@ -336,13 +357,7 @@ static int marker_set_probe_range(const char *name,
 					*iter->cmark->call =
 						__mark_empty_function;
 				}
-				/* Can have many enables for one function */
-				*MARK_ENABLE_OFFSET(iter->enable) = 0;
-#ifdef MARK_POLYMORPHIC
-				flush_icache_range(
-					MARK_ENABLE_OFFSET(iter->enable),
-					sizeof(*(iter->enable)));
-#endif
+				marker_set_enable(iter->enable, 0);
 			} else {
 				if (*iter->cmark->call
 					!= __mark_empty_function) {

@@ -360,12 +375,7 @@ static int marker_set_probe_range(const char *name,
 					*iter->cmark->call = probe;
 				}
 				/* Can have many enables for one function */
-				*MARK_ENABLE_OFFSET(iter->enable) = 1;
-#ifdef MARK_POLYMORPHIC
-				flush_icache_range(
-					MARK_ENABLE_OFFSET(iter->enable),
-					sizeof(*(iter->enable)));
-#endif
+				marker_set_enable(iter->enable, 1);
 			}
 			found++;
 		}

@@ -382,12 +392,7 @@ static int marker_remove_probe_range(marker_probe_func *probe,
 
 	for (iter = begin; iter < end; iter++) {
 		if (*iter->cmark->call == probe) {
-			*MARK_ENABLE_OFFSET(iter->enable) = 0;
-#ifdef MARK_POLYMORPHIC
-				flush_icache_range(
-					MARK_ENABLE_OFFSET(iter->enable),
-					sizeof(*(iter->enable)));
-#endif
+			marker_set_enable(iter->enable, 0);
 			*iter->cmark->call = __mark_empty_function;
 			found++;
 		}

@@ -419,9 +424,8 @@ int marker_set_probe(const char *name, const char *format,
 {
 	struct module *mod;
 	int found = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&modlist_lock, flags);
+	mutex_lock(&module_mutex);
 	/* Core kernel markers */
 	found += marker_set_probe_range(name, format, probe,
 			__start___markers, __stop___markers);

@@ -431,7 +435,7 @@ int marker_set_probe(const char *name, const char *format,
 			found += marker_set_probe_range(name, format, probe,
 				mod->markers, mod->markers+mod->num_markers);
 	}
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	mutex_unlock(&module_mutex);
 	return found;
 }
 EXPORT_SYMBOL(marker_set_probe);

@@ -440,9 +444,8 @@ int marker_remove_probe(marker_probe_func *probe)
 {
 	struct module *mod;
 	int found = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&modlist_lock, flags);
+	mutex_lock(&module_mutex);
 	/* Core kernel markers */
 	found += marker_remove_probe_range(probe,
 			__start___markers, __stop___markers);

@@ -452,7 +455,7 @@ int marker_remove_probe(marker_probe_func *probe)
 			found += marker_remove_probe_range(probe,
 				mod->markers, mod->markers+mod->num_markers);
 	}
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	mutex_unlock(&module_mutex);
 	return found;
 }
 EXPORT_SYMBOL(marker_remove_probe);

@@ -461,9 +464,8 @@ int marker_list_probe(marker_probe_func *probe)
 {
 	struct module *mod;
 	int found = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&modlist_lock, flags);
+	mutex_lock(&module_mutex);
 	/* Core kernel markers */
 	printk("Listing kernel markers\n");
 	found += marker_list_probe_range(probe,

@@ -477,7 +479,7 @@ int marker_list_probe(marker_probe_func *probe)
 				mod->markers, mod->markers+mod->num_markers);
 		}
 	}
-	spin_unlock_irqrestore(&modlist_lock, flags);
+	mutex_unlock(&module_mutex);
 	return found;
 }
 EXPORT_SYMBOL(marker_list_probe);
 
--

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Quoted 5 lines

_______________________________________________
Ltt-dev mailing list
[email protected]
http://listserv.shafik.org/mailman/listinfo/ltt-dev

--

OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

This patch adds a barrier() to lockdep.c lockdep_recursion updates. This variable behaves like the preemption count and should therefore use similar memory barriers.

This patch applies on 2.6.20-rc4-git3.

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

kernel/lockdep.c +0 -0

@@ -166,12 +166,14 @@ static struct list_head chainhash_table[CHAINHASH_SIZE];
 void lockdep_off(void)
 {
 	current->lockdep_recursion++;

Quoted 14 lines · On Tue, 16 Jan 2007 12:56:24 -0500 Mathieu Desnoyers…

This patch adds a barrier() to lockdep.c lockdep_recursion updates. This variable behaves like the preemption count and should therefore use similar memory barriers.

This patch applies on 2.6.20-rc4-git3.

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

kernel/lockdep.c +0 -0

@@ -166,12 +166,14 @@ static struct list_head chainhash_table[CHAINHASH_SIZE];
 void lockdep_off(void)
 {
 	current->lockdep_recursion++;

kernel/lockdep.c static struct list_head chainhash_table[CHAINHASH_SIZE]; · line 178

Quoted 10 lines

kernel/lockdep.c +2 -0

+	barrier();
 }
 
 EXPORT_SYMBOL(lockdep_off);
 
 void lockdep_on(void)
 {
+	barrier();
 	current->lockdep_recursion--;
 }

I am allergic to undocumented barriers. It is often unobvious what the barrier is supposed to protect against, yielding mystifying code. This is one such case.

Please add code comments.

(quoted diff) +5 -5

 	if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
-		return;
+		goto end;
 
 	if (unlikely(curr->hardirqs_enabled)) {
 		debug_atomic_inc(&redundant_hardirqs_on);
-		return;
+		goto end;
 	}
 	/* we'll do an OFF -> ON transition: */
 	curr->hardirqs_enabled = 1;
 	ip = (unsigned long) __builtin_return_address(0);
 
 	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-		return;
+		goto end;
 	if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
-		return;
+		goto end;
 	/*
 	 * We are going to turn hardirqs on, so set the
 	 * usage bit for all held locks:
 	 */
 	if (!mark_held_locks(curr, 1, ip))
-		return;
+		goto end;
 	/*
 	 * If we have softirqs enabled, then set the usage
 	 * bit for all held locks. (disabled hardirqs prevented

@@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
 	 */
 	if (curr->softirqs_enabled)

Review void trace_hardirqs_on(void)

Quoted 11 lines · On Tue, 16 Jan 2007 12:56:31 -0500 Mathieu Desnoyers…

Here is a patch to lockdep.c so it behaves correctly when a kprobe breakpoint is put on a marker within hardirq tracing functions as long as the marker is within the lockdep_recursion incremented boundaries. It should apply on 2.6.20-rc4-git3.

Mathieu

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

(quoted diff) +0 -0

@@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)

You lost the patch headers.

Quoted 3 lines

(quoted diff) +0 -0

 	struct task_struct *curr = current;
 	unsigned long ip;
 

Review void trace_hardirqs_on(void) · line 1850

Quoted 5 lines

(quoted diff) +2 -0

 	if (unlikely(!debug_locks || current->lockdep_recursion))
 		return;
 
+	current->lockdep_recursion++;
+	barrier();

Why can't we use lockdep_off() here?

Quoted 32 lines

(quoted diff) +5 -5

 	if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
-		return;
+		goto end;
 
 	if (unlikely(curr->hardirqs_enabled)) {
 		debug_atomic_inc(&redundant_hardirqs_on);
-		return;
+		goto end;
 	}
 	/* we'll do an OFF -> ON transition: */
 	curr->hardirqs_enabled = 1;
 	ip = (unsigned long) __builtin_return_address(0);
 
 	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-		return;
+		goto end;
 	if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
-		return;
+		goto end;
 	/*
 	 * We are going to turn hardirqs on, so set the
 	 * usage bit for all held locks:
 	 */
 	if (!mark_held_locks(curr, 1, ip))
-		return;
+		goto end;
 	/*
 	 * If we have softirqs enabled, then set the usage
 	 * bit for all held locks. (disabled hardirqs prevented

@@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
 	 */
 	if (curr->softirqs_enabled)

Review void trace_hardirqs_on(void) · line 1890

Quoted 10 lines

(quoted diff) +4 -1

 		if (!mark_held_locks(curr, 0, ip))
-			return;
+			goto end;
 
 	curr->hardirq_enable_ip = ip;
 	curr->hardirq_enable_event = ++curr->irq_events;
 	debug_atomic_inc(&hardirqs_on_events);
+end:
+	barrier();
+	current->lockdep_recursion--;

lockdep_on()?

Review void trace_hardirqs_off(void) · line 1903

Quoted 12 lines

(quoted diff) +2 -0

 }
 
 EXPORT_SYMBOL(trace_hardirqs_on);

@@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
 {
 	struct task_struct *curr = current;
 
 	if (unlikely(!debug_locks || current->lockdep_recursion))
 		return;
 
+	current->lockdep_recursion++;
+	barrier();

lockdep_off()?

Review void trace_hardirqs_off(void) · line 1926

Quoted 13 lines

(quoted diff) +4 -1

 	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-		return;
+		goto end;
 
 	if (curr->hardirqs_enabled) {
 		/*

@@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
 		debug_atomic_inc(&hardirqs_off_events);
 	} else
 		debug_atomic_inc(&redundant_hardirqs_off);
+end:
+	barrier();
+	current->lockdep_recursion--;

lockdep_on()?