[PATCH 2/7] kprobes: Add kprobe_insn_mutex and cleanup arch_remove_kprobe()

[PATCH 2/7] kprobes: Add kprobe_insn_mutex and cleanup arch_remove_kprobe()

[Masami Hiramatsu]

Add kprobe_insn_mutex for protecting kprobe_insn_pages hlist, and remove
kprobe_mutex from architecture dependent code.

This allows us to call arch_remove_kprobe() (and free_insn_slot) while holding
kprobe_mutex.

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
---
 arch/arm/kernel/kprobes.c     |    2 --
 arch/ia64/kernel/kprobes.c    |    8 +++++---
 arch/powerpc/kernel/kprobes.c |    7 ++++---
 arch/s390/kernel/kprobes.c    |    7 ++++---
 arch/x86/kernel/kprobes.c     |    7 ++++---
 include/linux/kprobes.h       |    1 -
 kernel/kprobes.c              |   25 +++++++++++++++++++++----
 7 files changed, 38 insertions(+), 19 deletions(-)

Index: 2.6.28-rc4/arch/arm/kernel/kprobes.c
===================================================================
--- 2.6.28-rc4.orig/arch/arm/kernel/kprobes.c
+++ 2.6.28-rc4/arch/arm/kernel/kprobes.c
@@ -92,9 +92,7 @@ void __kprobes arch_disarm_kprobe(struct
 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
 	if (p->ainsn.insn) {
-		mutex_lock(&kprobe_mutex);
 		free_insn_slot(p->ainsn.insn, 0);
-		mutex_unlock(&kprobe_mutex);
 		p->ainsn.insn = NULL;
 	}
 }
Index: 2.6.28-rc4/arch/ia64/kernel/kprobes.c
===================================================================
--- 2.6.28-rc4.orig/arch/ia64/kernel/kprobes.c
+++ 2.6.28-rc4/arch/ia64/kernel/kprobes.c
@@ -670,9 +670,11 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
-	free_insn_slot(p->ainsn.insn, p->ainsn.inst_flag & INST_FLAG_BOOSTABLE);
-	mutex_unlock(&kprobe_mutex);
+	if (p->ainsn.insn) {
+		free_insn_slot(p->ainsn.insn,
+			       p->ainsn.inst_flag & INST_FLAG_BOOSTABLE);
+		p->ainsn.insn = NULL;
+	}
 }
 /*
  * We are resuming execution after a single step fault, so the pt_regs
Index: 2.6.28-rc4/arch/powerpc/kernel/kprobes.c
===================================================================
--- 2.6.28-rc4.orig/arch/powerpc/kernel/kprobes.c
+++ 2.6.28-rc4/arch/powerpc/kernel/kprobes.c
@@ -96,9 +96,10 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
-	free_insn_slot(p->ainsn.insn, 0);
-	mutex_unlock(&kprobe_mutex);
+	if (p->ainsn.insn) {
+		free_insn_slot(p->ainsn.insn, 0);
+		p->ainsn.insn = NULL;
+	}
 }

 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
Index: 2.6.28-rc4/arch/s390/kernel/kprobes.c
===================================================================
--- 2.6.28-rc4.orig/arch/s390/kernel/kprobes.c
+++ 2.6.28-rc4/arch/s390/kernel/kprobes.c
@@ -218,9 +218,10 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
-	free_insn_slot(p->ainsn.insn, 0);
-	mutex_unlock(&kprobe_mutex);
+	if (p->ainsn.insn) {
+		free_insn_slot(p->ainsn.insn, 0);
+		p->ainsn.insn = NULL;
+	}
 }

 static void __kprobes prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
Index: 2.6.28-rc4/arch/x86/kernel/kprobes.c
===================================================================
--- 2.6.28-rc4.orig/arch/x86/kernel/kprobes.c
+++ 2.6.28-rc4/arch/x86/kernel/kprobes.c
@@ -376,9 +376,10 @@ void __kprobes arch_disarm_kprobe(struct

 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	mutex_lock(&kprobe_mutex);
-	free_insn_slot(p->ainsn.insn, (p->ainsn.boostable == 1));
-	mutex_unlock(&kprobe_mutex);
+	if (p->ainsn.insn) {
+		free_insn_slot(p->ainsn.insn, (p->ainsn.boostable == 1));
+		p->ainsn.insn = NULL;
+	}
 }

 static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
Index: 2.6.28-rc4/kernel/kprobes.c
===================================================================
--- 2.6.28-rc4.orig/kernel/kprobes.c
+++ 2.6.28-rc4/kernel/kprobes.c
@@ -69,7 +69,7 @@ static struct hlist_head kretprobe_inst_
 /* NOTE: change this value only with kprobe_mutex held */
 static bool kprobe_enabled;

-DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
+static DEFINE_MUTEX(kprobe_mutex);	/* Protects kprobe_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 static struct {
 	spinlock_t lock ____cacheline_aligned;
@@ -115,6 +115,7 @@ enum kprobe_slot_state {
 	SLOT_USED = 2,
 };

+static DEFINE_MUTEX(kprobe_insn_mutex);	/* Protects kprobe_insn_pages */
 static struct hlist_head kprobe_insn_pages;
 static int kprobe_garbage_slots;
 static int collect_garbage_slots(void);
@@ -144,10 +145,10 @@ loop_end:
 }

 /**
- * get_insn_slot() - Find a slot on an executable page for an instruction.
+ * __get_insn_slot() - Find a slot on an executable page for an instruction.
  * We allocate an executable page if there's no room on existing ones.
  */
-kprobe_opcode_t __kprobes *get_insn_slot(void)
+static kprobe_opcode_t __kprobes *__get_insn_slot(void)
 {
 	struct kprobe_insn_page *kip;
 	struct hlist_node *pos;
@@ -196,6 +197,15 @@ kprobe_opcode_t __kprobes *get_insn_slot
 	return kip->insns;
 }

+kprobe_opcode_t __kprobes *get_insn_slot(void)
+{
+	kprobe_opcode_t *ret;
+	mutex_lock(&kprobe_insn_mutex);
+	ret = __get_insn_slot();
+	mutex_unlock(&kprobe_insn_mutex);
+	return ret;
+}
+
 /* Return 1 if all garbages are collected, otherwise 0. */
 static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx)
 {
@@ -226,9 +236,13 @@ static int __kprobes collect_garbage_slo
 {
 	struct kprobe_insn_page *kip;
 	struct hlist_node *pos, *next;
+	int safety;

 	/* Ensure no-one is preepmted on the garbages */
-	if (check_safety() != 0)
+	mutex_unlock(&kprobe_insn_mutex);
+	safety = check_safety();
+	mutex_lock(&kprobe_insn_mutex);
+	if (safety != 0)
 		return -EAGAIN;

 	hlist_for_each_entry_safe(kip, pos, next, &kprobe_insn_pages, hlist) {
@@ -251,6 +265,7 @@ void __kprobes free_insn_slot(kprobe_opc
 	struct kprobe_insn_page *kip;
 	struct hlist_node *pos;

+	mutex_lock(&kprobe_insn_mutex);
 	hlist_for_each_entry(kip, pos, &kprobe_insn_pages, hlist) {
 		if (kip->insns <= slot &&
 		    slot < kip->insns + (INSNS_PER_PAGE * MAX_INSN_SIZE)) {
@@ -267,6 +282,8 @@ void __kprobes free_insn_slot(kprobe_opc

 	if (dirty && ++kprobe_garbage_slots > INSNS_PER_PAGE)
 		collect_garbage_slots();
+
+	mutex_unlock(&kprobe_insn_mutex);
 }
 #endif

Index: 2.6.28-rc4/include/linux/kprobes.h
===================================================================
--- 2.6.28-rc4.orig/include/linux/kprobes.h
+++ 2.6.28-rc4/include/linux/kprobes.h
@@ -201,7 +201,6 @@ static inline int init_test_probes(void)
 }
 #endif /* CONFIG_KPROBES_SANITY_TEST */

-extern struct mutex kprobe_mutex;
 extern int arch_prepare_kprobe(struct kprobe *p);
 extern void arch_arm_kprobe(struct kprobe *p);
 extern void arch_disarm_kprobe(struct kprobe *p);

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 2/7] kprobes: Add kprobe_insn_mutex and cleanup arch_remove_kprobe()

[Ananth N Mavinakayanahalli]

On Tue, Nov 11, 2008 at 03:56:21PM -0500, Masami Hiramatsu wrote:
> Add kprobe_insn_mutex for protecting kprobe_insn_pages hlist, and remove
> kprobe_mutex from architecture dependent code.
> 
> This allows us to call arch_remove_kprobe() (and free_insn_slot) while holding
> kprobe_mutex.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Re: [PATCH 2/7] kprobes: Add kprobe_insn_mutex and cleanup arch_remove_kprobe()

[Andrew Morton]

On Tue, 11 Nov 2008 15:56:21 -0500
Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Add kprobe_insn_mutex for protecting kprobe_insn_pages hlist, and remove
> kprobe_mutex from architecture dependent code.
> 
> This allows us to call arch_remove_kprobe() (and free_insn_slot) while holding
> kprobe_mutex.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> ---
>  arch/arm/kernel/kprobes.c     |    2 --
>  arch/ia64/kernel/kprobes.c    |    8 +++++---
>  arch/powerpc/kernel/kprobes.c |    7 ++++---
>  arch/s390/kernel/kprobes.c    |    7 ++++---
>  arch/x86/kernel/kprobes.c     |    7 ++++---
>  include/linux/kprobes.h       |    1 -
>  kernel/kprobes.c              |   25 +++++++++++++++++++++----

For some reasons sparc64 never had this mutex.  You've checked that
sparc64 will be OK after this change?

Re: [PATCH 2/7] kprobes: Add kprobe_insn_mutex and cleanup arch_remove_kprobe()

[Masami Hiramatsu]

Hi Andrew,

Andrew Morton wrote:
> On Tue, 11 Nov 2008 15:56:21 -0500
> Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
>> Add kprobe_insn_mutex for protecting kprobe_insn_pages hlist, and remove
>> kprobe_mutex from architecture dependent code.
>>
>> This allows us to call arch_remove_kprobe() (and free_insn_slot) while holding
>> kprobe_mutex.
>>
>> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
>> ---
>>  arch/arm/kernel/kprobes.c     |    2 --
>>  arch/ia64/kernel/kprobes.c    |    8 +++++---
>>  arch/powerpc/kernel/kprobes.c |    7 ++++---
>>  arch/s390/kernel/kprobes.c    |    7 ++++---
>>  arch/x86/kernel/kprobes.c     |    7 ++++---
>>  include/linux/kprobes.h       |    1 -
>>  kernel/kprobes.c              |   25 +++++++++++++++++++++----
> 
> For some reasons sparc64 never had this mutex.  You've checked that
> sparc64 will be OK after this change?

I think it's OK for those archs.

Actually, this mutex is protecting insn_slot which stores copied
instructions into executable memory. Some architectures which
doesn't need that special memory have its own buffer in the
arch_specific_insn and they don't use insn_slot. On those arch,
kprobe_insn_mutex is not used (not defined).

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com