Kprobes: scalability enhancements

PATCH In review Ananth N Mavinakayanahalli 2 participants 17 messages Oct 10, 2005 – Oct 18, 2005 kprobes

lore

Share & tools

lore

Ananth N Mavinakayanahalli

author

2005-10-10 14:38 UTC #0 lore raw

cover Kprobes: scalability enhancements

Hi,

The following set of patches are aimed at improving kprobes scalability. We currently serialize kprobe registration, unregistration and handler execution using a single spinlock - kprobe_lock.

With these changes, kprobe handlers can run without any locks held. It also allows for simultaneous kprobe handler executions on different processors as we now track kprobe execution on a per processor basis. It is now necessary that the handlers be re-entrant since handlers can run concurrently on multiple processors.

All changes have been tested on i386, ia64, ppc64 and x86_64, while sparc64 has been compile tested only.

The patches can be viewed as 3 logical chunks:

patch 1: Reorder preempt_(dis/en)able calls
patches 2-7: Introduce per_cpu data areas to track kprobe execution
patches 8-9: Use RCU to synchronize kprobe (un)registration and handler
execution.

Thanks to Maneesh Soni, James Keniston and Anil Keshavamurthy for their review and suggestions. Thanks again to Anil, Hien Nguyen and Kevin Stafford for testing the patches.

Ananth

Ananth N Mavinakayanahalli

author

2005-10-10 14:39 UTC #1 lore raw

PATCH 1/9 Kprobes: rearrange preempt_disable/enable() calls

From: Ananth N Mavinakayanahalli <[email protected]>

Reorder preempt_disable/enable() calls in arch kprobes files in preparation to introduce locking changes. No functional changes introduced by this patch.

Signed-off-by Ananth N Mavinakayahanalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/i386/kernel/kprobes.c    |   27 ++++++++++++++-------------
 arch/ia64/kernel/kprobes.c    |   22 ++++++++++++++--------
 arch/ppc64/kernel/kprobes.c   |   11 ++++++-----
 arch/sparc64/kernel/kprobes.c |   25 +++++++++++++------------
 arch/x86_64/kernel/kprobes.c  |   28 ++++++++++++++--------------
 5 files changed, 61 insertions(+), 52 deletions(-)

arch/i386/kernel/kprobes.c +14 ~~-13~~

Index: linux-2.6.14-rc3/arch/i386/kernel/kprobes.c
===================================================================
@@ -158,8 +158,6 @@ static int __kprobes kprobe_handler(stru
 	kprobe_opcode_t *addr = NULL;
 	unsigned long *lp;
 
-	/* We're in an interrupt, but this is clear and BUG()-safe. */
-	preempt_disable();
 	/* Check if the application is using LDT entry for its code segment and
 	 * calculate the address by reading the base address from the LDT entry.
 	 */

@@ -232,6 +230,11 @@ static int __kprobes kprobe_handler(stru
 		goto no_kprobe;
 	}
 
+	/*
+	 * This preempt_disable() matches the preempt_enable_no_resched()
+	 * in post_kprobe_handler()
+	 */
+	preempt_disable();
 	kprobe_status = KPROBE_HIT_ACTIVE;
 	set_current_kprobe(p, regs);

@@ -245,7 +248,6 @@ ss_probe:
 	return 1;
 
 no_kprobe:
-	preempt_enable_no_resched();
 	return ret;
 }

@@ -316,7 +318,7 @@ int __kprobes trampoline_probe_handler(s
         /*
          * By returning a non-zero value, we are telling
          * kprobe_handler() that we have handled unlocking
-         * and re-enabling preemption.
+	 * and re-enabling preemption
          */
         return 1;
 }

@@ -453,29 +455,29 @@ int __kprobes kprobe_exceptions_notify(s
 				       unsigned long val, void *data)
 {
 	struct die_args *args = (struct die_args *)data;
+	int ret = NOTIFY_DONE;
+
+	preempt_disable();
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_DEBUG:
 		if (post_kprobe_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_GPF:
-		if (kprobe_running() &&
-		    kprobe_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
-		break;
 	case DIE_PAGE_FAULT:
 		if (kprobe_running() &&
 		    kprobe_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	default:
 		break;
 	}
-	return NOTIFY_DONE;
+	preempt_enable();
+	return ret;
 }
 
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)

@@ -502,7 +504,6 @@ int __kprobes setjmp_pre_handler(struct 
 
 void __kprobes jprobe_return(void)
 {
-	preempt_enable_no_resched();
 	asm volatile ("       xchgl   %%ebx,%%esp     \n"
 		      "       int3			\n"
 		      "       .globl jprobe_return_end	\n"

arch/ia64/kernel/kprobes.c +14 -8

Index: linux-2.6.14-rc3/arch/ia64/kernel/kprobes.c
===================================================================
@@ -395,7 +395,7 @@ int __kprobes trampoline_probe_handler(s
         /*
          * By returning a non-zero value, we are telling
          * kprobe_handler() that we have handled unlocking
-         * and re-enabling preemption.
+	 * and re-enabling preemption
          */
         return 1;
 }

@@ -607,8 +607,6 @@ static int __kprobes pre_kprobes_handler
 	struct pt_regs *regs = args->regs;
 	kprobe_opcode_t *addr = (kprobe_opcode_t *)instruction_pointer(regs);
 
-	preempt_disable();
-
 	/* Handle recursion cases */
 	if (kprobe_running()) {
 		p = get_kprobe(addr);

@@ -665,6 +663,11 @@ static int __kprobes pre_kprobes_handler
 		goto no_kprobe;
 	}
 
+	/*
+	 * This preempt_disable() matches the preempt_enable_no_resched()
+	 * in post_kprobes_handler()
+	 */
+	preempt_disable();
 	kprobe_status = KPROBE_HIT_ACTIVE;
 	set_current_kprobe(p);

@@ -682,7 +685,6 @@ ss_probe:
 	return 1;
 
 no_kprobe:
-	preempt_enable_no_resched();
 	return ret;
 }

@@ -733,22 +735,26 @@ int __kprobes kprobe_exceptions_notify(s
 				       unsigned long val, void *data)
 {
 	struct die_args *args = (struct die_args *)data;
+	int ret = NOTIFY_DONE;
+
+	preempt_disable();
 	switch(val) {
 	case DIE_BREAK:
 		if (pre_kprobes_handler(args))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_SS:
 		if (post_kprobes_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_PAGE_FAULT:
 		if (kprobes_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 	default:
 		break;
 	}
-	return NOTIFY_DONE;
+	preempt_enable();
+	return ret;
 }
 
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)

arch/ppc64/kernel/kprobes.c +6 -5

Index: linux-2.6.14-rc3/arch/ppc64/kernel/kprobes.c
===================================================================
@@ -209,6 +209,11 @@ static inline int kprobe_handler(struct 
 		goto no_kprobe;
 	}
 
+	/*
+	 * This preempt_disable() matches the preempt_enable_no_resched()
+	 * in post_kprobe_handler().
+	 */
+	preempt_disable();
 	kprobe_status = KPROBE_HIT_ACTIVE;
 	current_kprobe = p;
 	kprobe_saved_msr = regs->msr;

@@ -219,11 +224,6 @@ static inline int kprobe_handler(struct 
 ss_probe:
 	prepare_singlestep(p, regs);
 	kprobe_status = KPROBE_HIT_SS;
-	/*
-	 * This preempt_disable() matches the preempt_enable_no_resched()
-	 * in post_kprobe_handler().
-	 */
-	preempt_disable();
 	return 1;
 
 no_kprobe:

@@ -293,6 +293,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->nip = orig_ret_address;
 
 	unlock_kprobes();
+	preempt_enable_no_resched();
 
         /*
          * By returning a non-zero value, we are telling

arch/sparc64/kernel/kprobes.c +13 ~~-12~~

Index: linux-2.6.14-rc3/arch/sparc64/kernel/kprobes.c
===================================================================
@@ -118,8 +118,6 @@ static int __kprobes kprobe_handler(stru
 	void *addr = (void *) regs->tpc;
 	int ret = 0;
 
-	preempt_disable();
-
 	if (kprobe_running()) {
 		/* We *are* holding lock here, so this is safe.
 		 * Disarm the probe we just hit, and ignore it.

@@ -171,6 +169,11 @@ static int __kprobes kprobe_handler(stru
 		goto no_kprobe;
 	}
 
+	/*
+	 * This preempt_disable() matches the preempt_enable_no_resched()
+	 * in post_kprobes_handler()
+	 */
+	preempt_disable();
 	set_current_kprobe(p, regs);
 	kprobe_status = KPROBE_HIT_ACTIVE;
 	if (p->pre_handler && p->pre_handler(p, regs))

@@ -182,7 +185,6 @@ ss_probe:
 	return 1;
 
 no_kprobe:
-	preempt_enable_no_resched();
 	return ret;
 }

@@ -322,29 +324,29 @@ int __kprobes kprobe_exceptions_notify(s
 				       unsigned long val, void *data)
 {
 	struct die_args *args = (struct die_args *)data;
+	int ret = NOTIFY_DONE;
+
+	preempt_disable();
 	switch (val) {
 	case DIE_DEBUG:
 		if (kprobe_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_DEBUG_2:
 		if (post_kprobe_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_GPF:
-		if (kprobe_running() &&
-		    kprobe_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
-		break;
 	case DIE_PAGE_FAULT:
 		if (kprobe_running() &&
 		    kprobe_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	default:
 		break;
 	}
-	return NOTIFY_DONE;
+	preempt_enable();
+	return ret;
 }
 
 asmlinkage void __kprobes kprobe_trap(unsigned long trap_level,

@@ -396,7 +398,6 @@ int __kprobes setjmp_pre_handler(struct 
 
 void __kprobes jprobe_return(void)
 {
-	preempt_enable_no_resched();
 	__asm__ __volatile__(
 		".globl	jprobe_return_trap_instruction\n"
 "jprobe_return_trap_instruction:\n\t"

arch/x86_64/kernel/kprobes.c +14 ~~-14~~

Index: linux-2.6.14-rc3/arch/x86_64/kernel/kprobes.c
===================================================================
@@ -302,9 +302,6 @@ int __kprobes kprobe_handler(struct pt_r
 	int ret = 0;
 	kprobe_opcode_t *addr = (kprobe_opcode_t *)(regs->rip - sizeof(kprobe_opcode_t));
 
-	/* We're in an interrupt, but this is clear and BUG()-safe. */
-	preempt_disable();
-
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
 		/* We *are* holding lock here, so this is safe.

@@ -372,6 +369,11 @@ int __kprobes kprobe_handler(struct pt_r
 		goto no_kprobe;
 	}
 
+	/*
+	 * This preempt_disable() matches the preempt_enable_no_resched()
+	 * in post_kprobe_handler()
+	 */
+	preempt_disable();
 	kprobe_status = KPROBE_HIT_ACTIVE;
 	set_current_kprobe(p, regs);

@@ -385,7 +387,6 @@ ss_probe:
 	return 1;
 
 no_kprobe:
-	preempt_enable_no_resched();
 	return ret;
 }

@@ -456,7 +457,7 @@ int __kprobes trampoline_probe_handler(s
         /*
          * By returning a non-zero value, we are telling
          * kprobe_handler() that we have handled unlocking
-         * and re-enabling preemption.
+	 * and re-enabling preemption
          */
         return 1;
 }

@@ -599,29 +600,29 @@ int __kprobes kprobe_exceptions_notify(s
 				       unsigned long val, void *data)
 {
 	struct die_args *args = (struct die_args *)data;
+	int ret = NOTIFY_DONE;
+
+	preempt_disable();
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_DEBUG:
 		if (post_kprobe_handler(args->regs))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	case DIE_GPF:
-		if (kprobe_running() &&
-		    kprobe_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
-		break;
 	case DIE_PAGE_FAULT:
 		if (kprobe_running() &&
 		    kprobe_fault_handler(args->regs, args->trapnr))
-			return NOTIFY_STOP;
+			ret = NOTIFY_STOP;
 		break;
 	default:
 		break;
 	}
-	return NOTIFY_DONE;
+	preempt_enable();
+	return ret;
 }
 
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)

@@ -647,7 +648,6 @@ int __kprobes setjmp_pre_handler(struct 
 
 void __kprobes jprobe_return(void)
 {
-	preempt_enable_no_resched();
 	asm volatile ("       xchg   %%rbx,%%rsp     \n"
 		      "       int3			\n"
 		      "       .globl jprobe_return_end	\n"

Ananth N Mavinakayanahalli

author

2005-10-10 14:41 UTC #2 lore raw

PATCH 2/9 Kprobes: Track kprobe on a per_cpu basis - base changes

From: Ananth N Mavinakayanahalli <[email protected]>

Changes to the base kprobe infrastructure to track kprobe execution on a per-cpu basis.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 include/linux/kprobes.h |   31 ++++++++++++++++++++++---------
 kernel/kprobes.c        |   43 ++++++++++++++++++++++++++++---------------
 2 files changed, 50 insertions(+), 24 deletions(-)

include/linux/kprobes.h +22 -9

Index: linux-2.6.14-rc3/include/linux/kprobes.h
===================================================================
@@ -33,6 +33,7 @@
 #include <linux/list.h>
 #include <linux/notifier.h>
 #include <linux/smp.h>
+#include <linux/percpu.h>
 
 #include <asm/kprobes.h>

@@ -106,6 +107,9 @@ struct jprobe {
 	kprobe_opcode_t *entry;	/* probe handling code to jump to */
 };
 
+DECLARE_PER_CPU(struct kprobe *, current_kprobe);
+DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+
 #ifdef ARCH_SUPPORTS_KRETPROBES
 extern void arch_prepare_kretprobe(struct kretprobe *rp, struct pt_regs *regs);
 #else /* ARCH_SUPPORTS_KRETPROBES */

@@ -146,13 +150,6 @@ struct kretprobe_instance {
 void lock_kprobes(void);
 void unlock_kprobes(void);
 
-/* kprobe running now on this CPU? */
-static inline int kprobe_running(void)
-{
-	extern unsigned int kprobe_cpu;
-	return kprobe_cpu == smp_processor_id();
-}
-
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_copy_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);

@@ -167,6 +164,22 @@ extern void free_insn_slot(kprobe_opcode
 struct kprobe *get_kprobe(void *addr);
 struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
 
+/* kprobe_running() will just return the current_kprobe on this CPU */
+static inline struct kprobe *kprobe_running(void)
+{
+	return (__get_cpu_var(current_kprobe));
+}
+
+static inline void reset_current_kprobe(void)
+{
+	__get_cpu_var(current_kprobe) = NULL;
+}
+
+static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
+{
+	return (&__get_cpu_var(kprobe_ctlblk));
+}
+
 int register_kprobe(struct kprobe *p);
 void unregister_kprobe(struct kprobe *p);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);

@@ -183,9 +196,9 @@ void add_rp_inst(struct kretprobe_instan
 void kprobe_flush_task(struct task_struct *tk);
 void recycle_rp_inst(struct kretprobe_instance *ri);
 #else /* CONFIG_KPROBES */
-static inline int kprobe_running(void)
+static inline struct kprobe *kprobe_running(void)
 {
-	return 0;
+	return NULL;
 }
 static inline int register_kprobe(struct kprobe *p)
 {

kernel/kprobes.c +28 ~~-15~~

88 lines not on this page · raw message

Ananth N Mavinakayanahalli

author

2005-10-10 14:42 UTC #3 lore raw

PATCH 3/9 Kprobes: Track kprobe on a per_cpu basis - i386 changes

From: Ananth N Mavinakayanahalli <[email protected]>

I386 changes to track kprobe execution on a per-cpu basis. We now track the kprobe state machine independently on each cpu, using an arch specific kprobe control block.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/i386/kernel/kprobes.c |  126 ++++++++++++++++++++++++---------------------
 include/asm-i386/kprobes.h |   17 ++++++
 2 files changed, 86 insertions(+), 57 deletions(-)

arch/i386/kernel/kprobes.c +69 ~~-57~~

Index: linux-2.6.14-rc3/arch/i386/kernel/kprobes.c
===================================================================
@@ -37,16 +37,11 @@
 #include <asm/kdebug.h>
 #include <asm/desc.h>
 
-static struct kprobe *current_kprobe;
-static unsigned long kprobe_status, kprobe_old_eflags, kprobe_saved_eflags;
-static struct kprobe *kprobe_prev;
-static unsigned long kprobe_status_prev, kprobe_old_eflags_prev, kprobe_saved_eflags_prev;
-static struct pt_regs jprobe_saved_regs;
-static long *jprobe_saved_esp;
-/* copy of the kernel stack at the probe fire time */
-static kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE];
 void jprobe_return_end(void);
 
+DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+
 /*
  * returns non-zero if opcode modifies the interrupt flag.
  */

@@ -91,29 +86,30 @@ void __kprobes arch_remove_kprobe(struct
 {
 }
 
-static inline void save_previous_kprobe(void)
+static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	kprobe_prev = current_kprobe;
-	kprobe_status_prev = kprobe_status;
-	kprobe_old_eflags_prev = kprobe_old_eflags;
-	kprobe_saved_eflags_prev = kprobe_saved_eflags;
+	kcb->prev_kprobe.kp = kprobe_running();
+	kcb->prev_kprobe.status = kcb->kprobe_status;
+	kcb->prev_kprobe.old_eflags = kcb->kprobe_old_eflags;
+	kcb->prev_kprobe.saved_eflags = kcb->kprobe_saved_eflags;
 }
 
-static inline void restore_previous_kprobe(void)
+static inline void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = kprobe_prev;
-	kprobe_status = kprobe_status_prev;
-	kprobe_old_eflags = kprobe_old_eflags_prev;
-	kprobe_saved_eflags = kprobe_saved_eflags_prev;
+	__get_cpu_var(current_kprobe) = kcb->prev_kprobe.kp;
+	kcb->kprobe_status = kcb->prev_kprobe.status;
+	kcb->kprobe_old_eflags = kcb->prev_kprobe.old_eflags;
+	kcb->kprobe_saved_eflags = kcb->prev_kprobe.saved_eflags;
 }
 
-static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs)
+static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
+				struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = p;
-	kprobe_saved_eflags = kprobe_old_eflags
+	__get_cpu_var(current_kprobe) = p;
+	kcb->kprobe_saved_eflags = kcb->kprobe_old_eflags
 		= (regs->eflags & (TF_MASK | IF_MASK));
 	if (is_IF_modifier(p->opcode))
-		kprobe_saved_eflags &= ~IF_MASK;
+		kcb->kprobe_saved_eflags &= ~IF_MASK;
 }
 
 static inline void prepare_singlestep(struct kprobe *p, struct pt_regs *regs)

@@ -157,6 +153,7 @@ static int __kprobes kprobe_handler(stru
 	int ret = 0;
 	kprobe_opcode_t *addr = NULL;
 	unsigned long *lp;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	/* Check if the application is using LDT entry for its code segment and
 	 * calculate the address by reading the base address from the LDT entry.

@@ -175,10 +172,10 @@ static int __kprobes kprobe_handler(stru
 		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
-			if (kprobe_status == KPROBE_HIT_SS &&
+			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
-				regs->eflags |= kprobe_saved_eflags;
+				regs->eflags |= kcb->kprobe_saved_eflags;
 				unlock_kprobes();
 				goto no_kprobe;
 			}

@@ -188,14 +185,14 @@ static int __kprobes kprobe_handler(stru
 			 * just single step on the instruction of the new probe
 			 * without calling any user handlers.
 			 */
-			save_previous_kprobe();
-			set_current_kprobe(p, regs);
+			save_previous_kprobe(kcb);
+			set_current_kprobe(p, regs, kcb);
 			p->nmissed++;
 			prepare_singlestep(p, regs);
-			kprobe_status = KPROBE_REENTER;
+			kcb->kprobe_status = KPROBE_REENTER;
 			return 1;
 		} else {
-			p = current_kprobe;
+			p = __get_cpu_var(current_kprobe);
 			if (p->break_handler && p->break_handler(p, regs)) {
 				goto ss_probe;
 			}

@@ -235,8 +232,8 @@ static int __kprobes kprobe_handler(stru
 	 * in post_kprobe_handler()
 	 */
 	preempt_disable();
-	kprobe_status = KPROBE_HIT_ACTIVE;
-	set_current_kprobe(p, regs);
+	set_current_kprobe(p, regs, kcb);
+	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 
 	if (p->pre_handler && p->pre_handler(p, regs))
 		/* handler has already set things up, so skip ss setup */

@@ -244,7 +241,7 @@ static int __kprobes kprobe_handler(stru
 
 ss_probe:
 	prepare_singlestep(p, regs);
-	kprobe_status = KPROBE_HIT_SS;
+	kcb->kprobe_status = KPROBE_HIT_SS;
 	return 1;
 
 no_kprobe:

@@ -312,6 +309,7 @@ int __kprobes trampoline_probe_handler(s
 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
 	regs->eip = orig_ret_address;
 
+	reset_current_kprobe();
 	unlock_kprobes();
 	preempt_enable_no_resched();

@@ -345,7 +343,8 @@ int __kprobes trampoline_probe_handler(s
  * that is atop the stack is the address following the copied instruction.
  * We need to make it the address following the original instruction.
  */
-static void __kprobes resume_execution(struct kprobe *p, struct pt_regs *regs)
+static void __kprobes resume_execution(struct kprobe *p,
+		struct pt_regs *regs, struct kprobe_ctlblk *kcb)
 {
 	unsigned long *tos = (unsigned long *)&regs->esp;
 	unsigned long next_eip = 0;

@@ -355,7 +354,7 @@ static void __kprobes resume_execution(s
 	switch (p->ainsn.insn[0]) {
 	case 0x9c:		/* pushfl */
 		*tos &= ~(TF_MASK | IF_MASK);
-		*tos |= kprobe_old_eflags;
+		*tos |= kcb->kprobe_old_eflags;
 		break;
 	case 0xc3:		/* ret/lret */
 	case 0xcb:

@@ -400,22 +399,26 @@ static void __kprobes resume_execution(s
  */
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {
-	if (!kprobe_running())
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (!cur)
 		return 0;
 
-	if ((kprobe_status != KPROBE_REENTER) && current_kprobe->post_handler) {
-		kprobe_status = KPROBE_HIT_SSDONE;
-		current_kprobe->post_handler(current_kprobe, regs, 0);
+	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
+		kcb->kprobe_status = KPROBE_HIT_SSDONE;
+		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(current_kprobe, regs);
-	regs->eflags |= kprobe_saved_eflags;
+	resume_execution(cur, regs, kcb);
+	regs->eflags |= kcb->kprobe_saved_eflags;
 
 	/*Restore back the original saved kprobes variables and continue. */
-	if (kprobe_status == KPROBE_REENTER) {
-		restore_previous_kprobe();
+	if (kcb->kprobe_status == KPROBE_REENTER) {
+		restore_previous_kprobe(kcb);
 		goto out;
 	}
+	reset_current_kprobe();
 	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -434,14 +437,17 @@ out:
 /* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
-	if (current_kprobe->fault_handler
-	    && current_kprobe->fault_handler(current_kprobe, regs, trapnr))
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
 		return 1;
 
-	if (kprobe_status & KPROBE_HIT_SS) {
-		resume_execution(current_kprobe, regs);
-		regs->eflags |= kprobe_old_eflags;
+	if (kcb->kprobe_status & KPROBE_HIT_SS) {
+		resume_execution(cur, regs, kcb);
+		regs->eflags |= kcb->kprobe_old_eflags;
 
+		reset_current_kprobe();
 		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

@@ -484,10 +490,11 @@ int __kprobes setjmp_pre_handler(struct 
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 	unsigned long addr;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
-	jprobe_saved_regs = *regs;
-	jprobe_saved_esp = &regs->esp;
-	addr = (unsigned long)jprobe_saved_esp;
+	kcb->jprobe_saved_regs = *regs;
+	kcb->jprobe_saved_esp = &regs->esp;
+	addr = (unsigned long)(kcb->jprobe_saved_esp);
 
 	/*
 	 * TBD: As Linus pointed out, gcc assumes that the callee

@@ -496,7 +503,8 @@ int __kprobes setjmp_pre_handler(struct 
 	 * we also save and restore enough stack bytes to cover
 	 * the argument area.
 	 */
-	memcpy(jprobes_stack, (kprobe_opcode_t *) addr, MIN_STACK_SIZE(addr));
+	memcpy(kcb->jprobes_stack, (kprobe_opcode_t *)addr,
+			MIN_STACK_SIZE(addr));
 	regs->eflags &= ~IF_MASK;
 	regs->eip = (unsigned long)(jp->entry);
 	return 1;

@@ -504,34 +512,38 @@ int __kprobes setjmp_pre_handler(struct 
 
 void __kprobes jprobe_return(void)
 {
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
 	asm volatile ("       xchgl   %%ebx,%%esp     \n"
 		      "       int3			\n"
 		      "       .globl jprobe_return_end	\n"
 		      "       jprobe_return_end:	\n"
 		      "       nop			\n"::"b"
-		      (jprobe_saved_esp):"memory");
+		      (kcb->jprobe_saved_esp):"memory");
 }
 
 int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 	u8 *addr = (u8 *) (regs->eip - 1);
-	unsigned long stack_addr = (unsigned long)jprobe_saved_esp;
+	unsigned long stack_addr = (unsigned long)(kcb->jprobe_saved_esp);
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 
 	if ((addr > (u8 *) jprobe_return) && (addr < (u8 *) jprobe_return_end)) {
-		if (&regs->esp != jprobe_saved_esp) {
+		if (&regs->esp != kcb->jprobe_saved_esp) {
 			struct pt_regs *saved_regs =
-			    container_of(jprobe_saved_esp, struct pt_regs, esp);
+			    container_of(kcb->jprobe_saved_esp,
+					    struct pt_regs, esp);
 			printk("current esp %p does not match saved esp %p\n",
-			       &regs->esp, jprobe_saved_esp);
+			       &regs->esp, kcb->jprobe_saved_esp);
 			printk("Saved registers for jprobe %p\n", jp);
 			show_registers(saved_regs);
 			printk("Current registers\n");
 			show_registers(regs);
 			BUG();
 		}
-		*regs = jprobe_saved_regs;
-		memcpy((kprobe_opcode_t *) stack_addr, jprobes_stack,
+		*regs = kcb->jprobe_saved_regs;
+		memcpy((kprobe_opcode_t *) stack_addr, kcb->jprobes_stack,
 		       MIN_STACK_SIZE(stack_addr));
 		return 1;
 	}

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

Index: linux-2.6.14-rc3/include/asm-i386/kprobes.h
===================================================================
@@ -49,6 +49,23 @@ struct arch_specific_insn {
 	kprobe_opcode_t insn[MAX_INSN_SIZE];
 };
 
+struct prev_kprobe {
+	struct kprobe *kp;
+	unsigned long status;
+	unsigned long old_eflags;
+	unsigned long saved_eflags;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+	unsigned long kprobe_status;
+	unsigned long kprobe_old_eflags;
+	unsigned long kprobe_saved_eflags;
+	long *jprobe_saved_esp;
+	struct pt_regs jprobe_saved_regs;
+	kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE];
+	struct prev_kprobe prev_kprobe;
+};
 
 /* trap3/1 are intr gates for kprobes.  So, restore the status of IF,
  * if necessary, before executing the original int3/1 (trap) handler.

Ananth N Mavinakayanahalli

author

2005-10-10 14:43 UTC #4 lore raw

PATCH 4/9 Kprobes: Track kprobe on a per_cpu basis - ia64 changes

From: Ananth N Mavinakayanahalli <[email protected]>

IA64 changes to track kprobe execution on a per-cpu basis. We now track the kprobe state machine independently on each cpu using an arch specific kprobe control block.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/ia64/kernel/kprobes.c |   83 ++++++++++++++++++++++++---------------------
 include/asm-ia64/kprobes.h |   13 +++++++
 2 files changed, 58 insertions(+), 38 deletions(-)

arch/ia64/kernel/kprobes.c +45 ~~-38~~

Index: linux-2.6.14-rc3/arch/ia64/kernel/kprobes.c
===================================================================
@@ -38,13 +38,8 @@
 
 extern void jprobe_inst_return(void);
 
-/* kprobe_status settings */
-#define KPROBE_HIT_ACTIVE	0x00000001
-#define KPROBE_HIT_SS		0x00000002
-
-static struct kprobe *current_kprobe, *kprobe_prev;
-static unsigned long kprobe_status, kprobe_status_prev;
-static struct pt_regs jprobe_saved_regs;
+DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 enum instruction_type {A, I, M, F, B, L, X, u};
 static enum instruction_type bundle_encoding[32][3] = {

@@ -313,21 +308,22 @@ static int __kprobes valid_kprobe_addr(i
 	return 0;
 }
 
-static inline void save_previous_kprobe(void)
+static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	kprobe_prev = current_kprobe;
-	kprobe_status_prev = kprobe_status;
+	kcb->prev_kprobe.kp = kprobe_running();
+	kcb->prev_kprobe.status = kcb->kprobe_status;
 }
 
-static inline void restore_previous_kprobe(void)
+static inline void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = kprobe_prev;
-	kprobe_status = kprobe_status_prev;
+	__get_cpu_var(current_kprobe) = kcb->prev_kprobe.kp;
+	kcb->kprobe_status = kcb->prev_kprobe.status;
 }
 
-static inline void set_current_kprobe(struct kprobe *p)
+static inline void set_current_kprobe(struct kprobe *p,
+			struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = p;
+	__get_cpu_var(current_kprobe) = p;
 }
 
 static void kretprobe_trampoline(void)

@@ -389,6 +385,7 @@ int __kprobes trampoline_probe_handler(s
 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
 	regs->cr_iip = orig_ret_address;
 
+	reset_current_kprobe();
 	unlock_kprobes();
 	preempt_enable_no_resched();

@@ -606,12 +603,13 @@ static int __kprobes pre_kprobes_handler
 	int ret = 0;
 	struct pt_regs *regs = args->regs;
 	kprobe_opcode_t *addr = (kprobe_opcode_t *)instruction_pointer(regs);
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	/* Handle recursion cases */
 	if (kprobe_running()) {
 		p = get_kprobe(addr);
 		if (p) {
-			if ( (kprobe_status == KPROBE_HIT_SS) &&
+			if ((kcb->kprobe_status == KPROBE_HIT_SS) &&
 	 		     (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)) {
   				ia64_psr(regs)->ss = 0;
 				unlock_kprobes();

@@ -623,17 +621,17 @@ static int __kprobes pre_kprobes_handler
 			 * just single step on the instruction of the new probe
 			 * without calling any user handlers.
 			 */
-			save_previous_kprobe();
-			set_current_kprobe(p);
+			save_previous_kprobe(kcb);
+			set_current_kprobe(p, kcb);
 			p->nmissed++;
 			prepare_ss(p, regs);
-			kprobe_status = KPROBE_REENTER;
+			kcb->kprobe_status = KPROBE_REENTER;
 			return 1;
 		} else if (args->err == __IA64_BREAK_JPROBE) {
 			/*
 			 * jprobe instrumented function just completed
 			 */
-			p = current_kprobe;
+			p = __get_cpu_var(current_kprobe);
 			if (p->break_handler && p->break_handler(p, regs)) {
 				goto ss_probe;
 			}

@@ -668,8 +666,8 @@ static int __kprobes pre_kprobes_handler
 	 * in post_kprobes_handler()
 	 */
 	preempt_disable();
-	kprobe_status = KPROBE_HIT_ACTIVE;
-	set_current_kprobe(p);
+	set_current_kprobe(p, kcb);
+	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 
 	if (p->pre_handler && p->pre_handler(p, regs))
 		/*

@@ -681,7 +679,7 @@ static int __kprobes pre_kprobes_handler
 
 ss_probe:
 	prepare_ss(p, regs);
-	kprobe_status = KPROBE_HIT_SS;
+	kcb->kprobe_status = KPROBE_HIT_SS;
 	return 1;
 
 no_kprobe:

@@ -690,22 +688,25 @@ no_kprobe:
 
 static int __kprobes post_kprobes_handler(struct pt_regs *regs)
 {
-	if (!kprobe_running())
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (!cur)
 		return 0;
 
-	if ((kprobe_status != KPROBE_REENTER) && current_kprobe->post_handler) {
-		kprobe_status = KPROBE_HIT_SSDONE;
-		current_kprobe->post_handler(current_kprobe, regs, 0);
+	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
+		kcb->kprobe_status = KPROBE_HIT_SSDONE;
+		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(current_kprobe, regs);
+	resume_execution(cur, regs);
 
 	/*Restore back the original saved kprobes variables and continue. */
-	if (kprobe_status == KPROBE_REENTER) {
-		restore_previous_kprobe();
+	if (kcb->kprobe_status == KPROBE_REENTER) {
+		restore_previous_kprobe(kcb);
 		goto out;
 	}
-
+	reset_current_kprobe();
 	unlock_kprobes();
 
 out:

@@ -715,15 +716,18 @@ out:
 
 static int __kprobes kprobes_fault_handler(struct pt_regs *regs, int trapnr)
 {
-	if (!kprobe_running())
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (!cur)
 		return 0;
 
-	if (current_kprobe->fault_handler &&
-	    current_kprobe->fault_handler(current_kprobe, regs, trapnr))
+	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
 		return 1;
 
-	if (kprobe_status & KPROBE_HIT_SS) {
-		resume_execution(current_kprobe, regs);
+	if (kcb->kprobe_status & KPROBE_HIT_SS) {
+		resume_execution(cur, regs);
+		reset_current_kprobe();
 		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

@@ -761,9 +765,10 @@ int __kprobes setjmp_pre_handler(struct 
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 	unsigned long addr = ((struct fnptr *)(jp->entry))->ip;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	/* save architectural state */
-	jprobe_saved_regs = *regs;
+	kcb->jprobe_saved_regs = *regs;
 
 	/* after rfi, execute the jprobe instrumented function */
 	regs->cr_iip = addr & ~0xFULL;

@@ -781,7 +786,9 @@ int __kprobes setjmp_pre_handler(struct 
 
 int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
-	*regs = jprobe_saved_regs;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	*regs = kcb->jprobe_saved_regs;
 	return 1;
 }

include/asm-ia64/kprobes.h +13 -0

Index: linux-2.6.14-rc3/include/asm-ia64/kprobes.h
===================================================================
@@ -26,6 +26,7 @@
  */
 #include <linux/types.h>
 #include <linux/ptrace.h>
+#include <linux/percpu.h>
 #include <asm/break.h>
 
 #define MAX_INSN_SIZE   16

@@ -62,6 +63,18 @@ typedef struct _bundle {
 	} quad1;
 } __attribute__((__aligned__(16)))  bundle_t;
 
+struct prev_kprobe {
+	struct kprobe *kp;
+	unsigned long status;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+	unsigned long kprobe_status;
+	struct pt_regs jprobe_saved_regs;
+	struct prev_kprobe prev_kprobe;
+};
+
 #define JPROBE_ENTRY(pentry)	(kprobe_opcode_t *)pentry
 
 #define ARCH_SUPPORTS_KRETPROBES

Ananth N Mavinakayanahalli

author

2005-10-10 14:44 UTC #5 lore raw

PATCH 5/9 Kprobes: Track kprobe on a per_cpu basis - ppc64 changes

From: Ananth N Mavinakayanahalli <[email protected]>

PPC64 changes to track kprobe execution on a per-cpu basis. We now track the kprobe state machine independently on each cpu using an arch specific kprobe control block.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/ppc64/kernel/kprobes.c |   94 +++++++++++++++++++++++++-------------------
 include/asm-ppc64/kprobes.h |   15 +++++++
 2 files changed, 69 insertions(+), 40 deletions(-)

arch/ppc64/kernel/kprobes.c +54 ~~-40~~

Index: linux-2.6.14-rc3/arch/ppc64/kernel/kprobes.c
===================================================================
@@ -37,12 +37,8 @@
 #include <asm/sstep.h>
 
 static DECLARE_MUTEX(kprobe_mutex);
-
-static struct kprobe *current_kprobe;
-static unsigned long kprobe_status, kprobe_saved_msr;
-static struct kprobe *kprobe_prev;
-static unsigned long kprobe_status_prev, kprobe_saved_msr_prev;
-static struct pt_regs jprobe_saved_regs;
+DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {

@@ -108,18 +104,25 @@ static inline void prepare_singlestep(st
 		regs->nip = (unsigned long)p->ainsn.insn;
 }
 
-static inline void save_previous_kprobe(void)
+static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
+{
+	kcb->prev_kprobe.kp = kprobe_running();
+	kcb->prev_kprobe.status = kcb->kprobe_status;
+	kcb->prev_kprobe.saved_msr = kcb->kprobe_saved_msr;
+}
+
+static inline void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	kprobe_prev = current_kprobe;
-	kprobe_status_prev = kprobe_status;
-	kprobe_saved_msr_prev = kprobe_saved_msr;
+	__get_cpu_var(current_kprobe) = kcb->prev_kprobe.kp;
+	kcb->kprobe_status = kcb->prev_kprobe.status;
+	kcb->kprobe_saved_msr = kcb->prev_kprobe.saved_msr;
 }
 
-static inline void restore_previous_kprobe(void)
+static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
+				struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = kprobe_prev;
-	kprobe_status = kprobe_status_prev;
-	kprobe_saved_msr = kprobe_saved_msr_prev;
+	__get_cpu_var(current_kprobe) = p;
+	kcb->kprobe_saved_msr = regs->msr;
 }
 
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,

@@ -145,6 +148,7 @@ static inline int kprobe_handler(struct 
 	struct kprobe *p;
 	int ret = 0;
 	unsigned int *addr = (unsigned int *)regs->nip;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {

@@ -153,10 +157,10 @@ static inline int kprobe_handler(struct 
 		p = get_kprobe(addr);
 		if (p) {
 			kprobe_opcode_t insn = *p->ainsn.insn;
-			if (kprobe_status == KPROBE_HIT_SS &&
+			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 					is_trap(insn)) {
 				regs->msr &= ~MSR_SE;
-				regs->msr |= kprobe_saved_msr;
+				regs->msr |= kcb->kprobe_saved_msr;
 				unlock_kprobes();
 				goto no_kprobe;
 			}

@@ -166,15 +170,15 @@ static inline int kprobe_handler(struct 
 			 * just single step on the instruction of the new probe
 			 * without calling any user handlers.
 			 */
-			save_previous_kprobe();
-			current_kprobe = p;
-			kprobe_saved_msr = regs->msr;
+			save_previous_kprobe(kcb);
+			set_current_kprobe(p, regs, kcb);
+			kcb->kprobe_saved_msr = regs->msr;
 			p->nmissed++;
 			prepare_singlestep(p, regs);
-			kprobe_status = KPROBE_REENTER;
+			kcb->kprobe_status = KPROBE_REENTER;
 			return 1;
 		} else {
-			p = current_kprobe;
+			p = __get_cpu_var(current_kprobe);
 			if (p->break_handler && p->break_handler(p, regs)) {
 				goto ss_probe;
 			}

@@ -214,16 +218,15 @@ static inline int kprobe_handler(struct 
 	 * in post_kprobe_handler().
 	 */
 	preempt_disable();
-	kprobe_status = KPROBE_HIT_ACTIVE;
-	current_kprobe = p;
-	kprobe_saved_msr = regs->msr;
+	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+	set_current_kprobe(p, regs, kcb);
 	if (p->pre_handler && p->pre_handler(p, regs))
 		/* handler has already set things up, so skip ss setup */
 		return 1;
 
 ss_probe:
 	prepare_singlestep(p, regs);
-	kprobe_status = KPROBE_HIT_SS;
+	kcb->kprobe_status = KPROBE_HIT_SS;
 	return 1;
 
 no_kprobe:

@@ -292,6 +295,7 @@ int __kprobes trampoline_probe_handler(s
 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
 	regs->nip = orig_ret_address;
 
+	reset_current_kprobe();
 	unlock_kprobes();
 	preempt_enable_no_resched();

@@ -324,22 +328,26 @@ static void __kprobes resume_execution(s
 
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {
-	if (!kprobe_running())
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (!cur)
 		return 0;
 
-	if ((kprobe_status != KPROBE_REENTER) && current_kprobe->post_handler) {
-		kprobe_status = KPROBE_HIT_SSDONE;
-		current_kprobe->post_handler(current_kprobe, regs, 0);
+	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
+		kcb->kprobe_status = KPROBE_HIT_SSDONE;
+		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(current_kprobe, regs);
-	regs->msr |= kprobe_saved_msr;
+	resume_execution(cur, regs);
+	regs->msr |= kcb->kprobe_saved_msr;
 
 	/*Restore back the original saved kprobes variables and continue. */
-	if (kprobe_status == KPROBE_REENTER) {
-		restore_previous_kprobe();
+	if (kcb->kprobe_status == KPROBE_REENTER) {
+		restore_previous_kprobe(kcb);
 		goto out;
 	}
+	reset_current_kprobe();
 	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -358,15 +366,18 @@ out:
 /* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
-	if (current_kprobe->fault_handler
-	    && current_kprobe->fault_handler(current_kprobe, regs, trapnr))
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
 		return 1;
 
-	if (kprobe_status & KPROBE_HIT_SS) {
-		resume_execution(current_kprobe, regs);
+	if (kcb->kprobe_status & KPROBE_HIT_SS) {
+		resume_execution(cur, regs);
 		regs->msr &= ~MSR_SE;
-		regs->msr |= kprobe_saved_msr;
+		regs->msr |= kcb->kprobe_saved_msr;
 
+		reset_current_kprobe();
 		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

@@ -412,8 +423,9 @@ int __kprobes kprobe_exceptions_notify(s
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
-	memcpy(&jprobe_saved_regs, regs, sizeof(struct pt_regs));
+	memcpy(&kcb->jprobe_saved_regs, regs, sizeof(struct pt_regs));
 
 	/* setup return addr to the jprobe handler routine */
 	regs->nip = (unsigned long)(((func_descr_t *)jp->entry)->entry);

@@ -433,12 +445,14 @@ void __kprobes jprobe_return_end(void)
 
 int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
 	/*
 	 * FIXME - we should ideally be validating that we got here 'cos
 	 * of the "trap" in jprobe_return() above, before restoring the
 	 * saved regs...
 	 */
-	memcpy(regs, &jprobe_saved_regs, sizeof(struct pt_regs));
+	memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs));
 	return 1;
 }

include/asm-ppc64/kprobes.h +15 -0

Index: linux-2.6.14-rc3/include/asm-ppc64/kprobes.h
===================================================================
@@ -28,6 +28,7 @@
  */
 #include <linux/types.h>
 #include <linux/ptrace.h>
+#include <linux/percpu.h>
 
 struct pt_regs;

@@ -54,6 +55,20 @@ struct arch_specific_insn {
 	kprobe_opcode_t *insn;
 };
 
+struct prev_kprobe {
+	struct kprobe *kp;
+	unsigned long status;
+	unsigned long saved_msr;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+	unsigned long kprobe_status;
+	unsigned long kprobe_saved_msr;
+	struct pt_regs jprobe_saved_regs;
+	struct prev_kprobe prev_kprobe;
+};
+
 #ifdef CONFIG_KPROBES
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);

Ananth N Mavinakayanahalli

author

2005-10-10 14:45 UTC #6 lore raw

PATCH 6/9 Kprobes: Track kprobe on a per_cpu basis - sparc64 changes

Dave,
This patch is similar to the one you reviewed sometime back:
http://sourceware.org/ml/systemtap/2005-q3/msg00196.html

From: Ananth N Mavinakayanahalli <[email protected]>

Sparc64 changes to track kprobe execution on a per-cpu basis. We now track the kprobe state machine independently on each cpu using an arch specific kprobe control block.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/sparc64/kernel/kprobes.c |  131 +++++++++++++++++++++---------------------
 include/asm-sparc64/kprobes.h |   20 ++++++
 2 files changed, 87 insertions(+), 64 deletions(-)

arch/sparc64/kernel/kprobes.c +67 ~~-64~~

Index: linux-2.6.14-rc3/arch/sparc64/kernel/kprobes.c
===================================================================
@@ -38,6 +38,9 @@
  * - Mark that we are no longer actively in a kprobe.
  */
 
+DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	return 0;

@@ -66,46 +69,39 @@ void __kprobes arch_remove_kprobe(struct
 {
 }
 
-static struct kprobe *current_kprobe;
-static unsigned long current_kprobe_orig_tnpc;
-static unsigned long current_kprobe_orig_tstate_pil;
-static unsigned int kprobe_status;
-static struct kprobe *kprobe_prev;
-static unsigned long kprobe_orig_tnpc_prev;
-static unsigned long kprobe_orig_tstate_pil_prev;
-static unsigned int kprobe_status_prev;
-
-static inline void save_previous_kprobe(void)
+static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	kprobe_status_prev = kprobe_status;
-	kprobe_orig_tnpc_prev = current_kprobe_orig_tnpc;
-	kprobe_orig_tstate_pil_prev = current_kprobe_orig_tstate_pil;
-	kprobe_prev = current_kprobe;
+	kcb->prev_kprobe.kp = kprobe_running();
+	kcb->prev_kprobe.status = kcb->kprobe_status;
+	kcb->prev_kprobe.orig_tnpc = kcb->kprobe_orig_tnpc;
+	kcb->prev_kprobe.orig_tstate_pil = kcb->kprobe_orig_tstate_pil;
 }
 
-static inline void restore_previous_kprobe(void)
+static inline void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	kprobe_status = kprobe_status_prev;
-	current_kprobe_orig_tnpc = kprobe_orig_tnpc_prev;
-	current_kprobe_orig_tstate_pil = kprobe_orig_tstate_pil_prev;
-	current_kprobe = kprobe_prev;
+	__get_cpu_var(current_kprobe) = kcb->prev_kprobe.kp;
+	kcb->kprobe_status = kcb->prev_kprobe.status;
+	kcb->kprobe_orig_tnpc = kcb->prev_kprobe.orig_tnpc;
+	kcb->kprobe_orig_tstate_pil = kcb->prev_kprobe.orig_tstate_pil;
 }
 
-static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs)
+static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
+				struct kprobe_ctlblk *kcb)
 {
-	current_kprobe_orig_tnpc = regs->tnpc;
-	current_kprobe_orig_tstate_pil = (regs->tstate & TSTATE_PIL);
-	current_kprobe = p;
+	__get_cpu_var(current_kprobe) = p;
+	kcb->kprobe_orig_tnpc = regs->tnpc;
+	kcb->kprobe_orig_tstate_pil = (regs->tstate & TSTATE_PIL);
 }
 
-static inline void prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
+static inline void prepare_singlestep(struct kprobe *p, struct pt_regs *regs,
+			struct kprobe_ctlblk *kcb)
 {
 	regs->tstate |= TSTATE_PIL;
 
 	/*single step inline, if it a breakpoint instruction*/
 	if (p->opcode == BREAKPOINT_INSTRUCTION) {
 		regs->tpc = (unsigned long) p->addr;
-		regs->tnpc = current_kprobe_orig_tnpc;
+		regs->tnpc = kcb->kprobe_orig_tnpc;
 	} else {
 		regs->tpc = (unsigned long) &p->ainsn.insn[0];
 		regs->tnpc = (unsigned long) &p->ainsn.insn[1];

@@ -117,6 +113,7 @@ static int __kprobes kprobe_handler(stru
 	struct kprobe *p;
 	void *addr = (void *) regs->tpc;
 	int ret = 0;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	if (kprobe_running()) {
 		/* We *are* holding lock here, so this is safe.

@@ -124,9 +121,9 @@ static int __kprobes kprobe_handler(stru
 		 */
 		p = get_kprobe(addr);
 		if (p) {
-			if (kprobe_status == KPROBE_HIT_SS) {
+			if (kcb->kprobe_status == KPROBE_HIT_SS) {
 				regs->tstate = ((regs->tstate & ~TSTATE_PIL) |
-					current_kprobe_orig_tstate_pil);
+					kcb->kprobe_orig_tstate_pil);
 				unlock_kprobes();
 				goto no_kprobe;
 			}

@@ -136,14 +133,14 @@ static int __kprobes kprobe_handler(stru
 			 * just single step on the instruction of the new probe
 			 * without calling any user handlers.
 			 */
-			save_previous_kprobe();
-			set_current_kprobe(p, regs);
+			save_previous_kprobe(kcb);
+			set_current_kprobe(p, regs, kcb);
 			p->nmissed++;
-			kprobe_status = KPROBE_REENTER;
-			prepare_singlestep(p, regs);
+			kcb->kprobe_status = KPROBE_REENTER;
+			prepare_singlestep(p, regs, kcb);
 			return 1;
 		} else {
-			p = current_kprobe;
+			p = __get_cpu_var(current_kprobe);
 			if (p->break_handler && p->break_handler(p, regs))
 				goto ss_probe;
 		}

@@ -174,14 +171,14 @@ static int __kprobes kprobe_handler(stru
 	 * in post_kprobes_handler()
 	 */
 	preempt_disable();
-	set_current_kprobe(p, regs);
-	kprobe_status = KPROBE_HIT_ACTIVE;
+	set_current_kprobe(p, regs, kcb);
+	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 	if (p->pre_handler && p->pre_handler(p, regs))
 		return 1;
 
 ss_probe:
-	prepare_singlestep(p, regs);
-	kprobe_status = KPROBE_HIT_SS;
+	prepare_singlestep(p, regs, kcb);
+	kcb->kprobe_status = KPROBE_HIT_SS;
 	return 1;
 
 no_kprobe:

@@ -262,11 +259,12 @@ static void __kprobes retpc_fixup(struct
  * This function prepares to return from the post-single-step
  * breakpoint trap.
  */
-static void __kprobes resume_execution(struct kprobe *p, struct pt_regs *regs)
+static void __kprobes resume_execution(struct kprobe *p,
+		struct pt_regs *regs, struct kprobe_ctlblk *kcb)
 {
 	u32 insn = p->ainsn.insn[0];
 
-	regs->tpc = current_kprobe_orig_tnpc;
+	regs->tpc = kcb->kprobe_orig_tnpc;
 	regs->tnpc = relbranch_fixup(insn,
 				     (unsigned long) p->addr,
 				     (unsigned long) &p->ainsn.insn[0],

@@ -274,26 +272,30 @@ static void __kprobes resume_execution(s
 	retpc_fixup(regs, insn, (unsigned long) p->addr);
 
 	regs->tstate = ((regs->tstate & ~TSTATE_PIL) |
-			current_kprobe_orig_tstate_pil);
+			kcb->kprobe_orig_tstate_pil);
 }
 
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {
-	if (!kprobe_running())
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (!cur)
 		return 0;
 
-	if ((kprobe_status != KPROBE_REENTER) && current_kprobe->post_handler) {
-		kprobe_status = KPROBE_HIT_SSDONE;
-		current_kprobe->post_handler(current_kprobe, regs, 0);
+	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
+		kcb->kprobe_status = KPROBE_HIT_SSDONE;
+		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(current_kprobe, regs);
+	resume_execution(cur, regs, kcb);
 
 	/*Restore back the original saved kprobes variables and continue. */
-	if (kprobe_status == KPROBE_REENTER) {
-		restore_previous_kprobe();
+	if (kcb->kprobe_status == KPROBE_REENTER) {
+		restore_previous_kprobe(kcb);
 		goto out;
 	}
+	reset_current_kprobe();
 	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -304,13 +306,16 @@ out:
 /* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
-	if (current_kprobe->fault_handler
-	    && current_kprobe->fault_handler(current_kprobe, regs, trapnr))
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
 		return 1;
 
-	if (kprobe_status & KPROBE_HIT_SS) {
-		resume_execution(current_kprobe, regs);
+	if (kcb->kprobe_status & KPROBE_HIT_SS) {
+		resume_execution(cur, regs, kcb);
 
+		reset_current_kprobe();
 		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

@@ -370,24 +375,21 @@ asmlinkage void __kprobes kprobe_trap(un
 }
 
 /* Jprobes support.  */
-static struct pt_regs jprobe_saved_regs;
-static struct pt_regs *jprobe_saved_regs_location;
-static struct sparc_stackf jprobe_saved_stack;
-
 int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
-	jprobe_saved_regs_location = regs;
-	memcpy(&jprobe_saved_regs, regs, sizeof(*regs));
+	kcb->jprobe_saved_regs_location = regs;
+	memcpy(&(kcb->jprobe_saved_regs), regs, sizeof(*regs));
 
 	/* Save a whole stack frame, this gets arguments
 	 * pushed onto the stack after using up all the
 	 * arg registers.
 	 */
-	memcpy(&jprobe_saved_stack,
+	memcpy(&(kcb->jprobe_saved_stack),
 	       (char *) (regs->u_regs[UREG_FP] + STACK_BIAS),
-	       sizeof(jprobe_saved_stack));
+	       sizeof(kcb->jprobe_saved_stack));
 
 	regs->tpc  = (unsigned long) jp->entry;
 	regs->tnpc = ((unsigned long) jp->entry) + 0x4UL;

@@ -411,14 +413,15 @@ extern void __show_regs(struct pt_regs *
 int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	u32 *addr = (u32 *) regs->tpc;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	if (addr == (u32 *) jprobe_return_trap_instruction) {
-		if (jprobe_saved_regs_location != regs) {
+		if (kcb->jprobe_saved_regs_location != regs) {
 			printk("JPROBE: Current regs (%p) does not match "
 			       "saved regs (%p).\n",
-			       regs, jprobe_saved_regs_location);
+			       regs, kcb->jprobe_saved_regs_location);
 			printk("JPROBE: Saved registers\n");
-			__show_regs(jprobe_saved_regs_location);
+			__show_regs(kcb->jprobe_saved_regs_location);
 			printk("JPROBE: Current registers\n");
 			__show_regs(regs);
 			BUG();

@@ -427,11 +430,11 @@ int __kprobes longjmp_break_handler(stru
 		 * first so that UREG_FP is the original one for
 		 * the stack frame restore.
 		 */
-		memcpy(regs, &jprobe_saved_regs, sizeof(*regs));
+		memcpy(regs, &(kcb->jprobe_saved_regs), sizeof(*regs));
 
 		memcpy((char *) (regs->u_regs[UREG_FP] + STACK_BIAS),
-		       &jprobe_saved_stack,
-		       sizeof(jprobe_saved_stack));
+		       &(kcb->jprobe_saved_stack),
+		       sizeof(kcb->jprobe_saved_stack));
 
 		return 1;
 	}

include/asm-sparc64/kprobes.h +20 -0

Index: linux-2.6.14-rc3/include/asm-sparc64/kprobes.h
===================================================================
@@ -3,6 +3,7 @@
 
 #include <linux/config.h>
 #include <linux/types.h>
+#include <linux/percpu.h>
 
 typedef u32 kprobe_opcode_t;

@@ -18,6 +19,25 @@ struct arch_specific_insn {
 	kprobe_opcode_t insn[MAX_INSN_SIZE];
 };
 
+struct prev_kprobe {
+	struct kprobe *kp;
+	unsigned int status;
+	unsigned long orig_tnpc;
+	unsigned long orig_tstate_pil;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+	unsigned long kprobe_status;
+	unsigned long kprobe_orig_tnpc;
+	unsigned long kprobe_orig_tstate_pil;
+	long *jprobe_saved_esp;
+	struct pt_regs jprobe_saved_regs;
+	struct pt_regs *jprobe_saved_regs_location;
+	struct sparc_stackf jprobe_saved_stack;
+	struct prev_kprobe prev_kprobe;
+};
+
 #ifdef CONFIG_KPROBES
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);

Ananth N Mavinakayanahalli

author

2005-10-10 14:45 UTC #7 lore raw

PATCH 7/9 Kprobes: Track kprobe on a per_cpu basis - x86_64 changes

From: Ananth N Mavinakayanahalli <[email protected]>

x86_64 changes to track kprobe execution on a per-cpu basis. We now track the kprobe state machine independently on each cpu using a arch specific kprobe control block.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/x86_64/kernel/kprobes.c |  129 +++++++++++++++++++++++--------------------
 include/asm-x86_64/kprobes.h |   19 ++++++
 2 files changed, 89 insertions(+), 59 deletions(-)

arch/x86_64/kernel/kprobes.c +70 ~~-59~~

Index: linux-2.6.14-rc3/arch/x86_64/kernel/kprobes.c
===================================================================
@@ -44,17 +44,10 @@
 #include <asm/kdebug.h>
 
 static DECLARE_MUTEX(kprobe_mutex);
-
-static struct kprobe *current_kprobe;
-static unsigned long kprobe_status, kprobe_old_rflags, kprobe_saved_rflags;
-static struct kprobe *kprobe_prev;
-static unsigned long kprobe_status_prev, kprobe_old_rflags_prev, kprobe_saved_rflags_prev;
-static struct pt_regs jprobe_saved_regs;
-static long *jprobe_saved_rsp;
 void jprobe_return_end(void);
 
-/* copy of the kernel stack at the probe fire time */
-static kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE];
+DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 /*
  * returns non-zero if opcode modifies the interrupt flag.

@@ -236,29 +229,30 @@ void __kprobes arch_remove_kprobe(struct
 	up(&kprobe_mutex);
 }
 
-static inline void save_previous_kprobe(void)
+static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	kprobe_prev = current_kprobe;
-	kprobe_status_prev = kprobe_status;
-	kprobe_old_rflags_prev = kprobe_old_rflags;
-	kprobe_saved_rflags_prev = kprobe_saved_rflags;
+	kcb->prev_kprobe.kp = kprobe_running();
+	kcb->prev_kprobe.status = kcb->kprobe_status;
+	kcb->prev_kprobe.old_rflags = kcb->kprobe_old_rflags;
+	kcb->prev_kprobe.saved_rflags = kcb->kprobe_saved_rflags;
 }
 
-static inline void restore_previous_kprobe(void)
+static inline void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = kprobe_prev;
-	kprobe_status = kprobe_status_prev;
-	kprobe_old_rflags = kprobe_old_rflags_prev;
-	kprobe_saved_rflags = kprobe_saved_rflags_prev;
+	__get_cpu_var(current_kprobe) = kcb->prev_kprobe.kp;
+	kcb->kprobe_status = kcb->prev_kprobe.status;
+	kcb->kprobe_old_rflags = kcb->prev_kprobe.old_rflags;
+	kcb->kprobe_saved_rflags = kcb->prev_kprobe.saved_rflags;
 }
 
-static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs)
+static inline void set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
+				struct kprobe_ctlblk *kcb)
 {
-	current_kprobe = p;
-	kprobe_saved_rflags = kprobe_old_rflags
+	__get_cpu_var(current_kprobe) = p;
+	kcb->kprobe_saved_rflags = kcb->kprobe_old_rflags
 		= (regs->eflags & (TF_MASK | IF_MASK));
 	if (is_IF_modifier(p->ainsn.insn))
-		kprobe_saved_rflags &= ~IF_MASK;
+		kcb->kprobe_saved_rflags &= ~IF_MASK;
 }
 
 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)

@@ -301,6 +295,7 @@ int __kprobes kprobe_handler(struct pt_r
 	struct kprobe *p;
 	int ret = 0;
 	kprobe_opcode_t *addr = (kprobe_opcode_t *)(regs->rip - sizeof(kprobe_opcode_t));
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {

@@ -308,13 +303,13 @@ int __kprobes kprobe_handler(struct pt_r
 		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
-			if (kprobe_status == KPROBE_HIT_SS &&
+			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
-				regs->eflags |= kprobe_saved_rflags;
+				regs->eflags |= kcb->kprobe_saved_rflags;
 				unlock_kprobes();
 				goto no_kprobe;
-			} else if (kprobe_status == KPROBE_HIT_SSDONE) {
+			} else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 				/* TODO: Provide re-entrancy from
 				 * post_kprobes_handler() and avoid exception
 				 * stack corruption while single-stepping on

@@ -322,6 +317,7 @@ int __kprobes kprobe_handler(struct pt_r
 				 */
 				arch_disarm_kprobe(p);
 				regs->rip = (unsigned long)p->addr;
+				reset_current_kprobe();
 				ret = 1;
 			} else {
 				/* We have reentered the kprobe_handler(), since

@@ -331,15 +327,15 @@ int __kprobes kprobe_handler(struct pt_r
 				 * of the new probe without calling any user
 				 * handlers.
 				 */
-				save_previous_kprobe();
-				set_current_kprobe(p, regs);
+				save_previous_kprobe(kcb);
+				set_current_kprobe(p, regs, kcb);
 				p->nmissed++;
 				prepare_singlestep(p, regs);
-				kprobe_status = KPROBE_REENTER;
+				kcb->kprobe_status = KPROBE_REENTER;
 				return 1;
 			}
 		} else {
-			p = current_kprobe;
+			p = __get_cpu_var(current_kprobe);
 			if (p->break_handler && p->break_handler(p, regs)) {
 				goto ss_probe;
 			}

@@ -374,8 +370,8 @@ int __kprobes kprobe_handler(struct pt_r
 	 * in post_kprobe_handler()
 	 */
 	preempt_disable();
-	kprobe_status = KPROBE_HIT_ACTIVE;
-	set_current_kprobe(p, regs);
+	set_current_kprobe(p, regs, kcb);
+	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 
 	if (p->pre_handler && p->pre_handler(p, regs))
 		/* handler has already set things up, so skip ss setup */

@@ -383,7 +379,7 @@ int __kprobes kprobe_handler(struct pt_r
 
 ss_probe:
 	prepare_singlestep(p, regs);
-	kprobe_status = KPROBE_HIT_SS;
+	kcb->kprobe_status = KPROBE_HIT_SS;
 	return 1;
 
 no_kprobe:

@@ -451,6 +447,7 @@ int __kprobes trampoline_probe_handler(s
 	BUG_ON(!orig_ret_address || (orig_ret_address == trampoline_address));
 	regs->rip = orig_ret_address;
 
+	reset_current_kprobe();
 	unlock_kprobes();
 	preempt_enable_no_resched();

@@ -484,7 +481,8 @@ int __kprobes trampoline_probe_handler(s
  * that is atop the stack is the address following the copied instruction.
  * We need to make it the address following the original instruction.
  */
-static void __kprobes resume_execution(struct kprobe *p, struct pt_regs *regs)
+static void __kprobes resume_execution(struct kprobe *p,
+		struct pt_regs *regs, struct kprobe_ctlblk *kcb)
 {
 	unsigned long *tos = (unsigned long *)regs->rsp;
 	unsigned long next_rip = 0;

@@ -499,7 +497,7 @@ static void __kprobes resume_execution(s
 	switch (*insn) {
 	case 0x9c:		/* pushfl */
 		*tos &= ~(TF_MASK | IF_MASK);
-		*tos |= kprobe_old_rflags;
+		*tos |= kcb->kprobe_old_rflags;
 		break;
 	case 0xc3:		/* ret/lret */
 	case 0xcb:

@@ -544,24 +542,28 @@ static void __kprobes resume_execution(s
  */
 int __kprobes post_kprobe_handler(struct pt_regs *regs)
 {
-	if (!kprobe_running())
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (!cur)
 		return 0;
 
-	if ((kprobe_status != KPROBE_REENTER) && current_kprobe->post_handler) {
-		kprobe_status = KPROBE_HIT_SSDONE;
-		current_kprobe->post_handler(current_kprobe, regs, 0);
+	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
+		kcb->kprobe_status = KPROBE_HIT_SSDONE;
+		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(current_kprobe, regs);
-	regs->eflags |= kprobe_saved_rflags;
+	resume_execution(cur, regs, kcb);
+	regs->eflags |= kcb->kprobe_saved_rflags;
 
 	/* Restore the original saved kprobes variables and continue. */
-	if (kprobe_status == KPROBE_REENTER) {
-		restore_previous_kprobe();
+	if (kcb->kprobe_status == KPROBE_REENTER) {
+		restore_previous_kprobe(kcb);
 		goto out;
 	} else {
 		unlock_kprobes();
 	}
+	reset_current_kprobe();
 out:
 	preempt_enable_no_resched();

@@ -579,14 +581,17 @@ out:
 /* Interrupts disabled, kprobe_lock held. */
 int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
-	if (current_kprobe->fault_handler
-	    && current_kprobe->fault_handler(current_kprobe, regs, trapnr))
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
 		return 1;
 
-	if (kprobe_status & KPROBE_HIT_SS) {
-		resume_execution(current_kprobe, regs);
-		regs->eflags |= kprobe_old_rflags;
+	if (kcb->kprobe_status & KPROBE_HIT_SS) {
+		resume_execution(cur, regs, kcb);
+		regs->eflags |= kcb->kprobe_old_rflags;
 
+		reset_current_kprobe();
 		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

@@ -629,10 +634,11 @@ int __kprobes setjmp_pre_handler(struct 
 {
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 	unsigned long addr;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
-	jprobe_saved_regs = *regs;
-	jprobe_saved_rsp = (long *) regs->rsp;
-	addr = (unsigned long)jprobe_saved_rsp;
+	kcb->jprobe_saved_regs = *regs;
+	kcb->jprobe_saved_rsp = (long *) regs->rsp;
+	addr = (unsigned long)(kcb->jprobe_saved_rsp);
 	/*
 	 * As Linus pointed out, gcc assumes that the callee
 	 * owns the argument space and could overwrite it, e.g.

@@ -640,7 +646,8 @@ int __kprobes setjmp_pre_handler(struct 
 	 * we also save and restore enough stack bytes to cover
 	 * the argument area.
 	 */
-	memcpy(jprobes_stack, (kprobe_opcode_t *) addr, MIN_STACK_SIZE(addr));
+	memcpy(kcb->jprobes_stack, (kprobe_opcode_t *)addr,
+			MIN_STACK_SIZE(addr));
 	regs->eflags &= ~IF_MASK;
 	regs->rip = (unsigned long)(jp->entry);
 	return 1;

@@ -648,34 +655,38 @@ int __kprobes setjmp_pre_handler(struct 
 
 void __kprobes jprobe_return(void)
 {
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
 	asm volatile ("       xchg   %%rbx,%%rsp     \n"
 		      "       int3			\n"
 		      "       .globl jprobe_return_end	\n"
 		      "       jprobe_return_end:	\n"
 		      "       nop			\n"::"b"
-		      (jprobe_saved_rsp):"memory");
+		      (kcb->jprobe_saved_rsp):"memory");
 }
 
 int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 	u8 *addr = (u8 *) (regs->rip - 1);
-	unsigned long stack_addr = (unsigned long)jprobe_saved_rsp;
+	unsigned long stack_addr = (unsigned long)(kcb->jprobe_saved_rsp);
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 
 	if ((addr > (u8 *) jprobe_return) && (addr < (u8 *) jprobe_return_end)) {
-		if ((long *)regs->rsp != jprobe_saved_rsp) {
+		if ((long *)regs->rsp != kcb->jprobe_saved_rsp) {
 			struct pt_regs *saved_regs =
-			    container_of(jprobe_saved_rsp, struct pt_regs, rsp);
+			    container_of(kcb->jprobe_saved_rsp,
+					    struct pt_regs, rsp);
 			printk("current rsp %p does not match saved rsp %p\n",
-			       (long *)regs->rsp, jprobe_saved_rsp);
+			       (long *)regs->rsp, kcb->jprobe_saved_rsp);
 			printk("Saved registers for jprobe %p\n", jp);
 			show_registers(saved_regs);
 			printk("Current registers\n");
 			show_registers(regs);
 			BUG();
 		}
-		*regs = jprobe_saved_regs;
-		memcpy((kprobe_opcode_t *) stack_addr, jprobes_stack,
+		*regs = kcb->jprobe_saved_regs;
+		memcpy((kprobe_opcode_t *) stack_addr, kcb->jprobes_stack,
 		       MIN_STACK_SIZE(stack_addr));
 		return 1;
 	}

include/asm-x86_64/kprobes.h +19 -0

Index: linux-2.6.14-rc3/include/asm-x86_64/kprobes.h
===================================================================
@@ -25,6 +25,7 @@
  */
 #include <linux/types.h>
 #include <linux/ptrace.h>
+#include <linux/percpu.h>
 
 struct pt_regs;

@@ -48,6 +49,24 @@ struct arch_specific_insn {
 	kprobe_opcode_t *insn;
 };
 
+struct prev_kprobe {
+	struct kprobe *kp;
+	unsigned long status;
+	unsigned long old_rflags;
+	unsigned long saved_rflags;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+	unsigned long kprobe_status;
+	unsigned long kprobe_old_rflags;
+	unsigned long kprobe_saved_rflags;
+	long *jprobe_saved_rsp;
+	struct pt_regs jprobe_saved_regs;
+	kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE];
+	struct prev_kprobe prev_kprobe;
+};
+
 /* trap3/1 are intr gates for kprobes.  So, restore the status of IF,
  * if necessary, before executing the original int3/1 (trap) handler.
  */

Ananth N Mavinakayanahalli

author

2005-10-10 14:47 UTC #8 lore raw

PATCH 8/9 Kprobes: Use RCU for (un)register synchronization - base changes

From: Ananth N Mavinakayanahalli <[email protected]>

Changes to the base kprobes infrastructure to use RCU for synchronization during kprobe registration and unregistration. These changes coupled with the arch kprobe changes (next in series):

a. serialize registration and unregistration of kprobes.
b. enable lockless execution of handlers. Handlers can now run in parallel.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 include/linux/kprobes.h |    9 +---
 kernel/kprobes.c        |  101 +++++++++++++++++++-----------------------------
 2 files changed, 45 insertions(+), 65 deletions(-)

include/linux/kprobes.h +4 -5

Index: linux-2.6.14-rc3/include/linux/kprobes.h
===================================================================
@@ -34,6 +34,8 @@
 #include <linux/notifier.h>
 #include <linux/smp.h>
 #include <linux/percpu.h>
+#include <linux/spinlock.h>
+#include <linux/rcupdate.h>
 
 #include <asm/kprobes.h>

@@ -146,10 +148,7 @@ struct kretprobe_instance {
 };
 
 #ifdef CONFIG_KPROBES
-/* Locks kprobe: irq must be disabled */
-void lock_kprobes(void);
-void unlock_kprobes(void);
-
+extern spinlock_t kretprobe_lock;
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_copy_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);

@@ -160,7 +159,7 @@ extern void show_registers(struct pt_reg
 extern kprobe_opcode_t *get_insn_slot(void);
 extern void free_insn_slot(kprobe_opcode_t *slot);
 
-/* Get the kprobe at this addr (if any).  Must have called lock_kprobes */
+/* Get the kprobe at this addr (if any) - called under a rcu_read_lock() */
 struct kprobe *get_kprobe(void *addr);
 struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);

kernel/kprobes.c +41 ~~-60~~

259 lines not on this page · raw message

Ananth N Mavinakayanahalli

author

2005-10-10 14:48 UTC #9 lore raw

PATCH 9/9 Kprobes: Use RCU for (un)register synchronization - arch changes

From: Ananth N Mavinakayanahalli <[email protected]>

Changes to the arch kprobes infrastructure to take advantage of the locking changes introduced by usage of RCU for synchronization. All handlers are now run without any locks held, so they have to be re-entrant or provide their own synchronization.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/i386/kernel/kprobes.c    |   22 +++++++---------------
 arch/ia64/kernel/kprobes.c    |   16 ++++++----------
 arch/ppc64/kernel/kprobes.c   |   24 ++++++------------------
 arch/sparc64/kernel/kprobes.c |   14 ++------------
 arch/x86_64/kernel/kprobes.c  |   25 ++++++-------------------
 5 files changed, 27 insertions(+), 74 deletions(-)

arch/i386/kernel/kprobes.c +7 ~~-15~~

Index: linux-2.6.14-rc3/arch/i386/kernel/kprobes.c
===================================================================
@@ -31,7 +31,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/preempt.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>

@@ -123,6 +122,7 @@ static inline void prepare_singlestep(st
 		regs->eip = (unsigned long)&p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -168,15 +168,12 @@ static int __kprobes kprobe_handler(stru
 	}
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_eflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -197,14 +194,11 @@ static int __kprobes kprobe_handler(stru
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (regs->eflags & VM_MASK) {
 			/* We are in virtual-8086 mode. Return 0 */
 			goto no_kprobe;

@@ -268,9 +262,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -310,7 +305,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->eip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -395,7 +390,7 @@ static void __kprobes resume_execution(s
 
 /*
  * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
+ * remain disabled thoroughout this function.
  */
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {

@@ -419,7 +414,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -434,7 +428,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -448,7 +441,6 @@ static inline int kprobe_fault_handler(s
 		regs->eflags |= kcb->kprobe_old_eflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

@@ -463,7 +455,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))

@@ -482,7 +474,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

arch/ia64/kernel/kprobes.c +6 ~~-10~~

Index: linux-2.6.14-rc3/arch/ia64/kernel/kprobes.c
===================================================================
@@ -26,7 +26,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -343,10 +342,11 @@ int __kprobes trampoline_probe_handler(s
 	struct kretprobe_instance *ri = NULL;
 	struct hlist_head *head;
 	struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =
 		((struct fnptr *)kretprobe_trampoline)->ip;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -386,7 +386,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->cr_iip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -397,6 +397,7 @@ int __kprobes trampoline_probe_handler(s
         return 1;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -612,7 +613,6 @@ static int __kprobes pre_kprobes_handler
 			if ((kcb->kprobe_status == KPROBE_HIT_SS) &&
 	 		     (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)) {
   				ia64_psr(regs)->ss = 0;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the pre_kprobe_handler(), since

@@ -641,10 +641,8 @@ static int __kprobes pre_kprobes_handler
 		}
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (!is_ia64_break_inst(regs)) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -707,7 +705,6 @@ static int __kprobes post_kprobes_handle
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 
 out:
 	preempt_enable_no_resched();

@@ -728,7 +725,6 @@ static int __kprobes kprobes_fault_handl
 	if (kcb->kprobe_status & KPROBE_HIT_SS) {
 		resume_execution(cur, regs);
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

@@ -741,7 +737,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();
 	switch(val) {
 	case DIE_BREAK:
 		if (pre_kprobes_handler(args))

@@ -757,7 +753,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

arch/ppc64/kernel/kprobes.c +6 ~~-18~~

Index: linux-2.6.14-rc3/arch/ppc64/kernel/kprobes.c
===================================================================
@@ -30,7 +30,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/preempt.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>

@@ -125,6 +124,7 @@ static inline void set_current_kprobe(st
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -152,8 +152,6 @@ static inline int kprobe_handler(struct 
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			kprobe_opcode_t insn = *p->ainsn.insn;

@@ -161,7 +159,6 @@ static inline int kprobe_handler(struct 
 					is_trap(insn)) {
 				regs->msr &= ~MSR_SE;
 				regs->msr |= kcb->kprobe_saved_msr;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -183,14 +180,11 @@ static inline int kprobe_handler(struct 
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * PowerPC has multiple variants of the "trap"

@@ -254,9 +248,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -296,7 +291,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->nip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -348,7 +343,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -363,7 +357,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -378,7 +371,6 @@ static inline int kprobe_fault_handler(s
 		regs->msr |= kcb->kprobe_saved_msr;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

@@ -393,11 +385,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	/*
-	 * Interrupts are not disabled here.  We need to disable
-	 * preemption, because kprobe_running() uses smp_processor_id().
-	 */
-	preempt_disable();
+	rcu_read_lock();
 	switch (val) {
 	case DIE_BPT:
 		if (kprobe_handler(args->regs))

@@ -416,7 +404,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable_no_resched();
+	rcu_read_unlock();
 	return ret;
 }

arch/sparc64/kernel/kprobes.c +2 ~~-12~~

Index: linux-2.6.14-rc3/arch/sparc64/kernel/kprobes.c
===================================================================
@@ -116,15 +116,11 @@ static int __kprobes kprobe_handler(stru
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		 * Disarm the probe we just hit, and ignore it.
-		 */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS) {
 				regs->tstate = ((regs->tstate & ~TSTATE_PIL) |
 					kcb->kprobe_orig_tstate_pil);
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -144,14 +140,11 @@ static int __kprobes kprobe_handler(stru
 			if (p->break_handler && p->break_handler(p, regs))
 				goto ss_probe;
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*(u32 *)addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -296,14 +289,12 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();
 
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -316,7 +307,6 @@ static inline int kprobe_fault_handler(s
 		resume_execution(cur, regs, kcb);
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

@@ -331,7 +321,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();
 	switch (val) {
 	case DIE_DEBUG:
 		if (kprobe_handler(args->regs))

@@ -350,7 +340,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

arch/x86_64/kernel/kprobes.c +6 ~~-19~~

Index: linux-2.6.14-rc3/arch/x86_64/kernel/kprobes.c
===================================================================
@@ -34,7 +34,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -266,6 +265,7 @@ static void __kprobes prepare_singlestep
 		regs->rip = (unsigned long)p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -299,15 +299,12 @@ int __kprobes kprobe_handler(struct pt_r
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_rflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			} else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 				/* TODO: Provide re-entrancy from

@@ -340,14 +337,11 @@ int __kprobes kprobe_handler(struct pt_r
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -406,9 +400,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -448,7 +443,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->rip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -536,10 +531,6 @@ static void __kprobes resume_execution(s
 	}
 }
 
-/*
- * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
- */
 int __kprobes post_kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *cur = kprobe_running();

@@ -560,8 +551,6 @@ int __kprobes post_kprobe_handler(struct
 	if (kcb->kprobe_status == KPROBE_REENTER) {
 		restore_previous_kprobe(kcb);
 		goto out;
-	} else {
-		unlock_kprobes();
 	}
 	reset_current_kprobe();
 out:

@@ -578,7 +567,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -592,7 +580,6 @@ int __kprobes kprobe_fault_handler(struc
 		regs->eflags |= kcb->kprobe_old_rflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

@@ -607,7 +594,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))

@@ -626,7 +613,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Paul E. McKenney

2005-10-18 14:01 UTC #10 lore raw

Re: [PATCH 9/9] Kprobes: Use RCU for (un)register synchronization - arch changes

Quoted 6 lines · Ananth N Mavinakayanahalli

From: Ananth N Mavinakayanahalli <[email protected]>
Changes to the arch kprobes infrastructure to take advantage of the locking changes introduced by usage of RCU for synchronization. All handlers are now run without any locks held, so they have to be re-entrant or provide their own synchronization.

And a few very similar questions here as well...

Thanx, Paul

Quoted 117 lines

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/i386/kernel/kprobes.c    |   22 +++++++---------------
 arch/ia64/kernel/kprobes.c    |   16 ++++++----------
 arch/ppc64/kernel/kprobes.c   |   24 ++++++------------------
 arch/sparc64/kernel/kprobes.c |   14 ++------------
 arch/x86_64/kernel/kprobes.c  |   25 ++++++-------------------
 5 files changed, 27 insertions(+), 74 deletions(-)

Index: linux-2.6.14-rc3/arch/i386/kernel/kprobes.c
===================================================================

arch/i386/kernel/kprobes.c +5 ~~-13~~

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

@@ -123,6 +122,7 @@ static inline void prepare_singlestep(st
 		regs->eip = (unsigned long)&p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -168,15 +168,12 @@ static int __kprobes kprobe_handler(stru
 	}
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_eflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -197,14 +194,11 @@ static int __kprobes kprobe_handler(stru
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (regs->eflags & VM_MASK) {
 			/* We are in virtual-8086 mode. Return 0 */
 			goto no_kprobe;

@@ -268,9 +262,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -310,7 +305,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->eip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -395,7 +390,7 @@ static void __kprobes resume_execution(s
 
 /*
  * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
+ * remain disabled thoroughout this function.
  */
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {

@@ -419,7 +414,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -434,7 +428,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -448,7 +441,6 @@ static inline int kprobe_fault_handler(s
 		regs->eflags |= kcb->kprobe_old_eflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/i386/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 458

Quoted 5 lines

arch/i386/kernel/kprobes.c +1 -1

@@ -463,7 +455,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

If synchronize_sched() is used on the update side, this needs to remain preempt_disable() rather than rcu_read_lock().

Quoted 90 lines

arch/i386/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))

@@ -482,7 +474,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/ia64/kernel/kprobes.c
===================================================================

arch/ia64/kernel/kprobes.c +4 -8

@@ -26,7 +26,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -343,10 +342,11 @@ int __kprobes trampoline_probe_handler(s
 	struct kretprobe_instance *ri = NULL;
 	struct hlist_head *head;
 	struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =
 		((struct fnptr *)kretprobe_trampoline)->ip;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -386,7 +386,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->cr_iip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -397,6 +397,7 @@ int __kprobes trampoline_probe_handler(s
         return 1;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -612,7 +613,6 @@ static int __kprobes pre_kprobes_handler
 			if ((kcb->kprobe_status == KPROBE_HIT_SS) &&
 	 		     (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)) {
   				ia64_psr(regs)->ss = 0;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the pre_kprobe_handler(), since

@@ -641,10 +641,8 @@ static int __kprobes pre_kprobes_handler
 		}
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (!is_ia64_break_inst(regs)) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -707,7 +705,6 @@ static int __kprobes post_kprobes_handle
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 
 out:
 	preempt_enable_no_resched();

@@ -728,7 +725,6 @@ static int __kprobes kprobes_fault_handl
 	if (kcb->kprobe_status & KPROBE_HIT_SS) {
 		resume_execution(cur, regs);
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

arch/ia64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 740

Quoted 5 lines

arch/ia64/kernel/kprobes.c +1 -1

@@ -741,7 +737,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

Ditto here...

Quoted 110 lines

arch/ia64/kernel/kprobes.c +1 -1

 	switch(val) {
 	case DIE_BREAK:
 		if (pre_kprobes_handler(args))

@@ -757,7 +753,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/ppc64/kernel/kprobes.c
===================================================================

arch/ppc64/kernel/kprobes.c +4 ~~-12~~

@@ -30,7 +30,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/preempt.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>

@@ -125,6 +124,7 @@ static inline void set_current_kprobe(st
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -152,8 +152,6 @@ static inline int kprobe_handler(struct 
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			kprobe_opcode_t insn = *p->ainsn.insn;

@@ -161,7 +159,6 @@ static inline int kprobe_handler(struct 
 					is_trap(insn)) {
 				regs->msr &= ~MSR_SE;
 				regs->msr |= kcb->kprobe_saved_msr;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -183,14 +180,11 @@ static inline int kprobe_handler(struct 
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * PowerPC has multiple variants of the "trap"

@@ -254,9 +248,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -296,7 +291,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->nip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -348,7 +343,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -363,7 +357,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -378,7 +371,6 @@ static inline int kprobe_fault_handler(s
 		regs->msr |= kcb->kprobe_saved_msr;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/ppc64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 388

Quoted 9 lines

arch/ppc64/kernel/kprobes.c +1 -5

@@ -393,11 +385,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	/*
-	 * Interrupts are not disabled here.  We need to disable
-	 * preemption, because kprobe_running() uses smp_processor_id().
-	 */
-	preempt_disable();
+	rcu_read_lock();

And here...

Quoted 71 lines

arch/ppc64/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_BPT:
 		if (kprobe_handler(args->regs))

@@ -416,7 +404,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable_no_resched();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/sparc64/kernel/kprobes.c
===================================================================

arch/sparc64/kernel/kprobes.c +0 ~~-10~~

@@ -116,15 +116,11 @@ static int __kprobes kprobe_handler(stru
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		 * Disarm the probe we just hit, and ignore it.
-		 */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS) {
 				regs->tstate = ((regs->tstate & ~TSTATE_PIL) |
 					kcb->kprobe_orig_tstate_pil);
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -144,14 +140,11 @@ static int __kprobes kprobe_handler(stru
 			if (p->break_handler && p->break_handler(p, regs))
 				goto ss_probe;
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*(u32 *)addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -296,14 +289,12 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();
 
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -316,7 +307,6 @@ static inline int kprobe_fault_handler(s
 		resume_execution(cur, regs, kcb);
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/sparc64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 324

Quoted 5 lines

arch/sparc64/kernel/kprobes.c +1 -1

@@ -331,7 +321,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

As well as here...

Quoted 121 lines

arch/sparc64/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_DEBUG:
 		if (kprobe_handler(args->regs))

@@ -350,7 +340,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/x86_64/kernel/kprobes.c
===================================================================

arch/x86_64/kernel/kprobes.c +4 ~~-17~~

@@ -34,7 +34,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -266,6 +265,7 @@ static void __kprobes prepare_singlestep
 		regs->rip = (unsigned long)p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -299,15 +299,12 @@ int __kprobes kprobe_handler(struct pt_r
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_rflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			} else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 				/* TODO: Provide re-entrancy from

@@ -340,14 +337,11 @@ int __kprobes kprobe_handler(struct pt_r
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -406,9 +400,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -448,7 +443,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->rip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -536,10 +531,6 @@ static void __kprobes resume_execution(s
 	}
 }
 
-/*
- * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
- */
 int __kprobes post_kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *cur = kprobe_running();

@@ -560,8 +551,6 @@ int __kprobes post_kprobe_handler(struct
 	if (kcb->kprobe_status == KPROBE_REENTER) {
 		restore_previous_kprobe(kcb);
 		goto out;
-	} else {
-		unlock_kprobes();
 	}
 	reset_current_kprobe();
 out:

@@ -578,7 +567,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -592,7 +580,6 @@ int __kprobes kprobe_fault_handler(struc
 		regs->eflags |= kcb->kprobe_old_rflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/x86_64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 597

Quoted 5 lines

arch/x86_64/kernel/kprobes.c +1 -1

@@ -607,7 +594,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

As well as here yet again...

Quoted 19 lines

arch/x86_64/kernel/kprobes.c +1 -2
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))
@@ -626,7 +613,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Ananth N Mavinakayanahalli

author

2005-10-18 14:45 UTC #11 lore raw

Re: [PATCH 9/9] Kprobes: Use RCU for (un)register synchronization - arch changes

Quoted 9 lines · Paul E. McKenney

Quoted 6 lines · Ananth N Mavinakayanahalli
From: Ananth N Mavinakayanahalli <[email protected]>
Changes to the arch kprobes infrastructure to take advantage of the locking changes introduced by usage of RCU for synchronization. All handlers are now run without any locks held, so they have to be re-entrant or provide their own synchronization.
And a few very similar questions here as well...

Replies inline...

Ananth

Quoted 122 lines

Thanx, Paul

Quoted 120 lines

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/i386/kernel/kprobes.c    |   22 +++++++---------------
 arch/ia64/kernel/kprobes.c    |   16 ++++++----------
 arch/ppc64/kernel/kprobes.c   |   24 ++++++------------------
 arch/sparc64/kernel/kprobes.c |   14 ++------------
 arch/x86_64/kernel/kprobes.c  |   25 ++++++-------------------
 5 files changed, 27 insertions(+), 74 deletions(-)

Index: linux-2.6.14-rc3/arch/i386/kernel/kprobes.c
===================================================================

arch/i386/kernel/kprobes.c +5 ~~-13~~

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

@@ -123,6 +122,7 @@ static inline void prepare_singlestep(st
 		regs->eip = (unsigned long)&p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -168,15 +168,12 @@ static int __kprobes kprobe_handler(stru
 	}
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_eflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -197,14 +194,11 @@ static int __kprobes kprobe_handler(stru
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (regs->eflags & VM_MASK) {
 			/* We are in virtual-8086 mode. Return 0 */
 			goto no_kprobe;

@@ -268,9 +262,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -310,7 +305,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->eip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -395,7 +390,7 @@ static void __kprobes resume_execution(s
 
 /*
  * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
+ * remain disabled thoroughout this function.
  */
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {

@@ -419,7 +414,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -434,7 +428,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -448,7 +441,6 @@ static inline int kprobe_fault_handler(s
 		regs->eflags |= kcb->kprobe_old_eflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

@@ -463,7 +455,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;

Quoted 5 lines

arch/i386/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 458
Quoted 2 lines
arch/i386/kernel/kprobes.c +1 -1
-	preempt_disable();
+	rcu_read_lock();
If synchronize_sched() is used on the update side, this needs to remain preempt_disable() rather than rcu_read_lock().

Kprobe handlers can't block/sleep. So the idea is to depend on a schedule() event to ensure handlers have executed. This and the others you have pointed out can surely be preempt_disable().

Quoted 447 lines

Quoted 90 lines

arch/i386/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))

@@ -482,7 +474,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/ia64/kernel/kprobes.c
===================================================================

arch/ia64/kernel/kprobes.c +4 -8

@@ -26,7 +26,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -343,10 +342,11 @@ int __kprobes trampoline_probe_handler(s
 	struct kretprobe_instance *ri = NULL;
 	struct hlist_head *head;
 	struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =
 		((struct fnptr *)kretprobe_trampoline)->ip;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -386,7 +386,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->cr_iip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -397,6 +397,7 @@ int __kprobes trampoline_probe_handler(s
         return 1;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -612,7 +613,6 @@ static int __kprobes pre_kprobes_handler
 			if ((kcb->kprobe_status == KPROBE_HIT_SS) &&
 	 		     (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)) {
   				ia64_psr(regs)->ss = 0;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the pre_kprobe_handler(), since

@@ -641,10 +641,8 @@ static int __kprobes pre_kprobes_handler
 		}
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (!is_ia64_break_inst(regs)) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -707,7 +705,6 @@ static int __kprobes post_kprobes_handle
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 
 out:
 	preempt_enable_no_resched();

@@ -728,7 +725,6 @@ static int __kprobes kprobes_fault_handl
 	if (kcb->kprobe_status & KPROBE_HIT_SS) {
 		resume_execution(cur, regs);
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

arch/ia64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 740

Quoted 5 lines

arch/ia64/kernel/kprobes.c +1 -1

@@ -741,7 +737,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

Ditto here...

Quoted 110 lines

arch/ia64/kernel/kprobes.c +1 -1

 	switch(val) {
 	case DIE_BREAK:
 		if (pre_kprobes_handler(args))

@@ -757,7 +753,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/ppc64/kernel/kprobes.c
===================================================================

arch/ppc64/kernel/kprobes.c +4 ~~-12~~

@@ -30,7 +30,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/preempt.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>

@@ -125,6 +124,7 @@ static inline void set_current_kprobe(st
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -152,8 +152,6 @@ static inline int kprobe_handler(struct 
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			kprobe_opcode_t insn = *p->ainsn.insn;

@@ -161,7 +159,6 @@ static inline int kprobe_handler(struct 
 					is_trap(insn)) {
 				regs->msr &= ~MSR_SE;
 				regs->msr |= kcb->kprobe_saved_msr;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -183,14 +180,11 @@ static inline int kprobe_handler(struct 
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * PowerPC has multiple variants of the "trap"

@@ -254,9 +248,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -296,7 +291,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->nip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -348,7 +343,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -363,7 +357,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -378,7 +371,6 @@ static inline int kprobe_fault_handler(s
 		regs->msr |= kcb->kprobe_saved_msr;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/ppc64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 388

Quoted 9 lines

arch/ppc64/kernel/kprobes.c +1 -5

@@ -393,11 +385,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	/*
-	 * Interrupts are not disabled here.  We need to disable
-	 * preemption, because kprobe_running() uses smp_processor_id().
-	 */
-	preempt_disable();
+	rcu_read_lock();

And here...

Quoted 71 lines

arch/ppc64/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_BPT:
 		if (kprobe_handler(args->regs))

@@ -416,7 +404,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable_no_resched();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/sparc64/kernel/kprobes.c
===================================================================

arch/sparc64/kernel/kprobes.c +0 ~~-10~~

@@ -116,15 +116,11 @@ static int __kprobes kprobe_handler(stru
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		 * Disarm the probe we just hit, and ignore it.
-		 */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS) {
 				regs->tstate = ((regs->tstate & ~TSTATE_PIL) |
 					kcb->kprobe_orig_tstate_pil);
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -144,14 +140,11 @@ static int __kprobes kprobe_handler(stru
 			if (p->break_handler && p->break_handler(p, regs))
 				goto ss_probe;
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*(u32 *)addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -296,14 +289,12 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();
 
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -316,7 +307,6 @@ static inline int kprobe_fault_handler(s
 		resume_execution(cur, regs, kcb);
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/sparc64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 324

Quoted 5 lines

arch/sparc64/kernel/kprobes.c +1 -1

@@ -331,7 +321,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

As well as here...

Quoted 121 lines

arch/sparc64/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_DEBUG:
 		if (kprobe_handler(args->regs))

@@ -350,7 +340,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/x86_64/kernel/kprobes.c
===================================================================

arch/x86_64/kernel/kprobes.c +4 ~~-17~~

@@ -34,7 +34,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -266,6 +265,7 @@ static void __kprobes prepare_singlestep
 		regs->rip = (unsigned long)p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -299,15 +299,12 @@ int __kprobes kprobe_handler(struct pt_r
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_rflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			} else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 				/* TODO: Provide re-entrancy from

@@ -340,14 +337,11 @@ int __kprobes kprobe_handler(struct pt_r
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -406,9 +400,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -448,7 +443,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->rip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -536,10 +531,6 @@ static void __kprobes resume_execution(s
 	}
 }
 
-/*
- * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
- */
 int __kprobes post_kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *cur = kprobe_running();

@@ -560,8 +551,6 @@ int __kprobes post_kprobe_handler(struct
 	if (kcb->kprobe_status == KPROBE_REENTER) {
 		restore_previous_kprobe(kcb);
 		goto out;
-	} else {
-		unlock_kprobes();
 	}
 	reset_current_kprobe();
 out:

@@ -578,7 +567,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -592,7 +580,6 @@ int __kprobes kprobe_fault_handler(struc
 		regs->eflags |= kcb->kprobe_old_rflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/x86_64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 597

Quoted 5 lines

arch/x86_64/kernel/kprobes.c +1 -1

@@ -607,7 +594,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

As well as here yet again...

Quoted 19 lines

arch/x86_64/kernel/kprobes.c +1 -2
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))
@@ -626,7 +613,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Paul E. McKenney

2005-10-18 16:34 UTC #12 lore raw

Re: [PATCH 9/9] Kprobes: Use RCU for (un)register synchronization - arch changes

Quoted 9 lines · Ananth N Mavinakayanahalli

Quoted 8 lines · Paul E. McKenney
Quoted 6 lines · Ananth N Mavinakayanahalli
From: Ananth N Mavinakayanahalli <[email protected]>
Changes to the arch kprobes infrastructure to take advantage of the locking changes introduced by usage of RCU for synchronization. All handlers are now run without any locks held, so they have to be re-entrant or provide their own synchronization.

Quoted 5 lines

Quoted 1 line
And a few very similar questions here as well...
Replies inline...
Ananth

Rereplies also inline!

Quoted 128 lines

Quoted 122 lines

Thanx, Paul

Quoted 120 lines

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 arch/i386/kernel/kprobes.c    |   22 +++++++---------------
 arch/ia64/kernel/kprobes.c    |   16 ++++++----------
 arch/ppc64/kernel/kprobes.c   |   24 ++++++------------------
 arch/sparc64/kernel/kprobes.c |   14 ++------------
 arch/x86_64/kernel/kprobes.c  |   25 ++++++-------------------
 5 files changed, 27 insertions(+), 74 deletions(-)

Index: linux-2.6.14-rc3/arch/i386/kernel/kprobes.c
===================================================================

arch/i386/kernel/kprobes.c +5 ~~-13~~

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

@@ -123,6 +122,7 @@ static inline void prepare_singlestep(st
 		regs->eip = (unsigned long)&p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -168,15 +168,12 @@ static int __kprobes kprobe_handler(stru
 	}
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_eflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -197,14 +194,11 @@ static int __kprobes kprobe_handler(stru
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (regs->eflags & VM_MASK) {
 			/* We are in virtual-8086 mode. Return 0 */
 			goto no_kprobe;

@@ -268,9 +262,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -310,7 +305,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->eip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -395,7 +390,7 @@ static void __kprobes resume_execution(s
 
 /*
  * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
+ * remain disabled thoroughout this function.
  */
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {

@@ -419,7 +414,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -434,7 +428,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -448,7 +441,6 @@ static inline int kprobe_fault_handler(s
 		regs->eflags |= kcb->kprobe_old_eflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

@@ -463,7 +455,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;

Quoted 5 lines

arch/i386/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 458
Quoted 2 lines
arch/i386/kernel/kprobes.c +1 -1
-	preempt_disable();
+	rcu_read_lock();
If synchronize_sched() is used on the update side, this needs to remain preempt_disable() rather than rcu_read_lock().

Quoted 3 lines

Kprobe handlers can't block/sleep. So the idea is to depend on a schedule() event to ensure handlers have executed. This and the others you have pointed out can surely be preempt_disable().

Yep! Or maybe you can rely on the preempt_disable() that guards the single-step operation?

Quoted 448 lines

Quoted 447 lines

Quoted 90 lines

arch/i386/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))

@@ -482,7 +474,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/ia64/kernel/kprobes.c
===================================================================

arch/ia64/kernel/kprobes.c +4 -8

@@ -26,7 +26,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -343,10 +342,11 @@ int __kprobes trampoline_probe_handler(s
 	struct kretprobe_instance *ri = NULL;
 	struct hlist_head *head;
 	struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =
 		((struct fnptr *)kretprobe_trampoline)->ip;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -386,7 +386,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->cr_iip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -397,6 +397,7 @@ int __kprobes trampoline_probe_handler(s
         return 1;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -612,7 +613,6 @@ static int __kprobes pre_kprobes_handler
 			if ((kcb->kprobe_status == KPROBE_HIT_SS) &&
 	 		     (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)) {
   				ia64_psr(regs)->ss = 0;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the pre_kprobe_handler(), since

@@ -641,10 +641,8 @@ static int __kprobes pre_kprobes_handler
 		}
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (!is_ia64_break_inst(regs)) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -707,7 +705,6 @@ static int __kprobes post_kprobes_handle
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 
 out:
 	preempt_enable_no_resched();

@@ -728,7 +725,6 @@ static int __kprobes kprobes_fault_handl
 	if (kcb->kprobe_status & KPROBE_HIT_SS) {
 		resume_execution(cur, regs);
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}

arch/ia64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 740

Quoted 5 lines

arch/ia64/kernel/kprobes.c +1 -1

@@ -741,7 +737,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

Ditto here...

Quoted 110 lines

arch/ia64/kernel/kprobes.c +1 -1

 	switch(val) {
 	case DIE_BREAK:
 		if (pre_kprobes_handler(args))

@@ -757,7 +753,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/ppc64/kernel/kprobes.c
===================================================================

arch/ppc64/kernel/kprobes.c +4 ~~-12~~

@@ -30,7 +30,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/preempt.h>
 #include <asm/cacheflush.h>
 #include <asm/kdebug.h>

@@ -125,6 +124,7 @@ static inline void set_current_kprobe(st
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -152,8 +152,6 @@ static inline int kprobe_handler(struct 
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			kprobe_opcode_t insn = *p->ainsn.insn;

@@ -161,7 +159,6 @@ static inline int kprobe_handler(struct 
 					is_trap(insn)) {
 				regs->msr &= ~MSR_SE;
 				regs->msr |= kcb->kprobe_saved_msr;
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -183,14 +180,11 @@ static inline int kprobe_handler(struct 
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * PowerPC has multiple variants of the "trap"

@@ -254,9 +248,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -296,7 +291,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->nip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -348,7 +343,6 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();

@@ -363,7 +357,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -378,7 +371,6 @@ static inline int kprobe_fault_handler(s
 		regs->msr |= kcb->kprobe_saved_msr;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/ppc64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 388

Quoted 9 lines

arch/ppc64/kernel/kprobes.c +1 -5

@@ -393,11 +385,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	/*
-	 * Interrupts are not disabled here.  We need to disable
-	 * preemption, because kprobe_running() uses smp_processor_id().
-	 */
-	preempt_disable();
+	rcu_read_lock();

And here...

Quoted 71 lines

arch/ppc64/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_BPT:
 		if (kprobe_handler(args->regs))

@@ -416,7 +404,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable_no_resched();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/sparc64/kernel/kprobes.c
===================================================================

arch/sparc64/kernel/kprobes.c +0 ~~-10~~

@@ -116,15 +116,11 @@ static int __kprobes kprobe_handler(stru
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		 * Disarm the probe we just hit, and ignore it.
-		 */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS) {
 				regs->tstate = ((regs->tstate & ~TSTATE_PIL) |
 					kcb->kprobe_orig_tstate_pil);
-				unlock_kprobes();
 				goto no_kprobe;
 			}
 			/* We have reentered the kprobe_handler(), since

@@ -144,14 +140,11 @@ static int __kprobes kprobe_handler(stru
 			if (p->break_handler && p->break_handler(p, regs))
 				goto ss_probe;
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*(u32 *)addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -296,14 +289,12 @@ static inline int post_kprobe_handler(st
 		goto out;
 	}
 	reset_current_kprobe();
-	unlock_kprobes();
 out:
 	preempt_enable_no_resched();
 
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -316,7 +307,6 @@ static inline int kprobe_fault_handler(s
 		resume_execution(cur, regs, kcb);
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/sparc64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 324

Quoted 5 lines

arch/sparc64/kernel/kprobes.c +1 -1

@@ -331,7 +321,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

As well as here...

Quoted 121 lines

arch/sparc64/kernel/kprobes.c +1 -1

 	switch (val) {
 	case DIE_DEBUG:
 		if (kprobe_handler(args->regs))

@@ -350,7 +340,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }

Index: linux-2.6.14-rc3/arch/x86_64/kernel/kprobes.c
===================================================================

arch/x86_64/kernel/kprobes.c +4 ~~-17~~

@@ -34,7 +34,6 @@
 #include <linux/config.h>
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
-#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/preempt.h>

@@ -266,6 +265,7 @@ static void __kprobes prepare_singlestep
 		regs->rip = (unsigned long)p->ainsn.insn;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes arch_prepare_kretprobe(struct kretprobe *rp,
 				      struct pt_regs *regs)
 {

@@ -299,15 +299,12 @@ int __kprobes kprobe_handler(struct pt_r
 
 	/* Check we're not actually recursing */
 	if (kprobe_running()) {
-		/* We *are* holding lock here, so this is safe.
-		   Disarm the probe we just hit, and ignore it. */
 		p = get_kprobe(addr);
 		if (p) {
 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
 				*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
 				regs->eflags &= ~TF_MASK;
 				regs->eflags |= kcb->kprobe_saved_rflags;
-				unlock_kprobes();
 				goto no_kprobe;
 			} else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 				/* TODO: Provide re-entrancy from

@@ -340,14 +337,11 @@ int __kprobes kprobe_handler(struct pt_r
 				goto ss_probe;
 			}
 		}
-		/* If it's not ours, can't be delete race, (we hold lock). */
 		goto no_kprobe;
 	}
 
-	lock_kprobes();
 	p = get_kprobe(addr);
 	if (!p) {
-		unlock_kprobes();
 		if (*addr != BREAKPOINT_INSTRUCTION) {
 			/*
 			 * The breakpoint instruction was removed right

@@ -406,9 +400,10 @@ int __kprobes trampoline_probe_handler(s
         struct kretprobe_instance *ri = NULL;
         struct hlist_head *head;
         struct hlist_node *node, *tmp;
-	unsigned long orig_ret_address = 0;
+	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address =(unsigned long)&kretprobe_trampoline;
 
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
 
 	/*

@@ -448,7 +443,7 @@ int __kprobes trampoline_probe_handler(s
 	regs->rip = orig_ret_address;
 
 	reset_current_kprobe();
-	unlock_kprobes();
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	preempt_enable_no_resched();
 
         /*

@@ -536,10 +531,6 @@ static void __kprobes resume_execution(s
 	}
 }
 
-/*
- * Interrupts are disabled on entry as trap1 is an interrupt gate and they
- * remain disabled thoroughout this function.  And we hold kprobe lock.
- */
 int __kprobes post_kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *cur = kprobe_running();

@@ -560,8 +551,6 @@ int __kprobes post_kprobe_handler(struct
 	if (kcb->kprobe_status == KPROBE_REENTER) {
 		restore_previous_kprobe(kcb);
 		goto out;
-	} else {
-		unlock_kprobes();
 	}
 	reset_current_kprobe();
 out:

@@ -578,7 +567,6 @@ out:
 	return 1;
 }
 
-/* Interrupts disabled, kprobe_lock held. */
 int __kprobes kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	struct kprobe *cur = kprobe_running();

@@ -592,7 +580,6 @@ int __kprobes kprobe_fault_handler(struc
 		regs->eflags |= kcb->kprobe_old_rflags;
 
 		reset_current_kprobe();
-		unlock_kprobes();
 		preempt_enable_no_resched();
 	}
 	return 0;

arch/x86_64/kernel/kprobes.c int __kprobes kprobe_exceptions_notify(s · line 597

Quoted 5 lines

arch/x86_64/kernel/kprobes.c +1 -1

@@ -607,7 +594,7 @@ int __kprobes kprobe_exceptions_notify(s
 	struct die_args *args = (struct die_args *)data;
 	int ret = NOTIFY_DONE;
 
-	preempt_disable();
+	rcu_read_lock();

As well as here yet again...

Quoted 19 lines

arch/x86_64/kernel/kprobes.c +1 -2
 	switch (val) {
 	case DIE_INT3:
 		if (kprobe_handler(args->regs))
@@ -626,7 +613,7 @@ int __kprobes kprobe_exceptions_notify(s
 	default:
 		break;
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return ret;
 }
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Paul E. McKenney

2005-10-18 14:01 UTC #13 lore raw

Re: [PATCH 8/9] Kprobes: Use RCU for (un)register synchronization - base changes

In reply to Ananth N Mavinakayanahalli

Quoted 8 lines · Ananth N Mavinakayanahalli

From: Ananth N Mavinakayanahalli <[email protected]>
Changes to the base kprobes infrastructure to use RCU for synchronization during kprobe registration and unregistration. These changes coupled with the arch kprobe changes (next in series):
a. serialize registration and unregistration of kprobes.
b. enable lockless execution of handlers. Handlers can now run in parallel.

A couple of questions interspersed... Probably my limited knowledge of kprobes, but I have to ask... Search for empty lines to find them.

Thanx, Paul

Quoted 34 lines

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 include/linux/kprobes.h |    9 +---
 kernel/kprobes.c        |  101 +++++++++++++++++++-----------------------------
 2 files changed, 45 insertions(+), 65 deletions(-)

Index: linux-2.6.14-rc3/include/linux/kprobes.h
===================================================================

include/linux/kprobes.h +3 -4

@@ -34,6 +34,8 @@
 #include <linux/notifier.h>
 #include <linux/smp.h>
 #include <linux/percpu.h>
+#include <linux/spinlock.h>
+#include <linux/rcupdate.h>
 
 #include <asm/kprobes.h>

@@ -146,10 +148,7 @@ struct kretprobe_instance {
 };
 
 #ifdef CONFIG_KPROBES
-/* Locks kprobe: irq must be disabled */
-void lock_kprobes(void);
-void unlock_kprobes(void);
-
+extern spinlock_t kretprobe_lock;
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_copy_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);

include/linux/kprobes.h extern void show_registers(struct pt_reg · line 162

Quoted 5 lines

include/linux/kprobes.h +1 -1

@@ -160,7 +159,7 @@ extern void show_registers(struct pt_reg
 extern kprobe_opcode_t *get_insn_slot(void);
 extern void free_insn_slot(kprobe_opcode_t *slot);
 
-/* Get the kprobe at this addr (if any).  Must have called lock_kprobes */
+/* Get the kprobe at this addr (if any) - called under a rcu_read_lock() */

Since the update side uses synchronize_kernel() (in patch 9/9, right?), shouldn't the above "rcu_read_lock()" instead by preempt_disable()?

Also, some of the code in the earlier patches seems to do preempt_disable. For example, kprobe_handler() in arch/i386/kernel/kprobes.c.

In realtime kernels, synchronize_sched() does -not- guarantee that all rcu_read_lock() critical sections have finished, only that any preempt_disable() code sequences (explicit or implicit) have completed.

Quoted 69 lines

include/linux/kprobes.h +0 -0

 struct kprobe *get_kprobe(void *addr);
 struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);

Index: linux-2.6.14-rc3/kernel/kprobes.c
===================================================================

kernel/kprobes.c +2 ~~-38~~

@@ -32,7 +32,6 @@
  *		<[email protected]> added function-return probes.
  */
 #include <linux/kprobes.h>
-#include <linux/spinlock.h>
 #include <linux/hash.h>
 #include <linux/init.h>
 #include <linux/module.h>

@@ -48,8 +47,8 @@
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
-unsigned int kprobe_cpu = NR_CPUS;
-static DEFINE_SPINLOCK(kprobe_lock);
+static DEFINE_SPINLOCK(kprobe_lock);	/* Protects kprobe_table */
+DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 
 /*

@@ -152,41 +151,6 @@ void __kprobes free_insn_slot(kprobe_opc
 	}
 }
 
-/* Locks kprobe: irqs must be disabled */
-void __kprobes lock_kprobes(void)
-{
-	unsigned long flags = 0;
-
-	/* Avoiding local interrupts to happen right after we take the kprobe_lock
-	 * and before we get a chance to update kprobe_cpu, this to prevent
-	 * deadlock when we have a kprobe on ISR routine and a kprobe on task
-	 * routine
-	 */
-	local_irq_save(flags);
-
-	spin_lock(&kprobe_lock);
-	kprobe_cpu = smp_processor_id();
-
- 	local_irq_restore(flags);
-}
-
-void __kprobes unlock_kprobes(void)
-{
-	unsigned long flags = 0;
-
-	/* Avoiding local interrupts to happen right after we update
-	 * kprobe_cpu and before we get a a chance to release kprobe_lock,
-	 * this to prevent deadlock when we have a kprobe on ISR routine and
-	 * a kprobe on task routine
-	 */
-	local_irq_save(flags);
-
-	kprobe_cpu = NR_CPUS;
-	spin_unlock(&kprobe_lock);
-
- 	local_irq_restore(flags);
-}
-
 /* We have preemption disabled.. so it is safe to use __ versions */
 static inline void set_kprobe_instance(struct kprobe *kp)
 {

kernel/kprobes.c static inline void reset_kprobe_instance · line 169

Quoted 9 lines

kernel/kprobes.c +5 -1

@@ -198,14 +162,19 @@ static inline void reset_kprobe_instance
 	__get_cpu_var(kprobe_instance) = NULL;
 }
 
-/* You have to be holding the kprobe_lock */
+/*
+ * This routine is called either:
+ * 	- under the kprobe_lock spinlock - during kprobe_[un]register()
+ * 				OR
+ * 	- under an rcu_read_lock() - from arch/xxx/kernel/kprobes.c

Same here -- shouldn't the rcu_read_lock() be preempt_disable()?

Quoted 215 lines

kernel/kprobes.c +34 ~~-22~~

+ */
 struct kprobe __kprobes *get_kprobe(void *addr)
 {
 	struct hlist_head *head;
 	struct hlist_node *node;
 
 	head = &kprobe_table[hash_ptr(addr, KPROBE_HASH_BITS)];
-	hlist_for_each(node, head) {
+	hlist_for_each_rcu(node, head) {
 		struct kprobe *p = hlist_entry(node, struct kprobe, hlist);
 		if (p->addr == addr)
 			return p;

@@ -221,7 +190,7 @@ static int __kprobes aggr_pre_handler(st
 {
 	struct kprobe *kp;
 
-	list_for_each_entry(kp, &p->list, list) {
+	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->pre_handler) {
 			set_kprobe_instance(kp);
 			if (kp->pre_handler(kp, regs))

@@ -237,7 +206,7 @@ static void __kprobes aggr_post_handler(
 {
 	struct kprobe *kp;
 
-	list_for_each_entry(kp, &p->list, list) {
+	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->post_handler) {
 			set_kprobe_instance(kp);
 			kp->post_handler(kp, regs, flags);

@@ -276,6 +245,7 @@ static int __kprobes aggr_break_handler(
 	return ret;
 }
 
+/* Called with kretprobe_lock held */
 struct kretprobe_instance __kprobes *get_free_rp_inst(struct kretprobe *rp)
 {
 	struct hlist_node *node;

@@ -285,6 +255,7 @@ struct kretprobe_instance __kprobes *get
 	return NULL;
 }
 
+/* Called with kretprobe_lock held */
 static struct kretprobe_instance __kprobes *get_used_rp_inst(struct kretprobe
 							      *rp)
 {

@@ -295,6 +266,7 @@ static struct kretprobe_instance __kprob
 	return NULL;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes add_rp_inst(struct kretprobe_instance *ri)
 {
 	/*

@@ -313,6 +285,7 @@ void __kprobes add_rp_inst(struct kretpr
 	hlist_add_head(&ri->uflist, &ri->rp->used_instances);
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes recycle_rp_inst(struct kretprobe_instance *ri)
 {
 	/* remove rp inst off the rprobe_inst_table */

@@ -346,13 +319,13 @@ void __kprobes kprobe_flush_task(struct 
 	struct hlist_node *node, *tmp;
 	unsigned long flags = 0;
 
-	spin_lock_irqsave(&kprobe_lock, flags);
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
         hlist_for_each_entry_safe(ri, node, tmp, head, hlist) {
                 if (ri->task == tk)
                         recycle_rp_inst(ri);
         }
-	spin_unlock_irqrestore(&kprobe_lock, flags);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 }
 
 /*

@@ -363,9 +336,12 @@ static int __kprobes pre_handler_kretpro
 					   struct pt_regs *regs)
 {
 	struct kretprobe *rp = container_of(p, struct kretprobe, kp);
+	unsigned long flags = 0;
 
 	/*TODO: consider to only swap the RA after the last pre_handler fired */
+	spin_lock_irqsave(&kretprobe_lock, flags);
 	arch_prepare_kretprobe(rp, regs);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	return 0;
 }

@@ -396,13 +372,13 @@ static int __kprobes add_new_kprobe(stru
         struct kprobe *kp;
 
 	if (p->break_handler) {
-		list_for_each_entry(kp, &old_p->list, list) {
+		list_for_each_entry_rcu(kp, &old_p->list, list) {
 			if (kp->break_handler)
 				return -EEXIST;
 		}
-		list_add_tail(&p->list, &old_p->list);
+		list_add_tail_rcu(&p->list, &old_p->list);
 	} else
-		list_add(&p->list, &old_p->list);
+		list_add_rcu(&p->list, &old_p->list);
 	return 0;
 }

@@ -420,18 +396,18 @@ static inline void add_aggr_kprobe(struc
 	ap->break_handler = aggr_break_handler;
 
 	INIT_LIST_HEAD(&ap->list);
-	list_add(&p->list, &ap->list);
+	list_add_rcu(&p->list, &ap->list);
 
 	INIT_HLIST_NODE(&ap->hlist);
-	hlist_del(&p->hlist);
-	hlist_add_head(&ap->hlist,
+	hlist_del_rcu(&p->hlist);
+	hlist_add_head_rcu(&ap->hlist,
 		&kprobe_table[hash_ptr(ap->addr, KPROBE_HASH_BITS)]);
 }
 
 /*
  * This is the second or subsequent kprobe at the address - handle
  * the intricacies
- * TODO: Move kcalloc outside the spinlock
+ * TODO: Move kcalloc outside the spin_lock
  */
 static int __kprobes register_aggr_kprobe(struct kprobe *old_p,
 					  struct kprobe *p)

@@ -457,7 +433,7 @@ static int __kprobes register_aggr_kprob
 static inline void cleanup_kprobe(struct kprobe *p, unsigned long flags)
 {
 	arch_disarm_kprobe(p);
-	hlist_del(&p->hlist);
+	hlist_del_rcu(&p->hlist);
 	spin_unlock_irqrestore(&kprobe_lock, flags);
 	arch_remove_kprobe(p);
 }

@@ -465,11 +441,10 @@ static inline void cleanup_kprobe(struct
 static inline void cleanup_aggr_kprobe(struct kprobe *old_p,
 		struct kprobe *p, unsigned long flags)
 {
-	list_del(&p->list);
-	if (list_empty(&old_p->list)) {
+	list_del_rcu(&p->list);
+	if (list_empty(&old_p->list))
 		cleanup_kprobe(old_p, flags);
-		kfree(old_p);
-	} else
+	else
 		spin_unlock_irqrestore(&kprobe_lock, flags);
 }

@@ -492,9 +467,9 @@ int __kprobes register_kprobe(struct kpr
 	if ((ret = arch_prepare_kprobe(p)) != 0)
 		goto rm_kprobe;
 
+	p->nmissed = 0;
 	spin_lock_irqsave(&kprobe_lock, flags);
 	old_p = get_kprobe(p->addr);
-	p->nmissed = 0;
 	if (old_p) {
 		ret = register_aggr_kprobe(old_p, p);
 		goto out;

@@ -502,7 +477,7 @@ int __kprobes register_kprobe(struct kpr
 
 	arch_copy_kprobe(p);
 	INIT_HLIST_NODE(&p->hlist);
-	hlist_add_head(&p->hlist,
+	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
   	arch_arm_kprobe(p);

@@ -523,10 +498,16 @@ void __kprobes unregister_kprobe(struct 
 	spin_lock_irqsave(&kprobe_lock, flags);
 	old_p = get_kprobe(p->addr);
 	if (old_p) {
+		/* cleanup_*_kprobe() does the spin_unlock_irqrestore */
 		if (old_p->pre_handler == aggr_pre_handler)
 			cleanup_aggr_kprobe(old_p, p, flags);
 		else
 			cleanup_kprobe(p, flags);
+
+		synchronize_sched();
+		if (old_p->pre_handler == aggr_pre_handler &&
+				list_empty(&old_p->list))
+			kfree(old_p);
 	} else
 		spin_unlock_irqrestore(&kprobe_lock, flags);
 }

@@ -603,13 +584,13 @@ void __kprobes unregister_kretprobe(stru
 
 	unregister_kprobe(&rp->kp);
 	/* No race here */
-	spin_lock_irqsave(&kprobe_lock, flags);
+	spin_lock_irqsave(&kretprobe_lock, flags);
 	free_rp_inst(rp);
 	while ((ri = get_used_rp_inst(rp)) != NULL) {
 		ri->rp = NULL;
 		hlist_del(&ri->uflist);
 	}
-	spin_unlock_irqrestore(&kprobe_lock, flags);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 }
 
 static int __init init_kprobes(void)
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Ananth N Mavinakayanahalli

author

2005-10-18 14:44 UTC #14 lore raw

Re: [PATCH 8/9] Kprobes: Use RCU for (un)register synchronization - base changes

Quoted 7 lines · Paul E. McKenney

Quoted 6 lines · Ananth N Mavinakayanahalli
From: Ananth N Mavinakayanahalli <[email protected]>
Changes to the base kprobes infrastructure to use RCU for synchronization during kprobe registration and unregistration. These changes coupled with the arch kprobe changes (next in series):

Quoted 5 lines

Quoted 2 lines
a. serialize registration and unregistration of kprobes.
b. enable lockless execution of handlers. Handlers can now run in parallel.
A couple of questions interspersed... Probably my limited knowledge of kprobes, but I have to ask... Search for empty lines to find them.

Paul,

Thanks for the review... replies inlined...

Ananth

Quoted 40 lines

Thanx, Paul

Quoted 37 lines

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Anil S Keshavamurthy <[email protected]>

---

 include/linux/kprobes.h |    9 +---
 kernel/kprobes.c        |  101 +++++++++++++++++++-----------------------------
 2 files changed, 45 insertions(+), 65 deletions(-)

Index: linux-2.6.14-rc3/include/linux/kprobes.h
===================================================================

include/linux/kprobes.h +3 -4

@@ -34,6 +34,8 @@
 #include <linux/notifier.h>
 #include <linux/smp.h>
 #include <linux/percpu.h>
+#include <linux/spinlock.h>
+#include <linux/rcupdate.h>
 
 #include <asm/kprobes.h>

@@ -146,10 +148,7 @@ struct kretprobe_instance {
 };
 
 #ifdef CONFIG_KPROBES
-/* Locks kprobe: irq must be disabled */
-void lock_kprobes(void);
-void unlock_kprobes(void);
-
+extern spinlock_t kretprobe_lock;
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_copy_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);

@@ -160,7 +159,7 @@ extern void show_registers(struct pt_reg
 extern kprobe_opcode_t *get_insn_slot(void);
 extern void free_insn_slot(kprobe_opcode_t *slot);

Quoted 4 lines

include/linux/kprobes.h extern void show_registers(struct pt_reg · line 162
Quoted 2 lines
include/linux/kprobes.h +1 -1
-/* Get the kprobe at this addr (if any).  Must have called lock_kprobes */
+/* Get the kprobe at this addr (if any) - called under a rcu_read_lock() */
Since the update side uses synchronize_kernel() (in patch 9/9, right?),

Yes, synchronize_sched() to be precise.

Quoted 1 line

shouldn't the above "rcu_read_lock()" instead by preempt_disable()?

My reasoning was that since rcu_read_(un)lock is #defined to preempt_(en)disable. But, I agree, since we use synchronize_sched() this can just be preempt_disable()...
Suggestions?

Quoted 2 lines

Also, some of the code in the earlier patches seems to do preempt_disable. For example, kprobe_handler() in arch/i386/kernel/kprobes.c.

Well, the convolution is due to the way kprobes work:
- Breakpoint hit; execute pre_handler
- single-step on a copy of the original instruction; execute the post_handler

We don't want to be preempted from the time of the breakpoint exception until after the post_handler is run. I've taken care to ensure the preempt_ calls from the various codepaths are balanced.

Quoted 3 lines

In realtime kernels, synchronize_sched() does -not- guarantee that all rcu_read_lock() critical sections have finished, only that any preempt_disable() code sequences (explicit or implicit) have completed.

Are we assured that the preempt_ depth is also taken care of? If that is the case, I think we are safe, since the matching preempt_enable() calls are _after_ the post_handler is executed.

Quoted 72 lines

include/linux/kprobes.h +0 -0

 struct kprobe *get_kprobe(void *addr);
 struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);

Index: linux-2.6.14-rc3/kernel/kprobes.c
===================================================================

kernel/kprobes.c +2 ~~-38~~

@@ -32,7 +32,6 @@
  *		<[email protected]> added function-return probes.
  */
 #include <linux/kprobes.h>
-#include <linux/spinlock.h>
 #include <linux/hash.h>
 #include <linux/init.h>
 #include <linux/module.h>

@@ -48,8 +47,8 @@
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
-unsigned int kprobe_cpu = NR_CPUS;
-static DEFINE_SPINLOCK(kprobe_lock);
+static DEFINE_SPINLOCK(kprobe_lock);	/* Protects kprobe_table */
+DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 
 /*

@@ -152,41 +151,6 @@ void __kprobes free_insn_slot(kprobe_opc
 	}
 }
 
-/* Locks kprobe: irqs must be disabled */
-void __kprobes lock_kprobes(void)
-{
-	unsigned long flags = 0;
-
-	/* Avoiding local interrupts to happen right after we take the kprobe_lock
-	 * and before we get a chance to update kprobe_cpu, this to prevent
-	 * deadlock when we have a kprobe on ISR routine and a kprobe on task
-	 * routine
-	 */
-	local_irq_save(flags);
-
-	spin_lock(&kprobe_lock);
-	kprobe_cpu = smp_processor_id();
-
- 	local_irq_restore(flags);
-}
-
-void __kprobes unlock_kprobes(void)
-{
-	unsigned long flags = 0;
-
-	/* Avoiding local interrupts to happen right after we update
-	 * kprobe_cpu and before we get a a chance to release kprobe_lock,
-	 * this to prevent deadlock when we have a kprobe on ISR routine and
-	 * a kprobe on task routine
-	 */
-	local_irq_save(flags);
-
-	kprobe_cpu = NR_CPUS;
-	spin_unlock(&kprobe_lock);
-
- 	local_irq_restore(flags);
-}
-
 /* We have preemption disabled.. so it is safe to use __ versions */
 static inline void set_kprobe_instance(struct kprobe *kp)
 {

@@ -198,14 +162,19 @@ static inline void reset_kprobe_instance
 	__get_cpu_var(kprobe_instance) = NULL;
 }

Quoted 8 lines

kernel/kprobes.c static inline void reset_kprobe_instance · line 169
Quoted 6 lines
kernel/kprobes.c +5 -1
-/* You have to be holding the kprobe_lock */
+/*
+ * This routine is called either:
+ * 	- under the kprobe_lock spinlock - during kprobe_[un]register()
+ * 				OR
+ * 	- under an rcu_read_lock() - from arch/xxx/kernel/kprobes.c
Same here -- shouldn't the rcu_read_lock() be preempt_disable()?

This is the same routine referred to in the comment above.

Quoted 215 lines

kernel/kprobes.c +34 ~~-22~~

+ */
 struct kprobe __kprobes *get_kprobe(void *addr)
 {
 	struct hlist_head *head;
 	struct hlist_node *node;
 
 	head = &kprobe_table[hash_ptr(addr, KPROBE_HASH_BITS)];
-	hlist_for_each(node, head) {
+	hlist_for_each_rcu(node, head) {
 		struct kprobe *p = hlist_entry(node, struct kprobe, hlist);
 		if (p->addr == addr)
 			return p;

@@ -221,7 +190,7 @@ static int __kprobes aggr_pre_handler(st
 {
 	struct kprobe *kp;
 
-	list_for_each_entry(kp, &p->list, list) {
+	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->pre_handler) {
 			set_kprobe_instance(kp);
 			if (kp->pre_handler(kp, regs))

@@ -237,7 +206,7 @@ static void __kprobes aggr_post_handler(
 {
 	struct kprobe *kp;
 
-	list_for_each_entry(kp, &p->list, list) {
+	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->post_handler) {
 			set_kprobe_instance(kp);
 			kp->post_handler(kp, regs, flags);

@@ -276,6 +245,7 @@ static int __kprobes aggr_break_handler(
 	return ret;
 }
 
+/* Called with kretprobe_lock held */
 struct kretprobe_instance __kprobes *get_free_rp_inst(struct kretprobe *rp)
 {
 	struct hlist_node *node;

@@ -285,6 +255,7 @@ struct kretprobe_instance __kprobes *get
 	return NULL;
 }
 
+/* Called with kretprobe_lock held */
 static struct kretprobe_instance __kprobes *get_used_rp_inst(struct kretprobe
 							      *rp)
 {

@@ -295,6 +266,7 @@ static struct kretprobe_instance __kprob
 	return NULL;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes add_rp_inst(struct kretprobe_instance *ri)
 {
 	/*

@@ -313,6 +285,7 @@ void __kprobes add_rp_inst(struct kretpr
 	hlist_add_head(&ri->uflist, &ri->rp->used_instances);
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes recycle_rp_inst(struct kretprobe_instance *ri)
 {
 	/* remove rp inst off the rprobe_inst_table */

@@ -346,13 +319,13 @@ void __kprobes kprobe_flush_task(struct 
 	struct hlist_node *node, *tmp;
 	unsigned long flags = 0;
 
-	spin_lock_irqsave(&kprobe_lock, flags);
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
         hlist_for_each_entry_safe(ri, node, tmp, head, hlist) {
                 if (ri->task == tk)
                         recycle_rp_inst(ri);
         }
-	spin_unlock_irqrestore(&kprobe_lock, flags);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 }
 
 /*

@@ -363,9 +336,12 @@ static int __kprobes pre_handler_kretpro
 					   struct pt_regs *regs)
 {
 	struct kretprobe *rp = container_of(p, struct kretprobe, kp);
+	unsigned long flags = 0;
 
 	/*TODO: consider to only swap the RA after the last pre_handler fired */
+	spin_lock_irqsave(&kretprobe_lock, flags);
 	arch_prepare_kretprobe(rp, regs);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	return 0;
 }

@@ -396,13 +372,13 @@ static int __kprobes add_new_kprobe(stru
         struct kprobe *kp;
 
 	if (p->break_handler) {
-		list_for_each_entry(kp, &old_p->list, list) {
+		list_for_each_entry_rcu(kp, &old_p->list, list) {
 			if (kp->break_handler)
 				return -EEXIST;
 		}
-		list_add_tail(&p->list, &old_p->list);
+		list_add_tail_rcu(&p->list, &old_p->list);
 	} else
-		list_add(&p->list, &old_p->list);
+		list_add_rcu(&p->list, &old_p->list);
 	return 0;
 }

@@ -420,18 +396,18 @@ static inline void add_aggr_kprobe(struc
 	ap->break_handler = aggr_break_handler;
 
 	INIT_LIST_HEAD(&ap->list);
-	list_add(&p->list, &ap->list);
+	list_add_rcu(&p->list, &ap->list);
 
 	INIT_HLIST_NODE(&ap->hlist);
-	hlist_del(&p->hlist);
-	hlist_add_head(&ap->hlist,
+	hlist_del_rcu(&p->hlist);
+	hlist_add_head_rcu(&ap->hlist,
 		&kprobe_table[hash_ptr(ap->addr, KPROBE_HASH_BITS)]);
 }
 
 /*
  * This is the second or subsequent kprobe at the address - handle
  * the intricacies
- * TODO: Move kcalloc outside the spinlock
+ * TODO: Move kcalloc outside the spin_lock
  */
 static int __kprobes register_aggr_kprobe(struct kprobe *old_p,
 					  struct kprobe *p)

@@ -457,7 +433,7 @@ static int __kprobes register_aggr_kprob
 static inline void cleanup_kprobe(struct kprobe *p, unsigned long flags)
 {
 	arch_disarm_kprobe(p);
-	hlist_del(&p->hlist);
+	hlist_del_rcu(&p->hlist);
 	spin_unlock_irqrestore(&kprobe_lock, flags);
 	arch_remove_kprobe(p);
 }

@@ -465,11 +441,10 @@ static inline void cleanup_kprobe(struct
 static inline void cleanup_aggr_kprobe(struct kprobe *old_p,
 		struct kprobe *p, unsigned long flags)
 {
-	list_del(&p->list);
-	if (list_empty(&old_p->list)) {
+	list_del_rcu(&p->list);
+	if (list_empty(&old_p->list))
 		cleanup_kprobe(old_p, flags);
-		kfree(old_p);
-	} else
+	else
 		spin_unlock_irqrestore(&kprobe_lock, flags);
 }

@@ -492,9 +467,9 @@ int __kprobes register_kprobe(struct kpr
 	if ((ret = arch_prepare_kprobe(p)) != 0)
 		goto rm_kprobe;
 
+	p->nmissed = 0;
 	spin_lock_irqsave(&kprobe_lock, flags);
 	old_p = get_kprobe(p->addr);
-	p->nmissed = 0;
 	if (old_p) {
 		ret = register_aggr_kprobe(old_p, p);
 		goto out;

@@ -502,7 +477,7 @@ int __kprobes register_kprobe(struct kpr
 
 	arch_copy_kprobe(p);
 	INIT_HLIST_NODE(&p->hlist);
-	hlist_add_head(&p->hlist,
+	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
   	arch_arm_kprobe(p);

@@ -523,10 +498,16 @@ void __kprobes unregister_kprobe(struct 
 	spin_lock_irqsave(&kprobe_lock, flags);
 	old_p = get_kprobe(p->addr);
 	if (old_p) {
+		/* cleanup_*_kprobe() does the spin_unlock_irqrestore */
 		if (old_p->pre_handler == aggr_pre_handler)
 			cleanup_aggr_kprobe(old_p, p, flags);
 		else
 			cleanup_kprobe(p, flags);
+
+		synchronize_sched();
+		if (old_p->pre_handler == aggr_pre_handler &&
+				list_empty(&old_p->list))
+			kfree(old_p);
 	} else
 		spin_unlock_irqrestore(&kprobe_lock, flags);
 }

@@ -603,13 +584,13 @@ void __kprobes unregister_kretprobe(stru
 
 	unregister_kprobe(&rp->kp);
 	/* No race here */
-	spin_lock_irqsave(&kprobe_lock, flags);
+	spin_lock_irqsave(&kretprobe_lock, flags);
 	free_rp_inst(rp);
 	while ((ri = get_used_rp_inst(rp)) != NULL) {
 		ri->rp = NULL;
 		hlist_del(&ri->uflist);
 	}
-	spin_unlock_irqrestore(&kprobe_lock, flags);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 }
 
 static int __init init_kprobes(void)
-

Paul E. McKenney

2005-10-18 16:30 UTC #15 lore raw

Re: [PATCH 8/9] Kprobes: Use RCU for (un)register synchronization - base changes

Quoted 60 lines · Ananth N Mavinakayanahalli

Quoted 7 lines · Paul E. McKenney
Quoted 6 lines · Ananth N Mavinakayanahalli
From: Ananth N Mavinakayanahalli <[email protected]>
Changes to the base kprobes infrastructure to use RCU for synchronization during kprobe registration and unregistration. These changes coupled with the arch kprobe changes (next in series):
Quoted 5 lines
Quoted 2 lines
a. serialize registration and unregistration of kprobes.
b. enable lockless execution of handlers. Handlers can now run in parallel.
A couple of questions interspersed... Probably my limited knowledge of kprobes, but I have to ask... Search for empty lines to find them.
Paul,
Thanks for the review... replies inlined...
Ananth
Quoted 40 lines
Thanx, Paul
Quoted 37 lines
Signed-off-by Ananth N Mavinakayanahalli <[email protected]>
Signed-off-by Anil S Keshavamurthy <[email protected]>
---
 include/linux/kprobes.h |    9 +---
 kernel/kprobes.c        |  101 +++++++++++++++++++-----------------------------
 2 files changed, 45 insertions(+), 65 deletions(-)
Index: linux-2.6.14-rc3/include/linux/kprobes.h
===================================================================
include/linux/kprobes.h +3 -4
@@ -34,6 +34,8 @@
 #include <linux/notifier.h>
 #include <linux/smp.h>
 #include <linux/percpu.h>
+#include <linux/spinlock.h>
+#include <linux/rcupdate.h>
 
 #include <asm/kprobes.h>
 
@@ -146,10 +148,7 @@ struct kretprobe_instance {
 };
 
 #ifdef CONFIG_KPROBES
-/* Locks kprobe: irq must be disabled */
-void lock_kprobes(void);
-void unlock_kprobes(void);
-
+extern spinlock_t kretprobe_lock;
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_copy_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);
@@ -160,7 +159,7 @@ extern void show_registers(struct pt_reg
 extern kprobe_opcode_t *get_insn_slot(void);
 extern void free_insn_slot(kprobe_opcode_t *slot);
 

Quoted 6 lines

Quoted 4 lines
include/linux/kprobes.h extern void show_registers(struct pt_reg · line 162
Quoted 2 lines
include/linux/kprobes.h +1 -1
-/* Get the kprobe at this addr (if any).  Must have called lock_kprobes */
+/* Get the kprobe at this addr (if any) - called under a rcu_read_lock() */
Since the update side uses synchronize_kernel() (in patch 9/9, right?),
Yes, synchronize_sched() to be precise.

Good, am not going blind yet. ;-)

Quoted 6 lines

Quoted 1 line
shouldn't the above "rcu_read_lock()" instead by preempt_disable()?
My reasoning was that since rcu_read_(un)lock is #defined to preempt_(en)disable. But, I agree, since we use synchronize_sched() this can just be preempt_disable()...
Suggestions?

In some realtime-friendly RCU implementations, the RCU read-side critical sections do not disable preemption. So, if you are using synchronize_sched() on the update side, you need to use preempt_disable() (or some other primitive that disables preemption, for example, any of the primitives that disables hardware interrupts) on the read side.

Quoted 1 line

From Documentation/RCU/whatisRCU.txt, the correspondence between the

update-side primitive and the read-side primitives needs to be as follows:

Defer Protect

a. synchronize_rcu() rcu_read_lock() / rcu_read_unlock()

	call_rcu()

b. call_rcu_bh() rcu_read_lock_bh() / rcu_read_unlock_bh()

c. synchronize_sched() preempt_disable() / preempt_enable()
local_irq_save() / local_irq_restore() hardirq enter / hardirq exit NMI enter / NMI exit

Quoted 11 lines

Quoted 2 lines
Also, some of the code in the earlier patches seems to do preempt_disable. For example, kprobe_handler() in arch/i386/kernel/kprobes.c.
Well, the convolution is due to the way kprobes work:
- Breakpoint hit; execute pre_handler
- single-step on a copy of the original instruction; execute the post_handler
We don't want to be preempted from the time of the breakpoint exception until after the post_handler is run. I've taken care to ensure the preempt_ calls from the various codepaths are balanced.

OK, didn't follow this -- thank you for the explanation! This sequence of events happens on each execution of the kprobe handler, or is there some optimization for subsequent invocations of the same kprobe handler?

If the former, then perhaps there is some way to make the preempt_disable() that protects the single stepping also protect the probe lookup.

Quoted 7 lines

Quoted 3 lines
In realtime kernels, synchronize_sched() does -not- guarantee that all rcu_read_lock() critical sections have finished, only that any preempt_disable() code sequences (explicit or implicit) have completed.
Are we assured that the preempt_ depth is also taken care of? If that is the case, I think we are safe, since the matching preempt_enable() calls are _after_ the post_handler is executed.

Seems so to me -- though if I understand you correctly, you should then be able to simply remove the rcu_read_lock() and rcu_read_unlock() calls, since that code sequence would be covered by the preempt_disable().

Or am I missing something?

Quoted 81 lines

Quoted 72 lines

include/linux/kprobes.h +0 -0

 struct kprobe *get_kprobe(void *addr);
 struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);

Index: linux-2.6.14-rc3/kernel/kprobes.c
===================================================================

kernel/kprobes.c +2 ~~-38~~

@@ -32,7 +32,6 @@
  *		<[email protected]> added function-return probes.
  */
 #include <linux/kprobes.h>
-#include <linux/spinlock.h>
 #include <linux/hash.h>
 #include <linux/init.h>
 #include <linux/module.h>

@@ -48,8 +47,8 @@
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
 static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
-unsigned int kprobe_cpu = NR_CPUS;
-static DEFINE_SPINLOCK(kprobe_lock);
+static DEFINE_SPINLOCK(kprobe_lock);	/* Protects kprobe_table */
+DEFINE_SPINLOCK(kretprobe_lock);	/* Protects kretprobe_inst_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 
 /*

@@ -152,41 +151,6 @@ void __kprobes free_insn_slot(kprobe_opc
 	}
 }
 
-/* Locks kprobe: irqs must be disabled */
-void __kprobes lock_kprobes(void)
-{
-	unsigned long flags = 0;
-
-	/* Avoiding local interrupts to happen right after we take the kprobe_lock
-	 * and before we get a chance to update kprobe_cpu, this to prevent
-	 * deadlock when we have a kprobe on ISR routine and a kprobe on task
-	 * routine
-	 */
-	local_irq_save(flags);
-
-	spin_lock(&kprobe_lock);
-	kprobe_cpu = smp_processor_id();
-
- 	local_irq_restore(flags);
-}
-
-void __kprobes unlock_kprobes(void)
-{
-	unsigned long flags = 0;
-
-	/* Avoiding local interrupts to happen right after we update
-	 * kprobe_cpu and before we get a a chance to release kprobe_lock,
-	 * this to prevent deadlock when we have a kprobe on ISR routine and
-	 * a kprobe on task routine
-	 */
-	local_irq_save(flags);
-
-	kprobe_cpu = NR_CPUS;
-	spin_unlock(&kprobe_lock);
-
- 	local_irq_restore(flags);
-}
-
 /* We have preemption disabled.. so it is safe to use __ versions */
 static inline void set_kprobe_instance(struct kprobe *kp)
 {

@@ -198,14 +162,19 @@ static inline void reset_kprobe_instance
 	__get_cpu_var(kprobe_instance) = NULL;
 }

Quoted 8 lines

kernel/kprobes.c static inline void reset_kprobe_instance · line 169
Quoted 6 lines
kernel/kprobes.c +5 -1
-/* You have to be holding the kprobe_lock */
+/*
+ * This routine is called either:
+ * 	- under the kprobe_lock spinlock - during kprobe_[un]register()
+ * 				OR
+ * 	- under an rcu_read_lock() - from arch/xxx/kernel/kprobes.c
Same here -- shouldn't the rcu_read_lock() be preempt_disable()?

Quoted 1 line

This is the same routine referred to in the comment above.

OK, same responses apply. ;-)

Quoted 216 lines

Quoted 215 lines

kernel/kprobes.c +34 ~~-22~~

+ */
 struct kprobe __kprobes *get_kprobe(void *addr)
 {
 	struct hlist_head *head;
 	struct hlist_node *node;
 
 	head = &kprobe_table[hash_ptr(addr, KPROBE_HASH_BITS)];
-	hlist_for_each(node, head) {
+	hlist_for_each_rcu(node, head) {
 		struct kprobe *p = hlist_entry(node, struct kprobe, hlist);
 		if (p->addr == addr)
 			return p;

@@ -221,7 +190,7 @@ static int __kprobes aggr_pre_handler(st
 {
 	struct kprobe *kp;
 
-	list_for_each_entry(kp, &p->list, list) {
+	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->pre_handler) {
 			set_kprobe_instance(kp);
 			if (kp->pre_handler(kp, regs))

@@ -237,7 +206,7 @@ static void __kprobes aggr_post_handler(
 {
 	struct kprobe *kp;
 
-	list_for_each_entry(kp, &p->list, list) {
+	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->post_handler) {
 			set_kprobe_instance(kp);
 			kp->post_handler(kp, regs, flags);

@@ -276,6 +245,7 @@ static int __kprobes aggr_break_handler(
 	return ret;
 }
 
+/* Called with kretprobe_lock held */
 struct kretprobe_instance __kprobes *get_free_rp_inst(struct kretprobe *rp)
 {
 	struct hlist_node *node;

@@ -285,6 +255,7 @@ struct kretprobe_instance __kprobes *get
 	return NULL;
 }
 
+/* Called with kretprobe_lock held */
 static struct kretprobe_instance __kprobes *get_used_rp_inst(struct kretprobe
 							      *rp)
 {

@@ -295,6 +266,7 @@ static struct kretprobe_instance __kprob
 	return NULL;
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes add_rp_inst(struct kretprobe_instance *ri)
 {
 	/*

@@ -313,6 +285,7 @@ void __kprobes add_rp_inst(struct kretpr
 	hlist_add_head(&ri->uflist, &ri->rp->used_instances);
 }
 
+/* Called with kretprobe_lock held */
 void __kprobes recycle_rp_inst(struct kretprobe_instance *ri)
 {
 	/* remove rp inst off the rprobe_inst_table */

@@ -346,13 +319,13 @@ void __kprobes kprobe_flush_task(struct 
 	struct hlist_node *node, *tmp;
 	unsigned long flags = 0;
 
-	spin_lock_irqsave(&kprobe_lock, flags);
+	spin_lock_irqsave(&kretprobe_lock, flags);
         head = kretprobe_inst_table_head(current);
         hlist_for_each_entry_safe(ri, node, tmp, head, hlist) {
                 if (ri->task == tk)
                         recycle_rp_inst(ri);
         }
-	spin_unlock_irqrestore(&kprobe_lock, flags);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 }
 
 /*

@@ -363,9 +336,12 @@ static int __kprobes pre_handler_kretpro
 					   struct pt_regs *regs)
 {
 	struct kretprobe *rp = container_of(p, struct kretprobe, kp);
+	unsigned long flags = 0;
 
 	/*TODO: consider to only swap the RA after the last pre_handler fired */
+	spin_lock_irqsave(&kretprobe_lock, flags);
 	arch_prepare_kretprobe(rp, regs);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 	return 0;
 }

@@ -396,13 +372,13 @@ static int __kprobes add_new_kprobe(stru
         struct kprobe *kp;
 
 	if (p->break_handler) {
-		list_for_each_entry(kp, &old_p->list, list) {
+		list_for_each_entry_rcu(kp, &old_p->list, list) {
 			if (kp->break_handler)
 				return -EEXIST;
 		}
-		list_add_tail(&p->list, &old_p->list);
+		list_add_tail_rcu(&p->list, &old_p->list);
 	} else
-		list_add(&p->list, &old_p->list);
+		list_add_rcu(&p->list, &old_p->list);
 	return 0;
 }

@@ -420,18 +396,18 @@ static inline void add_aggr_kprobe(struc
 	ap->break_handler = aggr_break_handler;
 
 	INIT_LIST_HEAD(&ap->list);
-	list_add(&p->list, &ap->list);
+	list_add_rcu(&p->list, &ap->list);
 
 	INIT_HLIST_NODE(&ap->hlist);
-	hlist_del(&p->hlist);
-	hlist_add_head(&ap->hlist,
+	hlist_del_rcu(&p->hlist);
+	hlist_add_head_rcu(&ap->hlist,
 		&kprobe_table[hash_ptr(ap->addr, KPROBE_HASH_BITS)]);
 }
 
 /*
  * This is the second or subsequent kprobe at the address - handle
  * the intricacies
- * TODO: Move kcalloc outside the spinlock
+ * TODO: Move kcalloc outside the spin_lock
  */
 static int __kprobes register_aggr_kprobe(struct kprobe *old_p,
 					  struct kprobe *p)

@@ -457,7 +433,7 @@ static int __kprobes register_aggr_kprob
 static inline void cleanup_kprobe(struct kprobe *p, unsigned long flags)
 {
 	arch_disarm_kprobe(p);
-	hlist_del(&p->hlist);
+	hlist_del_rcu(&p->hlist);
 	spin_unlock_irqrestore(&kprobe_lock, flags);
 	arch_remove_kprobe(p);
 }

@@ -465,11 +441,10 @@ static inline void cleanup_kprobe(struct
 static inline void cleanup_aggr_kprobe(struct kprobe *old_p,
 		struct kprobe *p, unsigned long flags)
 {
-	list_del(&p->list);
-	if (list_empty(&old_p->list)) {
+	list_del_rcu(&p->list);
+	if (list_empty(&old_p->list))
 		cleanup_kprobe(old_p, flags);
-		kfree(old_p);
-	} else
+	else
 		spin_unlock_irqrestore(&kprobe_lock, flags);
 }

@@ -492,9 +467,9 @@ int __kprobes register_kprobe(struct kpr
 	if ((ret = arch_prepare_kprobe(p)) != 0)
 		goto rm_kprobe;
 
+	p->nmissed = 0;
 	spin_lock_irqsave(&kprobe_lock, flags);
 	old_p = get_kprobe(p->addr);
-	p->nmissed = 0;
 	if (old_p) {
 		ret = register_aggr_kprobe(old_p, p);
 		goto out;

@@ -502,7 +477,7 @@ int __kprobes register_kprobe(struct kpr
 
 	arch_copy_kprobe(p);
 	INIT_HLIST_NODE(&p->hlist);
-	hlist_add_head(&p->hlist,
+	hlist_add_head_rcu(&p->hlist,
 		       &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
   	arch_arm_kprobe(p);

@@ -523,10 +498,16 @@ void __kprobes unregister_kprobe(struct 
 	spin_lock_irqsave(&kprobe_lock, flags);
 	old_p = get_kprobe(p->addr);
 	if (old_p) {
+		/* cleanup_*_kprobe() does the spin_unlock_irqrestore */
 		if (old_p->pre_handler == aggr_pre_handler)
 			cleanup_aggr_kprobe(old_p, p, flags);
 		else
 			cleanup_kprobe(p, flags);
+
+		synchronize_sched();
+		if (old_p->pre_handler == aggr_pre_handler &&
+				list_empty(&old_p->list))
+			kfree(old_p);
 	} else
 		spin_unlock_irqrestore(&kprobe_lock, flags);
 }

@@ -603,13 +584,13 @@ void __kprobes unregister_kretprobe(stru
 
 	unregister_kprobe(&rp->kp);
 	/* No race here */
-	spin_lock_irqsave(&kprobe_lock, flags);
+	spin_lock_irqsave(&kretprobe_lock, flags);
 	free_rp_inst(rp);
 	while ((ri = get_used_rp_inst(rp)) != NULL) {
 		ri->rp = NULL;
 		hlist_del(&ri->uflist);
 	}
-	spin_unlock_irqrestore(&kprobe_lock, flags);
+	spin_unlock_irqrestore(&kretprobe_lock, flags);
 }
 
 static int __init init_kprobes(void)
-

Ananth N Mavinakayanahalli

author

2005-10-18 17:10 UTC #16 lore raw

Re: [PATCH 8/9] Kprobes: Use RCU for (un)register synchronization - base changes

Quoted 4 lines · Paul E. McKenney

Quoted 3 lines · Ananth N Mavinakayanahalli
Quoted 2 lines · Paul E. McKenney
Quoted 1 line · Ananth N Mavinakayanahalli
From: Ananth N Mavinakayanahalli <[email protected]>

<snip>

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
include/linux/kprobes.h extern void show_registers(struct pt_reg · line 162
Quoted 2 lines
include/linux/kprobes.h +1 -1
-/* Get the kprobe at this addr (if any).  Must have called lock_kprobes */
+/* Get the kprobe at this addr (if any) - called under a rcu_read_lock() */
Since the update side uses synchronize_kernel() (in patch 9/9, right?),
Yes, synchronize_sched() to be precise.
Good, am not going blind yet. ;-)

Quoted 19 lines

Quoted 6 lines
Quoted 1 line
shouldn't the above "rcu_read_lock()" instead by preempt_disable()?
My reasoning was that since rcu_read_(un)lock is #defined to preempt_(en)disable. But, I agree, since we use synchronize_sched() this can just be preempt_disable()...
Suggestions?
In some realtime-friendly RCU implementations, the RCU read-side critical sections do not disable preemption. So, if you are using synchronize_sched() on the update side, you need to use preempt_disable() (or some other primitive that disables preemption, for example, any of the primitives that disables hardware interrupts) on the read side.
Quoted 1 line
From Documentation/RCU/whatisRCU.txt, the correspondence between the
update-side primitive and the read-side primitives needs to be as follows:
Defer Protect
a. synchronize_rcu() rcu_read_lock() / rcu_read_unlock()

Quoted 8 lines

	call_rcu()
b. call_rcu_bh() rcu_read_lock_bh() / rcu_read_unlock_bh()
c. synchronize_sched() preempt_disable() / preempt_enable()
local_irq_save() / local_irq_restore() hardirq enter / hardirq exit NMI enter / NMI exit

Thanks for the pointers.

Quoted 12 lines

Quoted 11 lines
Quoted 2 lines
Also, some of the code in the earlier patches seems to do preempt_disable. For example, kprobe_handler() in arch/i386/kernel/kprobes.c.
Well, the convolution is due to the way kprobes work:
- Breakpoint hit; execute pre_handler
- single-step on a copy of the original instruction; execute the post_handler
We don't want to be preempted from the time of the breakpoint exception until after the post_handler is run. I've taken care to ensure the preempt_ calls from the various codepaths are balanced.

Quoted 6 lines

OK, didn't follow this -- thank you for the explanation! This sequence of events happens on each execution of the kprobe handler, or is there some optimization for subsequent invocations of the same kprobe handler?
If the former, then perhaps there is some way to make the preempt_disable() that protects the single stepping also protect the probe lookup.

It is the former.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
In realtime kernels, synchronize_sched() does -not- guarantee that all rcu_read_lock() critical sections have finished, only that any preempt_disable() code sequences (explicit or implicit) have completed.
Are we assured that the preempt_ depth is also taken care of? If that is the case, I think we are safe, since the matching preempt_enable() calls are _after_ the post_handler is executed.

Quoted 5 lines

Seems so to me -- though if I understand you correctly, you should then be able to simply remove the rcu_read_lock() and rcu_read_unlock() calls, since that code sequence would be covered by the preempt_disable().
Or am I missing something?

Nope, you are right. The extra depth was introduced so the older kprobes code wouldn't throw warnings on every smp_processor_id() invocation without preemption disabled. With the current changes, we can get rid of the extra depth with a little bit of code rearrangement. I am on it now.

Thanks,
Ananth