LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCHv2] kprobes: Introduce is_kprobe_fault()
@ 2008-01-07 20:24 Harvey Harrison
  2008-01-08  5:37 ` Ananth N Mavinakayanahalli
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-01-07 20:24 UTC (permalink / raw)
  To: Andrew Morton, LKML, Masami Hiramatsu, Ananth N Mavinakayanahalli
  Cc: David Miller, hskinnemoen, schwidefsky, heiko.carstens,
	tony.luck, paulus, Ingo Molnar

Use a central is_kprobe_fault() inline in kprobes.h to remove all
of the arch-dependant, practically identical implementations in
avr32, ia64, powerpc, s390, sparc64, and x86.

avr32 was the only arch without the preempt_disable/enable pair
in its notify_page_fault implementation.

This uncovered a possible bug in the s390 version as that purely
copied the x86 version unconditionally passing 14 as the trapnr
rather than the error_code parameter.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Andrew, update to my previous patch, with relevant (I hope)
maintainers CC'd.  Patch against v2.6.24-rc7

X86_32 is ignored for now as it truly needs user_mode_vm
in the implementation.

 arch/avr32/mm/fault.c   |   21 +--------------------
 arch/ia64/mm/fault.c    |   24 +-----------------------
 arch/powerpc/mm/fault.c |   25 +------------------------
 arch/s390/mm/fault.c    |   25 +------------------------
 arch/sparc64/mm/fault.c |   23 +----------------------
 arch/x86/mm/fault_64.c  |   26 ++------------------------
 include/linux/kprobes.h |   19 +++++++++++++++++++
 7 files changed, 26 insertions(+), 137 deletions(-)

diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
index 6560cb1..a95cce2 100644
--- a/arch/avr32/mm/fault.c
+++ b/arch/avr32/mm/fault.c
@@ -20,25 +20,6 @@
 #include <asm/tlb.h>
 #include <asm/uaccess.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		if (kprobe_running() && kprobe_fault_handler(regs, trap))
-			ret = 1;
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 int exception_trace = 1;
 
 /*
@@ -66,7 +47,7 @@ asmlinkage void do_page_fault(unsigned long ecr, struct pt_regs *regs)
 	int code;
 	int fault;
 
-	if (notify_page_fault(regs, ecr))
+	if (is_kprobe_fault(regs, ecr))
 		return;
 
 	address = sysreg_read(TLBEAR);
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 7571076..1696805 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -18,28 +18,6 @@
 
 extern void die (char *, struct pt_regs *, long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		/* kprobe_running() needs smp_processor_id() */
-		preempt_disable();
-		if (kprobe_running() && kprobes_fault_handler(regs, trap))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 /*
  * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
  * (inside region 5, on ia64) and that page is present.
@@ -106,7 +84,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	/*
 	 * This is to handle the kprobes on user space access instructions
 	 */
-	if (notify_page_fault(regs, TRAP_BRKPT))
+	if (is_kprobe_fault(regs, TRAP_BRKPT))
 		return;
 
 	down_read(&mm->mmap_sem);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8135da0..00df6f9 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -39,29 +39,6 @@
 #include <asm/tlbflush.h>
 #include <asm/siginfo.h>
 
-
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 11))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
@@ -164,7 +141,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 
-	if (notify_page_fault(regs))
+	if (is_kprobe_fault(regs, 11))
 		return 0;
 
 	if (trap == 0x300) {
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2456b52..59d3f0e 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
 
 extern void die(const char *,struct pt_regs *,long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	return 0;
-}
-#endif
-
-
 /*
  * Unlock any spinlocks which will prevent us from getting the
  * message out.
@@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
 	int si_code;
 	int fault;
 
-	if (notify_page_fault(regs, error_code))
+	if (is_kprobe_fault(regs, 14))
 		return;
 
 	tsk = current;
diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
index e2027f2..bb5c36a 100644
--- a/arch/sparc64/mm/fault.c
+++ b/arch/sparc64/mm/fault.c
@@ -31,27 +31,6 @@
 #include <asm/sections.h>
 #include <asm/mmu_context.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 0))
-			ret = 1;
-		preempt_enable();
-	}
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * To debug kernel to catch accesses to certain virtual/physical addresses.
  * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
@@ -280,7 +259,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 
 	fault_code = get_thread_fault_code();
 
-	if (notify_page_fault(regs))
+	if (is_kprobe_fault(regs, 0))
 		return;
 
 	si_code = SEGV_MAPERR;
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index 0e26230..376e218 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -41,28 +41,6 @@
 #define PF_RSVD	(1<<3)
 #define PF_INSTR	(1<<4)
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /* Sometimes the CPU reports invalid exceptions on prefetch.
    Check that here and ignore.
    Opcode checker based on code by Richard Brunner */
@@ -343,7 +321,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 			if (vmalloc_fault(address) >= 0)
 				return;
 		}
-		if (notify_page_fault(regs))
+		if (is_kprobe_fault(regs, 14))
 			return;
 		/*
 		 * Don't take the mm semaphore here. If we fixup a prefetch
@@ -352,7 +330,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 		goto bad_area_nosemaphore;
 	}
 
-	if (notify_page_fault(regs))
+	if (is_kprobe_fault(regs, 14))
 		return;
 
 	if (likely(regs->eflags & X86_EFLAGS_IF))
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 8189158..65c1ffb 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/hardirq.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>
@@ -203,6 +204,20 @@ static inline struct kprobe *kprobe_running(void)
 	return (__get_cpu_var(current_kprobe));
 }
 
+/*
+ * If it is a kprobe pagefault we can not be premptible so return before
+ * calling kprobe_running() as it will assert on smp_processor_id if
+ * preemption is enabled.
+ */
+static inline int is_kprobe_fault(struct pt_regs *regs, int trapnr)
+{
+	if (!user_mode(regs) && !preemptible() && kprobe_running() &&
+	    kprobe_fault_handler(regs, trapnr))
+		return 1;
+	else
+		return 0;
+}
+
 static inline void reset_current_kprobe(void)
 {
 	__get_cpu_var(current_kprobe) = NULL;
@@ -237,6 +252,10 @@ static inline struct kprobe *kprobe_running(void)
 {
 	return NULL;
 }
+static inline int is_kprobe_fault(struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
 static inline int register_kprobe(struct kprobe *p)
 {
 	return -ENOSYS;
-- 
1.5.4.rc2.1117.ge5809


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv2] kprobes: Introduce is_kprobe_fault()
  2008-01-07 20:24 [PATCHv2] kprobes: Introduce is_kprobe_fault() Harvey Harrison
@ 2008-01-08  5:37 ` Ananth N Mavinakayanahalli
  2008-01-08 17:03 ` Masami Hiramatsu
  2008-01-08 22:45 ` Paul Mackerras
  2 siblings, 0 replies; 24+ messages in thread
From: Ananth N Mavinakayanahalli @ 2008-01-08  5:37 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Andrew Morton, LKML, Masami Hiramatsu, David Miller, hskinnemoen,
	schwidefsky, heiko.carstens, tony.luck, paulus, Ingo Molnar

On Mon, Jan 07, 2008 at 12:24:46PM -0800, Harvey Harrison wrote:
> Use a central is_kprobe_fault() inline in kprobes.h to remove all
> of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

Tested on powerpc.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv2] kprobes: Introduce is_kprobe_fault()
  2008-01-07 20:24 [PATCHv2] kprobes: Introduce is_kprobe_fault() Harvey Harrison
  2008-01-08  5:37 ` Ananth N Mavinakayanahalli
@ 2008-01-08 17:03 ` Masami Hiramatsu
  2008-01-08 22:45 ` Paul Mackerras
  2 siblings, 0 replies; 24+ messages in thread
From: Masami Hiramatsu @ 2008-01-08 17:03 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Andrew Morton, LKML, Ananth N Mavinakayanahalli, David Miller,
	hskinnemoen, schwidefsky, heiko.carstens, tony.luck, paulus,
	Ingo Molnar

Hi,

Thank you for good work!

Harvey Harrison wrote:
> Use a central is_kprobe_fault() inline in kprobes.h to remove all
> of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>


I tested it on x86-64.

Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv2] kprobes: Introduce is_kprobe_fault()
  2008-01-07 20:24 [PATCHv2] kprobes: Introduce is_kprobe_fault() Harvey Harrison
  2008-01-08  5:37 ` Ananth N Mavinakayanahalli
  2008-01-08 17:03 ` Masami Hiramatsu
@ 2008-01-08 22:45 ` Paul Mackerras
  2008-01-08 23:02   ` Harvey Harrison
  2 siblings, 1 reply; 24+ messages in thread
From: Paul Mackerras @ 2008-01-08 22:45 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, heiko.carstens, tony.luck, Ingo Molnar

Harvey Harrison writes:

> Use a central is_kprobe_fault() inline in kprobes.h to remove all
> of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.

I don't like the name "is_kprobe_fault" since the function actually
handles the fault - i.e. it does more than just tell the caller
whether this is a kprobes fault.  Something like
"handle_kprobes_fault" or "maybe_handle_kprobes_fault" would be
better IMO.

Paul.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv2] kprobes: Introduce is_kprobe_fault()
  2008-01-08 22:45 ` Paul Mackerras
@ 2008-01-08 23:02   ` Harvey Harrison
  2008-01-09  4:19     ` [PATCHv3] kprobes: Introduce kprobe_handle_fault() Harvey Harrison
  0 siblings, 1 reply; 24+ messages in thread
From: Harvey Harrison @ 2008-01-08 23:02 UTC (permalink / raw)
  To: Paul Mackerras
  Cc: Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, heiko.carstens, tony.luck, Ingo Molnar

On Wed, 2008-01-09 at 09:45 +1100, Paul Mackerras wrote:
> Harvey Harrison writes:
> 
> > Use a central is_kprobe_fault() inline in kprobes.h to remove all
> > of the arch-dependant, practically identical implementations in
> > avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> I don't like the name "is_kprobe_fault" since the function actually
> handles the fault - i.e. it does more than just tell the caller
> whether this is a kprobes fault.  Something like
> "handle_kprobes_fault" or "maybe_handle_kprobes_fault" would be
> better IMO.

Good point, I chose the name based simply on the usage pattern found
in all the callers.  Of your suggestions I like handle_kprobes_fault
better.

How about check_kprobes_fault?  That seems to cover what you were
getting at with maybe_handle_kprobes_fault but is shorter.  That
also fits better with the !CONFIG_KPROBES case.

Harvey




^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCHv3] kprobes: Introduce kprobe_handle_fault()
  2008-01-08 23:02   ` Harvey Harrison
@ 2008-01-09  4:19     ` Harvey Harrison
  2008-01-09  6:14       ` Heiko Carstens
  2008-01-09  7:57       ` Christoph Hellwig
  0 siblings, 2 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-01-09  4:19 UTC (permalink / raw)
  To: Andrew Morton
  Cc: LKML, Masami Hiramatsu, Ananth N Mavinakayanahalli, David Miller,
	hskinnemoen, schwidefsky, heiko.carstens, tony.luck, Ingo Molnar,
	Paul Mackerras

Use a central kprobe_handle_fault() inline in kprobes.h to remove
all of the arch-dependant, practically identical implementations in
avr32, ia64, powerpc, s390, sparc64, and x86.

avr32 was the only arch without the preempt_disable/enable pair
in its notify_page_fault implementation.

This uncovered a possible bug in the s390 version as that purely
copied the x86 version unconditionally passing 14 as the trapnr
rather than the error_code parameter.

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

X86-64
Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 arch/avr32/mm/fault.c   |   21 +--------------------
 arch/ia64/mm/fault.c    |   24 +-----------------------
 arch/powerpc/mm/fault.c |   25 +------------------------
 arch/s390/mm/fault.c    |   25 +------------------------
 arch/sparc64/mm/fault.c |   23 +----------------------
 arch/x86/mm/fault_64.c  |   26 ++------------------------
 include/linux/kprobes.h |   19 +++++++++++++++++++
 7 files changed, 26 insertions(+), 137 deletions(-)

diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
index 6560cb1..e41953e 100644
--- a/arch/avr32/mm/fault.c
+++ b/arch/avr32/mm/fault.c
@@ -20,25 +20,6 @@
 #include <asm/tlb.h>
 #include <asm/uaccess.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		if (kprobe_running() && kprobe_fault_handler(regs, trap))
-			ret = 1;
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 int exception_trace = 1;
 
 /*
@@ -66,7 +47,7 @@ asmlinkage void do_page_fault(unsigned long ecr, struct pt_regs *regs)
 	int code;
 	int fault;
 
-	if (notify_page_fault(regs, ecr))
+	if (kprobe_handle_fault(regs, ecr))
 		return;
 
 	address = sysreg_read(TLBEAR);
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 7571076..bfc83e8 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -18,28 +18,6 @@
 
 extern void die (char *, struct pt_regs *, long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		/* kprobe_running() needs smp_processor_id() */
-		preempt_disable();
-		if (kprobe_running() && kprobes_fault_handler(regs, trap))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 /*
  * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
  * (inside region 5, on ia64) and that page is present.
@@ -106,7 +84,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	/*
 	 * This is to handle the kprobes on user space access instructions
 	 */
-	if (notify_page_fault(regs, TRAP_BRKPT))
+	if (kprobe_handle_fault(regs, TRAP_BRKPT))
 		return;
 
 	down_read(&mm->mmap_sem);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8135da0..ff64bd3 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -39,29 +39,6 @@
 #include <asm/tlbflush.h>
 #include <asm/siginfo.h>
 
-
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 11))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
@@ -164,7 +141,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 11))
 		return 0;
 
 	if (trap == 0x300) {
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2456b52..a9033cf 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
 
 extern void die(const char *,struct pt_regs *,long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	return 0;
-}
-#endif
-
-
 /*
  * Unlock any spinlocks which will prevent us from getting the
  * message out.
@@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
 	int si_code;
 	int fault;
 
-	if (notify_page_fault(regs, error_code))
+	if (kprobe_handle_fault(regs, 14))
 		return;
 
 	tsk = current;
diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
index e2027f2..8467dd6 100644
--- a/arch/sparc64/mm/fault.c
+++ b/arch/sparc64/mm/fault.c
@@ -31,27 +31,6 @@
 #include <asm/sections.h>
 #include <asm/mmu_context.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 0))
-			ret = 1;
-		preempt_enable();
-	}
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * To debug kernel to catch accesses to certain virtual/physical addresses.
  * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
@@ -280,7 +259,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 
 	fault_code = get_thread_fault_code();
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 0))
 		return;
 
 	si_code = SEGV_MAPERR;
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index 0e26230..c29e462 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -41,28 +41,6 @@
 #define PF_RSVD	(1<<3)
 #define PF_INSTR	(1<<4)
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /* Sometimes the CPU reports invalid exceptions on prefetch.
    Check that here and ignore.
    Opcode checker based on code by Richard Brunner */
@@ -343,7 +321,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 			if (vmalloc_fault(address) >= 0)
 				return;
 		}
-		if (notify_page_fault(regs))
+		if (kprobe_handle_fault(regs, 14))
 			return;
 		/*
 		 * Don't take the mm semaphore here. If we fixup a prefetch
@@ -352,7 +330,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 		goto bad_area_nosemaphore;
 	}
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 14))
 		return;
 
 	if (likely(regs->eflags & X86_EFLAGS_IF))
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 8189158..a1ca019 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/hardirq.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>
@@ -203,6 +204,20 @@ static inline struct kprobe *kprobe_running(void)
 	return (__get_cpu_var(current_kprobe));
 }
 
+/*
+ * If it is a kprobe pagefault we can not be premptible so return before
+ * calling kprobe_running() as it will assert on smp_processor_id if
+ * preemption is enabled.
+ */
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	if (!user_mode(regs) && !preemptible() && kprobe_running() &&
+	    kprobe_fault_handler(regs, trapnr))
+		return 1;
+	else
+		return 0;
+}
+
 static inline void reset_current_kprobe(void)
 {
 	__get_cpu_var(current_kprobe) = NULL;
@@ -237,6 +252,10 @@ static inline struct kprobe *kprobe_running(void)
 {
 	return NULL;
 }
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
 static inline int register_kprobe(struct kprobe *p)
 {
 	return -ENOSYS;
-- 
1.5.4.rc2.1164.g6451




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
  2008-01-09  4:19     ` [PATCHv3] kprobes: Introduce kprobe_handle_fault() Harvey Harrison
@ 2008-01-09  6:14       ` Heiko Carstens
  2008-01-09  6:22         ` Harvey Harrison
  2008-01-09  7:58         ` [PATCHv3] " Christoph Hellwig
  2008-01-09  7:57       ` Christoph Hellwig
  1 sibling, 2 replies; 24+ messages in thread
From: Heiko Carstens @ 2008-01-09  6:14 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras

> +/*
> + * If it is a kprobe pagefault we can not be premptible so return before

Missing 'e' in preemptible.
However, the old code you removed had a lot of preempt_disable/enable calls
that you removed. Hope you checked that preemption was always disabled
already and the calls were not necessary (true at least for s390).

Are there cases where this code could be called with preemption enabled?
If so then that looks like a bug anyway. I'd say the preemptible check
should be removed or turned into a WARN_ON.

> + * calling kprobe_running() as it will assert on smp_processor_id if
> + * preemption is enabled.
> + */
> +static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
> +{
> +	if (!user_mode(regs) && !preemptible() && kprobe_running() &&
> +	    kprobe_fault_handler(regs, trapnr))
> +		return 1;
> +	else
> +		return 0;

I like this better (not including any other changes):

	if (!user_mode(regs) && !preemptible() && kprobe_running())
		return kprobe_fault_handler(regs, trapnr);
	return 0;

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
  2008-01-09  6:14       ` Heiko Carstens
@ 2008-01-09  6:22         ` Harvey Harrison
  2008-01-09 22:01           ` [PATCHv4] " Harvey Harrison
  2008-01-09  7:58         ` [PATCHv3] " Christoph Hellwig
  1 sibling, 1 reply; 24+ messages in thread
From: Harvey Harrison @ 2008-01-09  6:22 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras

On Wed, 2008-01-09 at 07:14 +0100, Heiko Carstens wrote:
> > +/*
> > + * If it is a kprobe pagefault we can not be premptible so return before
> 
> Missing 'e' in preemptible.

OK.

> However, the old code you removed had a lot of preempt_disable/enable calls
> that you removed. Hope you checked that preemption was always disabled
> already and the calls were not necessary (true at least for s390).
> 
> Are there cases where this code could be called with preemption enabled?
> If so then that looks like a bug anyway. I'd say the preemptible check
> should be removed or turned into a WARN_ON.
> 
> I like this better (not including any other changes):
> 
> 	if (!user_mode(regs) && !preemptible() && kprobe_running())
> 		return kprobe_fault_handler(regs, trapnr);
> 	return 0;

I could live with that too, will defer to kprobes maintainers if they
prefer that as a follow-on.

Regarding the preempt_enable/disable, the reasoning behind it comes from
the following, I stole the changelog from x86.git which has a good
description of why this should be safe:

commit 6624c638928acce52fbe57d73284efcf9f86abd2
Author: Quentin Barnes <qbarnes@gmail.com>
Date:   Wed Jan 9 02:32:57 2008 +0100

    Code clarification patch to Kprobes arch code
    
    When developing the Kprobes arch code for ARM, I ran across some
code
    found in x86 and s390 Kprobes arch code which I didn't consider as
    good as it could be.
    
    Once I figured out what the code was doing, I changed the code
    for ARM Kprobes to work the way I felt was more appropriate.
    I've tested the code this way in ARM for about a year and would
    like to push the same change to the other affected architectures.
    
    The code in question is in kprobe_exceptions_notify() which
    does:
    ====
              /* kprobe_running() needs smp_processor_id() */
              preempt_disable();
              if (kprobe_running() &&
                  kprobe_fault_handler(args->regs, args->trapnr))
                      ret = NOTIFY_STOP;
              preempt_enable();
    ====
    
    For the moment, ignore the code having the preempt_disable()/
    preempt_enable() pair in it.
    
    The problem is that kprobe_running() needs to call
smp_processor_id()
    which will assert if preemption is enabled.  That sanity check by
    smp_processor_id() makes perfect sense since calling it with
preemption
    enabled would return an unreliable result.
    
    But the function kprobe_exceptions_notify() can be called from a
    context where preemption could be enabled.  If that happens, the
    assertion in smp_processor_id() happens and we're dead.  So what
    the original author did (speculation on my part!) is put in the
    preempt_disable()/preempt_enable() pair to simply defeat the check.
    
    Once I figured out what was going on, I considered this an
    inappropriate approach.  If kprobe_exceptions_notify() is called
    from a preemptible context, we can't be in a kprobe processing
    context at that time anyways since kprobes requires preemption to
    already be disabled, so just check for preemption enabled, and if
    so, blow out before ever calling kprobe_running().  I wrote the ARM
    kprobe code like this:
    ====
              /* To be potentially processing a kprobe fault and to
               * trust the result from kprobe_running(), we have
               * be non-preemptible. */
              if (!preemptible() && kprobe_running() &&
                  kprobe_fault_handler(args->regs, args->trapnr))
                      ret = NOTIFY_STOP;
    ====
    
    The above code has been working fine for ARM Kprobes for a year.
    So I changed the x86 code (2.6.24-rc6) to be the same way and ran
    the Systemtap tests on that kernel.  As on ARM, Systemtap on x86
    comes up with the same test results either way, so it's a neutral
    external functional change (as expected).
    
    This issue has been discussed previously on linux-arm-kernel and the
    Systemtap mailing lists.  Pointers to the by base for the two
    discussions:

http://lists.arm.linux.org.uk/lurker/message/20071219.223225.1f5c2a5e.en.html
    http://sourceware.org/ml/systemtap/2007-q1/msg00251.html


Cheers,

Harvey


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
  2008-01-09  4:19     ` [PATCHv3] kprobes: Introduce kprobe_handle_fault() Harvey Harrison
  2008-01-09  6:14       ` Heiko Carstens
@ 2008-01-09  7:57       ` Christoph Hellwig
  1 sibling, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2008-01-09  7:57 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, heiko.carstens, tony.luck, Ingo Molnar,
	Paul Mackerras

On Tue, Jan 08, 2008 at 08:19:20PM -0800, Harvey Harrison wrote:
> Use a central kprobe_handle_fault() inline in kprobes.h to remove
> all of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.

This is not an equivalnet transformation.  The old code always made
sure to disable preemption and the new one just skips the whole
kprobes handler if preemption is enabled.  Please separate code
consolidation from changing semantics _and_ describe the semantics
changes in detail.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
  2008-01-09  6:14       ` Heiko Carstens
  2008-01-09  6:22         ` Harvey Harrison
@ 2008-01-09  7:58         ` Christoph Hellwig
  1 sibling, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2008-01-09  7:58 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Harvey Harrison, Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras

On Wed, Jan 09, 2008 at 07:14:08AM +0100, Heiko Carstens wrote:
> I like this better (not including any other changes):
> 
> 	if (!user_mode(regs) && !preemptible() && kprobe_running())
> 		return kprobe_fault_handler(regs, trapnr);

seconded.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCHv4] kprobes: Introduce kprobe_handle_fault()
  2008-01-09  6:22         ` Harvey Harrison
@ 2008-01-09 22:01           ` Harvey Harrison
  2008-01-09 23:16             ` Heiko Carstens
  2008-01-09 23:31             ` [PATCHv4] kprobes: Introduce kprobe_handle_fault() Masami Hiramatsu
  0 siblings, 2 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-01-09 22:01 UTC (permalink / raw)
  To: Heiko Carstens, Christoph Hellwig, Andrew Morton
  Cc: LKML, Masami Hiramatsu, Ananth N Mavinakayanahalli, David Miller,
	hskinnemoen, schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras

Use a central kprobe_handle_fault() inline in kprobes.h to remove
all of the arch-dependant, practically identical implementations in
avr32, ia64, powerpc, s390, sparc64, and x86.

This patch removes the preempt_disable/enable pair around kprobe_running
which was originally added to avoid the assertion from smp_processor_id
which would be unreliable if preemption was enabled.

Kprobes can not be running if we are preemptible, so test explicitly
for preemption and bail out before hitting kprobe_running().

Style comments from Christoph Hellwig and Heiko Carstens included in
this version.

avr32 was the only arch without the preempt_disable/enable pair
in its notify_page_fault implementation.

This uncovered a possible bug in the s390 version as that purely
copied the x86 version unconditionally passing 14 as the trapnr
rather than the error_code parameter.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 arch/avr32/mm/fault.c   |   21 +--------------------
 arch/ia64/mm/fault.c    |   24 +-----------------------
 arch/powerpc/mm/fault.c |   25 +------------------------
 arch/s390/mm/fault.c    |   25 +------------------------
 arch/sparc64/mm/fault.c |   23 +----------------------
 arch/x86/mm/fault_64.c  |   23 ++---------------------
 include/linux/kprobes.h |   17 +++++++++++++++++
 7 files changed, 24 insertions(+), 134 deletions(-)

diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
index 6560cb1..e41953e 100644
--- a/arch/avr32/mm/fault.c
+++ b/arch/avr32/mm/fault.c
@@ -20,25 +20,6 @@
 #include <asm/tlb.h>
 #include <asm/uaccess.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		if (kprobe_running() && kprobe_fault_handler(regs, trap))
-			ret = 1;
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 int exception_trace = 1;
 
 /*
@@ -66,7 +47,7 @@ asmlinkage void do_page_fault(unsigned long ecr, struct pt_regs *regs)
 	int code;
 	int fault;
 
-	if (notify_page_fault(regs, ecr))
+	if (kprobe_handle_fault(regs, ecr))
 		return;
 
 	address = sysreg_read(TLBEAR);
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 7571076..bfc83e8 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -18,28 +18,6 @@
 
 extern void die (char *, struct pt_regs *, long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		/* kprobe_running() needs smp_processor_id() */
-		preempt_disable();
-		if (kprobe_running() && kprobes_fault_handler(regs, trap))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 /*
  * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
  * (inside region 5, on ia64) and that page is present.
@@ -106,7 +84,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	/*
 	 * This is to handle the kprobes on user space access instructions
 	 */
-	if (notify_page_fault(regs, TRAP_BRKPT))
+	if (kprobe_handle_fault(regs, TRAP_BRKPT))
 		return;
 
 	down_read(&mm->mmap_sem);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8135da0..ff64bd3 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -39,29 +39,6 @@
 #include <asm/tlbflush.h>
 #include <asm/siginfo.h>
 
-
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 11))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
@@ -164,7 +141,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 11))
 		return 0;
 
 	if (trap == 0x300) {
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2456b52..a9033cf 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
 
 extern void die(const char *,struct pt_regs *,long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	return 0;
-}
-#endif
-
-
 /*
  * Unlock any spinlocks which will prevent us from getting the
  * message out.
@@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
 	int si_code;
 	int fault;
 
-	if (notify_page_fault(regs, error_code))
+	if (kprobe_handle_fault(regs, 14))
 		return;
 
 	tsk = current;
diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
index e2027f2..8467dd6 100644
--- a/arch/sparc64/mm/fault.c
+++ b/arch/sparc64/mm/fault.c
@@ -31,27 +31,6 @@
 #include <asm/sections.h>
 #include <asm/mmu_context.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 0))
-			ret = 1;
-		preempt_enable();
-	}
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * To debug kernel to catch accesses to certain virtual/physical addresses.
  * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
@@ -280,7 +259,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 
 	fault_code = get_thread_fault_code();
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 0))
 		return;
 
 	si_code = SEGV_MAPERR;
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index f681ff8..f81660f 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -45,25 +45,6 @@
 #define PF_RSVD		(1<<3)
 #define PF_INSTR	(1<<4)
 
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-#ifdef CONFIG_KPROBES
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-#else
-	return 0;
-#endif
-}
-
 #ifdef CONFIG_X86_32
 /*
  * Return EIP plus the CS segment base.  The segment limit is also
@@ -468,7 +449,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 			if (vmalloc_fault(address) >= 0)
 				return;
 		}
-		if (notify_page_fault(regs))
+		if (kprobe_handle_fault(regs, 14))
 			return;
 		/*
 		 * Don't take the mm semaphore here. If we fixup a prefetch
@@ -477,7 +458,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 		goto bad_area_nosemaphore;
 	}
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 14))
 		return;
 
 	if (likely(regs->flags & X86_EFLAGS_IF))
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 6168c0a..e099426 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/hardirq.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>
@@ -212,6 +213,18 @@ static inline struct kprobe *kprobe_running(void)
 	return (__get_cpu_var(current_kprobe));
 }
 
+/*
+ * If it is a kprobe pagefault we can not be preemptible so return before
+ * calling kprobe_running() as it will assert on smp_processor_id if
+ * preemption is enabled.
+ */
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	if (!user_mode(regs) && !preemptible() && kprobe_running())
+		return kprobe_fault_handler(regs, trapnr);
+	return 0;
+}
+
 static inline void reset_current_kprobe(void)
 {
 	__get_cpu_var(current_kprobe) = NULL;
@@ -247,6 +260,10 @@ static inline struct kprobe *kprobe_running(void)
 {
 	return NULL;
 }
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
 static inline int register_kprobe(struct kprobe *p)
 {
 	return -ENOSYS;
-- 
1.5.4.rc2.1164.g6451




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv4] kprobes: Introduce kprobe_handle_fault()
  2008-01-09 22:01           ` [PATCHv4] " Harvey Harrison
@ 2008-01-09 23:16             ` Heiko Carstens
  2008-01-10  0:25               ` Harvey Harrison
                                 ` (2 more replies)
  2008-01-09 23:31             ` [PATCHv4] kprobes: Introduce kprobe_handle_fault() Masami Hiramatsu
  1 sibling, 3 replies; 24+ messages in thread
From: Heiko Carstens @ 2008-01-09 23:16 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Christoph Hellwig, Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras,
	David Wilder, jkenisto

>  arch/avr32/mm/fault.c   |   21 +--------------------
>  arch/ia64/mm/fault.c    |   24 +-----------------------
>  arch/powerpc/mm/fault.c |   25 +------------------------
>  arch/s390/mm/fault.c    |   25 +------------------------
>  arch/sparc64/mm/fault.c |   23 +----------------------
>  arch/x86/mm/fault_64.c  |   23 ++---------------------
>  include/linux/kprobes.h |   17 +++++++++++++++++
>  7 files changed, 24 insertions(+), 134 deletions(-)

Somehow I think you missed arch/x86/mm/fault_32.c :)

> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.

> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 2456b52..a9033cf 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
> 
>  extern void die(const char *,struct pt_regs *,long);
> 
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs, long err)
> -{
> -	int ret = 0;
> -
> -	/* kprobe_running() needs smp_processor_id() */
> -	if (!user_mode(regs)) {
> -		preempt_disable();
> -		if (kprobe_running() && kprobe_fault_handler(regs, 14))
> -			ret = 1;
> -		preempt_enable();
> -	}
> -
> -	return ret;
> -}
> -#else
> -static inline int notify_page_fault(struct pt_regs *regs, long err)
> -{
> -	return 0;
> -}
> -#endif
> -
> -
>  /*
>   * Unlock any spinlocks which will prevent us from getting the
>   * message out.
> @@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
>  	int si_code;
>  	int fault;
> 
> -	if (notify_page_fault(regs, error_code))
> +	if (kprobe_handle_fault(regs, 14))
>  		return;

Uhm.. yes. 14 is HFP-Significance exception. That doesn't make too much
sense. Passing error_code here would be the right thing to do.
Also I just checked with David Wilder: system tap itself doesn't have any
fault handlers. So it should be safe to change this.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv4] kprobes: Introduce kprobe_handle_fault()
  2008-01-09 22:01           ` [PATCHv4] " Harvey Harrison
  2008-01-09 23:16             ` Heiko Carstens
@ 2008-01-09 23:31             ` Masami Hiramatsu
  1 sibling, 0 replies; 24+ messages in thread
From: Masami Hiramatsu @ 2008-01-09 23:31 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Heiko Carstens, Christoph Hellwig, Andrew Morton, LKML,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras

Hi Harvey,

Harvey Harrison wrote:
> Use a central kprobe_handle_fault() inline in kprobes.h to remove
> all of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> This patch removes the preempt_disable/enable pair around kprobe_running
> which was originally added to avoid the assertion from smp_processor_id
> which would be unreliable if preemption was enabled.
> 
> Kprobes can not be running if we are preemptible, so test explicitly
> for preemption and bail out before hitting kprobe_running().
> 
> Style comments from Christoph Hellwig and Heiko Carstens included in
> this version.

Could you please separate this patch into 2 parts as Christoph mentioned?:
- introduce kprobe_handle_fault()
- remove preempt_disable/enable.

I agree with removing preempt_disable/enable, because kprobes premises
that it runs on same CPU while processing probing and single-stepping.
Actually, it uses per-cpu  to check the running kprobe's status.
Thus, if a fault occurs when preemption is enabled, we can ensure
that it is not caused by a kprobe handler.

> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
>  arch/avr32/mm/fault.c   |   21 +--------------------
>  arch/ia64/mm/fault.c    |   24 +-----------------------
>  arch/powerpc/mm/fault.c |   25 +------------------------
>  arch/s390/mm/fault.c    |   25 +------------------------
>  arch/sparc64/mm/fault.c |   23 +----------------------
>  arch/x86/mm/fault_64.c  |   23 ++---------------------
>  include/linux/kprobes.h |   17 +++++++++++++++++
>  7 files changed, 24 insertions(+), 134 deletions(-)
> 
> diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
> index 6560cb1..e41953e 100644
> --- a/arch/avr32/mm/fault.c
> +++ b/arch/avr32/mm/fault.c
> @@ -20,25 +20,6 @@
>  #include <asm/tlb.h>
>  #include <asm/uaccess.h>
>  
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs, int trap)
> -{
> -	int ret = 0;
> -
> -	if (!user_mode(regs)) {
> -		if (kprobe_running() && kprobe_fault_handler(regs, trap))
> -			ret = 1;
> -	}
> -
> -	return ret;
> -}
> -#else
> -static inline int notify_page_fault(struct pt_regs *regs, int trap)
> -{
> -	return 0;
> -}
> -#endif
> -
>  int exception_trace = 1;
>  
>  /*
> @@ -66,7 +47,7 @@ asmlinkage void do_page_fault(unsigned long ecr, struct pt_regs *regs)
>  	int code;
>  	int fault;
>  
> -	if (notify_page_fault(regs, ecr))
> +	if (kprobe_handle_fault(regs, ecr))
>  		return;
>  
>  	address = sysreg_read(TLBEAR);
> diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
> index 7571076..bfc83e8 100644
> --- a/arch/ia64/mm/fault.c
> +++ b/arch/ia64/mm/fault.c
> @@ -18,28 +18,6 @@
>  
>  extern void die (char *, struct pt_regs *, long);
>  
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs, int trap)
> -{
> -	int ret = 0;
> -
> -	if (!user_mode(regs)) {
> -		/* kprobe_running() needs smp_processor_id() */
> -		preempt_disable();
> -		if (kprobe_running() && kprobes_fault_handler(regs, trap))
> -			ret = 1;
> -		preempt_enable();
> -	}
> -
> -	return ret;
> -}
> -#else
> -static inline int notify_page_fault(struct pt_regs *regs, int trap)
> -{
> -	return 0;
> -}
> -#endif
> -
>  /*
>   * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
>   * (inside region 5, on ia64) and that page is present.
> @@ -106,7 +84,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
>  	/*
>  	 * This is to handle the kprobes on user space access instructions
>  	 */
> -	if (notify_page_fault(regs, TRAP_BRKPT))
> +	if (kprobe_handle_fault(regs, TRAP_BRKPT))
>  		return;
>  
>  	down_read(&mm->mmap_sem);
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 8135da0..ff64bd3 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -39,29 +39,6 @@
>  #include <asm/tlbflush.h>
>  #include <asm/siginfo.h>
>  
> -
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs)
> -{
> -	int ret = 0;
> -
> -	/* kprobe_running() needs smp_processor_id() */
> -	if (!user_mode(regs)) {
> -		preempt_disable();
> -		if (kprobe_running() && kprobe_fault_handler(regs, 11))
> -			ret = 1;
> -		preempt_enable();
> -	}
> -
> -	return ret;
> -}
> -#else
> -static inline int notify_page_fault(struct pt_regs *regs)
> -{
> -	return 0;
> -}
> -#endif
> -
>  /*
>   * Check whether the instruction at regs->nip is a store using
>   * an update addressing form which will update r1.
> @@ -164,7 +141,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  	is_write = error_code & ESR_DST;
>  #endif /* CONFIG_4xx || CONFIG_BOOKE */
>  
> -	if (notify_page_fault(regs))
> +	if (kprobe_handle_fault(regs, 11))
>  		return 0;
>  
>  	if (trap == 0x300) {
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 2456b52..a9033cf 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
>  
>  extern void die(const char *,struct pt_regs *,long);
>  
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs, long err)
> -{
> -	int ret = 0;
> -
> -	/* kprobe_running() needs smp_processor_id() */
> -	if (!user_mode(regs)) {
> -		preempt_disable();
> -		if (kprobe_running() && kprobe_fault_handler(regs, 14))
> -			ret = 1;
> -		preempt_enable();
> -	}
> -
> -	return ret;
> -}
> -#else
> -static inline int notify_page_fault(struct pt_regs *regs, long err)
> -{
> -	return 0;
> -}
> -#endif
> -
> -
>  /*
>   * Unlock any spinlocks which will prevent us from getting the
>   * message out.
> @@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
>  	int si_code;
>  	int fault;
>  
> -	if (notify_page_fault(regs, error_code))
> +	if (kprobe_handle_fault(regs, 14))
>  		return;
>  
>  	tsk = current;
> diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
> index e2027f2..8467dd6 100644
> --- a/arch/sparc64/mm/fault.c
> +++ b/arch/sparc64/mm/fault.c
> @@ -31,27 +31,6 @@
>  #include <asm/sections.h>
>  #include <asm/mmu_context.h>
>  
> -#ifdef CONFIG_KPROBES
> -static inline int notify_page_fault(struct pt_regs *regs)
> -{
> -	int ret = 0;
> -
> -	/* kprobe_running() needs smp_processor_id() */
> -	if (!user_mode(regs)) {
> -		preempt_disable();
> -		if (kprobe_running() && kprobe_fault_handler(regs, 0))
> -			ret = 1;
> -		preempt_enable();
> -	}
> -	return ret;
> -}
> -#else
> -static inline int notify_page_fault(struct pt_regs *regs)
> -{
> -	return 0;
> -}
> -#endif
> -
>  /*
>   * To debug kernel to catch accesses to certain virtual/physical addresses.
>   * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
> @@ -280,7 +259,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
>  
>  	fault_code = get_thread_fault_code();
>  
> -	if (notify_page_fault(regs))
> +	if (kprobe_handle_fault(regs, 0))
>  		return;
>  
>  	si_code = SEGV_MAPERR;
> diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
> index f681ff8..f81660f 100644
> --- a/arch/x86/mm/fault_64.c
> +++ b/arch/x86/mm/fault_64.c
> @@ -45,25 +45,6 @@
>  #define PF_RSVD		(1<<3)
>  #define PF_INSTR	(1<<4)
>  
> -static inline int notify_page_fault(struct pt_regs *regs)
> -{
> -#ifdef CONFIG_KPROBES
> -	int ret = 0;
> -
> -	/* kprobe_running() needs smp_processor_id() */
> -	if (!user_mode(regs)) {
> -		preempt_disable();
> -		if (kprobe_running() && kprobe_fault_handler(regs, 14))
> -			ret = 1;
> -		preempt_enable();
> -	}
> -
> -	return ret;
> -#else
> -	return 0;
> -#endif
> -}
> -
>  #ifdef CONFIG_X86_32
>  /*
>   * Return EIP plus the CS segment base.  The segment limit is also
> @@ -468,7 +449,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
>  			if (vmalloc_fault(address) >= 0)
>  				return;
>  		}
> -		if (notify_page_fault(regs))
> +		if (kprobe_handle_fault(regs, 14))
>  			return;
>  		/*
>  		 * Don't take the mm semaphore here. If we fixup a prefetch
> @@ -477,7 +458,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
>  		goto bad_area_nosemaphore;
>  	}
>  
> -	if (notify_page_fault(regs))
> +	if (kprobe_handle_fault(regs, 14))
>  		return;
>  
>  	if (likely(regs->flags & X86_EFLAGS_IF))
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 6168c0a..e099426 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -36,6 +36,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/rcupdate.h>
>  #include <linux/mutex.h>
> +#include <linux/hardirq.h>
>  
>  #ifdef CONFIG_KPROBES
>  #include <asm/kprobes.h>
> @@ -212,6 +213,18 @@ static inline struct kprobe *kprobe_running(void)
>  	return (__get_cpu_var(current_kprobe));
>  }
>  
> +/*
> + * If it is a kprobe pagefault we can not be preemptible so return before
> + * calling kprobe_running() as it will assert on smp_processor_id if
> + * preemption is enabled.
> + */
> +static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
> +{
> +	if (!user_mode(regs) && !preemptible() && kprobe_running())
> +		return kprobe_fault_handler(regs, trapnr);
> +	return 0;
> +}
> +
>  static inline void reset_current_kprobe(void)
>  {
>  	__get_cpu_var(current_kprobe) = NULL;
> @@ -247,6 +260,10 @@ static inline struct kprobe *kprobe_running(void)
>  {
>  	return NULL;
>  }
> +static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
> +{
> +	return 0;
> +}
>  static inline int register_kprobe(struct kprobe *p)
>  {
>  	return -ENOSYS;

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCHv4] kprobes: Introduce kprobe_handle_fault()
  2008-01-09 23:16             ` Heiko Carstens
@ 2008-01-10  0:25               ` Harvey Harrison
  2008-01-10  0:44               ` [PATCH 1/2] " Harvey Harrison
  2008-01-10  0:45               ` [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault() Harvey Harrison
  2 siblings, 0 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-01-10  0:25 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Christoph Hellwig, Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras,
	David Wilder, jkenisto

On Thu, 2008-01-10 at 00:16 +0100, Heiko Carstens wrote:
> >  arch/avr32/mm/fault.c   |   21 +--------------------
> >  arch/ia64/mm/fault.c    |   24 +-----------------------
> >  arch/powerpc/mm/fault.c |   25 +------------------------
> >  arch/s390/mm/fault.c    |   25 +------------------------
> >  arch/sparc64/mm/fault.c |   23 +----------------------
> >  arch/x86/mm/fault_64.c  |   23 ++---------------------
> >  include/linux/kprobes.h |   17 +++++++++++++++++
> >  7 files changed, 24 insertions(+), 134 deletions(-)
> 
> Somehow I think you missed arch/x86/mm/fault_32.c :)
> 

X86_32 _needs_ !user_mode_vm() as opposed to all of the others needing
!user_mode(), I was planning to deal with this in the unification of
fault_32|64.c at a later date.  So for now X86_32 will not use this.

> > This uncovered a possible bug in the s390 version as that purely
> > copied the x86 version unconditionally passing 14 as the trapnr
> > rather than the error_code parameter.

> 
> Uhm.. yes. 14 is HFP-Significance exception. That doesn't make too much
> sense. Passing error_code here would be the right thing to do.
> Also I just checked with David Wilder: system tap itself doesn't have any
> fault handlers. So it should be safe to change this.

OK, I will send a two patch series as per Christoph's request and will
include the s390 error_code fix instead of 14.

Cheers,

Harvey


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-01-09 23:16             ` Heiko Carstens
  2008-01-10  0:25               ` Harvey Harrison
@ 2008-01-10  0:44               ` Harvey Harrison
  2008-01-10  3:15                 ` Masami Hiramatsu
  2008-01-10 12:50                 ` Ingo Molnar
  2008-01-10  0:45               ` [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault() Harvey Harrison
  2 siblings, 2 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-01-10  0:44 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Christoph Hellwig, Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras,
	David Wilder, jkenisto

Use a central kprobe_handle_fault() inline in kprobes.h to remove
all of the arch-dependant, practically identical implementations in
avr32, ia64, powerpc, s390, sparc64, and x86.

avr32 was the only arch without the preempt_disable/enable pair
in its notify_page_fault implementation.

This uncovered a possible bug in the s390 version as that purely
copied the x86 version unconditionally passing 14 as the trapnr
rather than the error_code parameter.  s390 is changed to pass
error_code in this patch.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 arch/avr32/mm/fault.c   |   21 +--------------------
 arch/ia64/mm/fault.c    |   24 +-----------------------
 arch/powerpc/mm/fault.c |   25 +------------------------
 arch/s390/mm/fault.c    |   25 +------------------------
 arch/sparc64/mm/fault.c |   23 +----------------------
 arch/x86/mm/fault_64.c  |   26 ++------------------------
 include/linux/kprobes.h |   20 ++++++++++++++++++++
 7 files changed, 27 insertions(+), 137 deletions(-)

diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
index 6560cb1..e41953e 100644
--- a/arch/avr32/mm/fault.c
+++ b/arch/avr32/mm/fault.c
@@ -20,25 +20,6 @@
 #include <asm/tlb.h>
 #include <asm/uaccess.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		if (kprobe_running() && kprobe_fault_handler(regs, trap))
-			ret = 1;
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 int exception_trace = 1;
 
 /*
@@ -66,7 +47,7 @@ asmlinkage void do_page_fault(unsigned long ecr, struct pt_regs *regs)
 	int code;
 	int fault;
 
-	if (notify_page_fault(regs, ecr))
+	if (kprobe_handle_fault(regs, ecr))
 		return;
 
 	address = sysreg_read(TLBEAR);
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 7571076..bfc83e8 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -18,28 +18,6 @@
 
 extern void die (char *, struct pt_regs *, long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		/* kprobe_running() needs smp_processor_id() */
-		preempt_disable();
-		if (kprobe_running() && kprobes_fault_handler(regs, trap))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 /*
  * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
  * (inside region 5, on ia64) and that page is present.
@@ -106,7 +84,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	/*
 	 * This is to handle the kprobes on user space access instructions
 	 */
-	if (notify_page_fault(regs, TRAP_BRKPT))
+	if (kprobe_handle_fault(regs, TRAP_BRKPT))
 		return;
 
 	down_read(&mm->mmap_sem);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8135da0..ff64bd3 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -39,29 +39,6 @@
 #include <asm/tlbflush.h>
 #include <asm/siginfo.h>
 
-
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 11))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
@@ -164,7 +141,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 11))
 		return 0;
 
 	if (trap == 0x300) {
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2456b52..5a47295 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
 
 extern void die(const char *,struct pt_regs *,long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	return 0;
-}
-#endif
-
-
 /*
  * Unlock any spinlocks which will prevent us from getting the
  * message out.
@@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
 	int si_code;
 	int fault;
 
-	if (notify_page_fault(regs, error_code))
+	if (kprobe_handle_fault(regs, error_code))
 		return;
 
 	tsk = current;
diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
index e2027f2..8467dd6 100644
--- a/arch/sparc64/mm/fault.c
+++ b/arch/sparc64/mm/fault.c
@@ -31,27 +31,6 @@
 #include <asm/sections.h>
 #include <asm/mmu_context.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 0))
-			ret = 1;
-		preempt_enable();
-	}
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * To debug kernel to catch accesses to certain virtual/physical addresses.
  * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
@@ -280,7 +259,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 
 	fault_code = get_thread_fault_code();
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 0))
 		return;
 
 	si_code = SEGV_MAPERR;
diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c
index 0e26230..c29e462 100644
--- a/arch/x86/mm/fault_64.c
+++ b/arch/x86/mm/fault_64.c
@@ -41,28 +41,6 @@
 #define PF_RSVD	(1<<3)
 #define PF_INSTR	(1<<4)
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /* Sometimes the CPU reports invalid exceptions on prefetch.
    Check that here and ignore.
    Opcode checker based on code by Richard Brunner */
@@ -343,7 +321,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 			if (vmalloc_fault(address) >= 0)
 				return;
 		}
-		if (notify_page_fault(regs))
+		if (kprobe_handle_fault(regs, 14))
 			return;
 		/*
 		 * Don't take the mm semaphore here. If we fixup a prefetch
@@ -352,7 +330,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 		goto bad_area_nosemaphore;
 	}
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 14))
 		return;
 
 	if (likely(regs->eflags & X86_EFLAGS_IF))
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 8189158..c01e320 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/hardirq.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>
@@ -203,6 +204,21 @@ static inline struct kprobe *kprobe_running(void)
 	return (__get_cpu_var(current_kprobe));
 }
 
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	int ret = 0;
+
+	/* kprobe_running() needs smp_processor_id() */
+	if (!user_mode(regs)) {
+		preempt_disable();
+		if (kprobe_running() && kprobe_fault_handler(regs, trapnr))
+			ret = 1;
+		preempt_enable();
+	}
+
+	return ret;
+}
+
 static inline void reset_current_kprobe(void)
 {
 	__get_cpu_var(current_kprobe) = NULL;
@@ -237,6 +253,10 @@ static inline struct kprobe *kprobe_running(void)
 {
 	return NULL;
 }
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
 static inline int register_kprobe(struct kprobe *p)
 {
 	return -ENOSYS;
-- 
1.5.4.rc2.1164.g6451



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault()
  2008-01-09 23:16             ` Heiko Carstens
  2008-01-10  0:25               ` Harvey Harrison
  2008-01-10  0:44               ` [PATCH 1/2] " Harvey Harrison
@ 2008-01-10  0:45               ` Harvey Harrison
  2008-01-10  3:15                 ` Masami Hiramatsu
  2 siblings, 1 reply; 24+ messages in thread
From: Harvey Harrison @ 2008-01-10  0:45 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Christoph Hellwig, Andrew Morton, LKML, Masami Hiramatsu,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras,
	David Wilder, jkenisto

This patch removes the preempt_disable/enable pair around kprobe_running
which was originally added to avoid the assertion from smp_processor_id
which would be hit an asertion if preemption was enabled.

Kprobes can not be running if we are preemptible, so test explicitly
for preemption and bail out before hitting kprobe_running().

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/kprobes.h |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index c01e320..e61607a 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -204,19 +204,16 @@ static inline struct kprobe *kprobe_running(void)
 	return (__get_cpu_var(current_kprobe));
 }
 
+/*
+ * If it is a kprobe pagefault we can not be preemptible so return before
+ * calling kprobe_running() as it will assert on smp_processor_id if
+ * preemption is enabled.
+ */
 static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
 {
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, trapnr))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
+	if (!user_mode(regs) && !preemptible() && kprobe_running())
+		return kprobe_fault_handler(regs, trapnr);
+	return 0;
 }
 
 static inline void reset_current_kprobe(void)
-- 
1.5.4.rc2.1164.g6451


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-01-10  0:44               ` [PATCH 1/2] " Harvey Harrison
@ 2008-01-10  3:15                 ` Masami Hiramatsu
  2008-01-10 12:50                 ` Ingo Molnar
  1 sibling, 0 replies; 24+ messages in thread
From: Masami Hiramatsu @ 2008-01-10  3:15 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Heiko Carstens, Christoph Hellwig, Andrew Morton, LKML,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras,
	David Wilder, jkenisto

Harvey Harrison wrote:
> Use a central kprobe_handle_fault() inline in kprobes.h to remove
> all of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.  s390 is changed to pass
> error_code in this patch.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

I tested it on x86-64.

Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

Thank you!

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault()
  2008-01-10  0:45               ` [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault() Harvey Harrison
@ 2008-01-10  3:15                 ` Masami Hiramatsu
  0 siblings, 0 replies; 24+ messages in thread
From: Masami Hiramatsu @ 2008-01-10  3:15 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Heiko Carstens, Christoph Hellwig, Andrew Morton, LKML,
	Ananth N Mavinakayanahalli, David Miller, hskinnemoen,
	schwidefsky, tony.luck, Ingo Molnar, Paul Mackerras,
	David Wilder, jkenisto

Harvey Harrison wrote:
> This patch removes the preempt_disable/enable pair around kprobe_running
> which was originally added to avoid the assertion from smp_processor_id
> which would be hit an asertion if preemption was enabled.
> 
> Kprobes can not be running if we are preemptible, so test explicitly
> for preemption and bail out before hitting kprobe_running().
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

Tested on x86-64.

Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

Thank you!

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-01-10  0:44               ` [PATCH 1/2] " Harvey Harrison
  2008-01-10  3:15                 ` Masami Hiramatsu
@ 2008-01-10 12:50                 ` Ingo Molnar
  1 sibling, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2008-01-10 12:50 UTC (permalink / raw)
  To: Harvey Harrison
  Cc: Heiko Carstens, Christoph Hellwig, Andrew Morton, LKML,
	Masami Hiramatsu, Ananth N Mavinakayanahalli, David Miller,
	hskinnemoen, schwidefsky, tony.luck, Paul Mackerras,
	David Wilder, jkenisto


* Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Use a central kprobe_handle_fault() inline in kprobes.h to remove all 
> of the arch-dependant, practically identical implementations in avr32, 
> ia64, powerpc, s390, sparc64, and x86.

i guess this should be done via -mm, because it changes all 
architectures.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-02-29 11:16 ` Andrew Morton
@ 2008-02-29 16:30   ` Harvey Harrison
  0 siblings, 0 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-02-29 16:30 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-arch, LKML

On Fri, Feb 29, 2008 at 3:16 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sat, 02 Feb 2008 19:00:23 -0800 Harvey Harrison <harvey.harrison@gmail.com> wrote:
>
>  > Use a central kprobe_handle_fault() inline in kprobes.h to remove
>  > all of the arch-dependant, practically identical implementations in
>  > avr32, ia64, powerpc, s390, sparc64, and x86.
>  >
>  > avr32 was the only arch without the preempt_disable/enable pair
>  > in its notify_page_fault implementation.
>  >
>  > This uncovered a possible bug in the s390 version as that purely
>  > copied the x86 version unconditionally passing 14 as the trapnr
>  > rather than the error_code parameter.  s390 is changed to pass
>  > error_code in this patch.
>  >
>  > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>  > ---
>  > Andrew, 1/2 is low risk, 2/2 has been tested on x86-32 and 64,
>  > but the !preemptible invariant seems correct.
>  >
>  >  arch/arm/mm/fault.c     |   25 +------------------------
>  >  arch/avr32/mm/fault.c   |   21 +--------------------
>  >  arch/ia64/mm/fault.c    |   24 +-----------------------
>  >  arch/powerpc/mm/fault.c |   25 +------------------------
>  >  arch/s390/mm/fault.c    |   25 +------------------------
>  >  arch/sparc64/mm/fault.c |   23 +----------------------
>  >  arch/x86/mm/fault.c     |   26 ++------------------------
>  >  include/linux/kprobes.h |   20 ++++++++++++++++++++
>  >  8 files changed, 28 insertions(+), 161 deletions(-)
>
>  Breaks the ia64 build:
>
>  include/linux/kprobes.h: In function `kprobe_handle_fault':
>  include/linux/kprobes.h:234: error: implicit declaration of function `kprobe_fault_handler'
>
>  and ia64 doesn't implement kprobe_fault_handler() so there's no easy fix
>  here.

Yes there is, ia64 apparently called theirs kprobes_handle_fault, sorry I didn't
notice the extra 's'.  Want a new patch, or will you change the three
occurances?

Harvey

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-02-03  3:00 [PATCH 1/2] " Harvey Harrison
  2008-02-03 11:52 ` Heiko Carstens
  2008-02-07 19:12 ` Masami Hiramatsu
@ 2008-02-29 11:16 ` Andrew Morton
  2008-02-29 16:30   ` Harvey Harrison
  2 siblings, 1 reply; 24+ messages in thread
From: Andrew Morton @ 2008-02-29 11:16 UTC (permalink / raw)
  To: Harvey Harrison; +Cc: linux-arch, LKML

On Sat, 02 Feb 2008 19:00:23 -0800 Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Use a central kprobe_handle_fault() inline in kprobes.h to remove
> all of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.  s390 is changed to pass
> error_code in this patch.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> Andrew, 1/2 is low risk, 2/2 has been tested on x86-32 and 64,
> but the !preemptible invariant seems correct.
> 
>  arch/arm/mm/fault.c     |   25 +------------------------
>  arch/avr32/mm/fault.c   |   21 +--------------------
>  arch/ia64/mm/fault.c    |   24 +-----------------------
>  arch/powerpc/mm/fault.c |   25 +------------------------
>  arch/s390/mm/fault.c    |   25 +------------------------
>  arch/sparc64/mm/fault.c |   23 +----------------------
>  arch/x86/mm/fault.c     |   26 ++------------------------
>  include/linux/kprobes.h |   20 ++++++++++++++++++++
>  8 files changed, 28 insertions(+), 161 deletions(-)

Breaks the ia64 build:

include/linux/kprobes.h: In function `kprobe_handle_fault':
include/linux/kprobes.h:234: error: implicit declaration of function `kprobe_fault_handler'                                                                     

and ia64 doesn't implement kprobe_fault_handler() so there's no easy fix
here.

There's too much inlining in linux/kprobes.h, btw.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-02-03  3:00 [PATCH 1/2] " Harvey Harrison
  2008-02-03 11:52 ` Heiko Carstens
@ 2008-02-07 19:12 ` Masami Hiramatsu
  2008-02-29 11:16 ` Andrew Morton
  2 siblings, 0 replies; 24+ messages in thread
From: Masami Hiramatsu @ 2008-02-07 19:12 UTC (permalink / raw)
  To: Harvey Harrison; +Cc: Andrew Morton, linux-arch, LKML

Harvey Harrison wrote:
> Use a central kprobe_handle_fault() inline in kprobes.h to remove
> all of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.  s390 is changed to pass
> error_code in this patch.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

Tested on i386/x86-64.

Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

-- 
Masami Hiramatsu

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

e-mail: mhiramat@redhat.com


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
  2008-02-03  3:00 [PATCH 1/2] " Harvey Harrison
@ 2008-02-03 11:52 ` Heiko Carstens
  2008-02-07 19:12 ` Masami Hiramatsu
  2008-02-29 11:16 ` Andrew Morton
  2 siblings, 0 replies; 24+ messages in thread
From: Heiko Carstens @ 2008-02-03 11:52 UTC (permalink / raw)
  To: Harvey Harrison; +Cc: Andrew Morton, linux-arch, LKML

On Sat, Feb 02, 2008 at 07:00:23PM -0800, Harvey Harrison wrote:
> Use a central kprobe_handle_fault() inline in kprobes.h to remove
> all of the arch-dependant, practically identical implementations in
> avr32, ia64, powerpc, s390, sparc64, and x86.
> 
> avr32 was the only arch without the preempt_disable/enable pair
> in its notify_page_fault implementation.
> 
> This uncovered a possible bug in the s390 version as that purely
> copied the x86 version unconditionally passing 14 as the trapnr
> rather than the error_code parameter.  s390 is changed to pass
> error_code in this patch.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

for the s390 bits:

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
@ 2008-02-03  3:00 Harvey Harrison
  2008-02-03 11:52 ` Heiko Carstens
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Harvey Harrison @ 2008-02-03  3:00 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-arch, LKML

Use a central kprobe_handle_fault() inline in kprobes.h to remove
all of the arch-dependant, practically identical implementations in
avr32, ia64, powerpc, s390, sparc64, and x86.

avr32 was the only arch without the preempt_disable/enable pair
in its notify_page_fault implementation.

This uncovered a possible bug in the s390 version as that purely
copied the x86 version unconditionally passing 14 as the trapnr
rather than the error_code parameter.  s390 is changed to pass
error_code in this patch.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Andrew, 1/2 is low risk, 2/2 has been tested on x86-32 and 64,
but the !preemptible invariant seems correct.

 arch/arm/mm/fault.c     |   25 +------------------------
 arch/avr32/mm/fault.c   |   21 +--------------------
 arch/ia64/mm/fault.c    |   24 +-----------------------
 arch/powerpc/mm/fault.c |   25 +------------------------
 arch/s390/mm/fault.c    |   25 +------------------------
 arch/sparc64/mm/fault.c |   23 +----------------------
 arch/x86/mm/fault.c     |   26 ++------------------------
 include/linux/kprobes.h |   20 ++++++++++++++++++++
 8 files changed, 28 insertions(+), 161 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 28ad7ab..5def64f 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -21,29 +21,6 @@
 
 #include "fault.h"
 
-
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, unsigned int fsr)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		/* kprobe_running() needs smp_processor_id() */
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, fsr))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, unsigned int fsr)
-{
-	return 0;
-}
-#endif
-
 /*
  * This is useful to dump out the page tables associated with
  * 'addr' in mm 'mm'.
@@ -246,7 +223,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	struct mm_struct *mm;
 	int fault, sig, code;
 
-	if (notify_page_fault(regs, fsr))
+	if (handle_kprobe_fault(regs, fsr))
 		return 0;
 
 	tsk = current;
diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
index 6560cb1..e41953e 100644
--- a/arch/avr32/mm/fault.c
+++ b/arch/avr32/mm/fault.c
@@ -20,25 +20,6 @@
 #include <asm/tlb.h>
 #include <asm/uaccess.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		if (kprobe_running() && kprobe_fault_handler(regs, trap))
-			ret = 1;
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 int exception_trace = 1;
 
 /*
@@ -66,7 +47,7 @@ asmlinkage void do_page_fault(unsigned long ecr, struct pt_regs *regs)
 	int code;
 	int fault;
 
-	if (notify_page_fault(regs, ecr))
+	if (kprobe_handle_fault(regs, ecr))
 		return;
 
 	address = sysreg_read(TLBEAR);
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 7571076..bfc83e8 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -18,28 +18,6 @@
 
 extern void die (char *, struct pt_regs *, long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	int ret = 0;
-
-	if (!user_mode(regs)) {
-		/* kprobe_running() needs smp_processor_id() */
-		preempt_disable();
-		if (kprobe_running() && kprobes_fault_handler(regs, trap))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, int trap)
-{
-	return 0;
-}
-#endif
-
 /*
  * Return TRUE if ADDRESS points at a page in the kernel's mapped segment
  * (inside region 5, on ia64) and that page is present.
@@ -106,7 +84,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	/*
 	 * This is to handle the kprobes on user space access instructions
 	 */
-	if (notify_page_fault(regs, TRAP_BRKPT))
+	if (kprobe_handle_fault(regs, TRAP_BRKPT))
 		return;
 
 	down_read(&mm->mmap_sem);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 7b25107..3246953 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -39,29 +39,6 @@
 #include <asm/tlbflush.h>
 #include <asm/siginfo.h>
 
-
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 11))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * Check whether the instruction at regs->nip is a store using
  * an update addressing form which will update r1.
@@ -164,7 +141,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 11))
 		return 0;
 
 	if (unlikely(debugger_fault_handler(regs)))
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 2456b52..5a47295 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -51,29 +51,6 @@ extern int sysctl_userprocess_debug;
 
 extern void die(const char *,struct pt_regs *,long);
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs, long err)
-{
-	return 0;
-}
-#endif
-
-
 /*
  * Unlock any spinlocks which will prevent us from getting the
  * message out.
@@ -309,7 +286,7 @@ do_exception(struct pt_regs *regs, unsigned long error_code, int write)
 	int si_code;
 	int fault;
 
-	if (notify_page_fault(regs, error_code))
+	if (kprobe_handle_fault(regs, error_code))
 		return;
 
 	tsk = current;
diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
index e2027f2..8467dd6 100644
--- a/arch/sparc64/mm/fault.c
+++ b/arch/sparc64/mm/fault.c
@@ -31,27 +31,6 @@
 #include <asm/sections.h>
 #include <asm/mmu_context.h>
 
-#ifdef CONFIG_KPROBES
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-	if (!user_mode(regs)) {
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 0))
-			ret = 1;
-		preempt_enable();
-	}
-	return ret;
-}
-#else
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-	return 0;
-}
-#endif
-
 /*
  * To debug kernel to catch accesses to certain virtual/physical addresses.
  * Mode = 0 selects physical watchpoints, mode = 1 selects virtual watchpoints.
@@ -280,7 +259,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 
 	fault_code = get_thread_fault_code();
 
-	if (notify_page_fault(regs))
+	if (kprobe_handle_fault(regs, 0))
 		return;
 
 	si_code = SEGV_MAPERR;
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 3fff490..f0adbb0 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -49,29 +49,6 @@
 #define PF_RSVD		(1<<3)
 #define PF_INSTR	(1<<4)
 
-static inline int notify_page_fault(struct pt_regs *regs)
-{
-#ifdef CONFIG_KPROBES
-	int ret = 0;
-
-	/* kprobe_running() needs smp_processor_id() */
-#ifdef CONFIG_X86_32
-	if (!user_mode_vm(regs)) {
-#else
-	if (!user_mode(regs)) {
-#endif
-		preempt_disable();
-		if (kprobe_running() && kprobe_fault_handler(regs, 14))
-			ret = 1;
-		preempt_enable();
-	}
-
-	return ret;
-#else
-	return 0;
-#endif
-}
-
 /*
  * X86_32
  * Sometimes AMD Athlon/Opteron CPUs report invalid exceptions on prefetch.
@@ -589,7 +566,8 @@ void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
 
 	si_code = SEGV_MAPERR;
 
-	if (notify_page_fault(regs))
+	/* Must not try to handle kprobes in v8086 mode */
+	if (!v8086_mode(regs) && kprobe_handle_fault(regs, 14))
 		return;
 
 	/*
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 6168c0a..e5ecb1e 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/mutex.h>
+#include <linux/hardirq.h>
 
 #ifdef CONFIG_KPROBES
 #include <asm/kprobes.h>
@@ -212,6 +213,21 @@ static inline struct kprobe *kprobe_running(void)
 	return (__get_cpu_var(current_kprobe));
 }
 
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	int ret = 0;
+
+	/* kprobe_running() needs smp_processor_id() */
+	if (!user_mode(regs)) {
+		preempt_disable();
+		if (kprobe_running() && kprobe_fault_handler(regs, trapnr))
+			ret = 1;
+		preempt_enable();
+	}
+
+	return ret;
+}
+
 static inline void reset_current_kprobe(void)
 {
 	__get_cpu_var(current_kprobe) = NULL;
@@ -247,6 +263,10 @@ static inline struct kprobe *kprobe_running(void)
 {
 	return NULL;
 }
+static inline int kprobe_handle_fault(struct pt_regs *regs, int trapnr)
+{
+	return 0;
+}
 static inline int register_kprobe(struct kprobe *p)
 {
 	return -ENOSYS;
-- 
1.5.4.rc4.1142.gf5a97



^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2008-02-29 16:30 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-07 20:24 [PATCHv2] kprobes: Introduce is_kprobe_fault() Harvey Harrison
2008-01-08  5:37 ` Ananth N Mavinakayanahalli
2008-01-08 17:03 ` Masami Hiramatsu
2008-01-08 22:45 ` Paul Mackerras
2008-01-08 23:02   ` Harvey Harrison
2008-01-09  4:19     ` [PATCHv3] kprobes: Introduce kprobe_handle_fault() Harvey Harrison
2008-01-09  6:14       ` Heiko Carstens
2008-01-09  6:22         ` Harvey Harrison
2008-01-09 22:01           ` [PATCHv4] " Harvey Harrison
2008-01-09 23:16             ` Heiko Carstens
2008-01-10  0:25               ` Harvey Harrison
2008-01-10  0:44               ` [PATCH 1/2] " Harvey Harrison
2008-01-10  3:15                 ` Masami Hiramatsu
2008-01-10 12:50                 ` Ingo Molnar
2008-01-10  0:45               ` [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault() Harvey Harrison
2008-01-10  3:15                 ` Masami Hiramatsu
2008-01-09 23:31             ` [PATCHv4] kprobes: Introduce kprobe_handle_fault() Masami Hiramatsu
2008-01-09  7:58         ` [PATCHv3] " Christoph Hellwig
2008-01-09  7:57       ` Christoph Hellwig
2008-02-03  3:00 [PATCH 1/2] " Harvey Harrison
2008-02-03 11:52 ` Heiko Carstens
2008-02-07 19:12 ` Masami Hiramatsu
2008-02-29 11:16 ` Andrew Morton
2008-02-29 16:30   ` Harvey Harrison

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).