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 <[email protected]>
---
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
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 <[email protected]>
Tested on powerpc.
Tested-by: Ananth N Mavinakayanahalli <[email protected]>
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 <[email protected]>
I tested it on x86-64.
Acked-by: Masami Hiramatsu <[email protected]>
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected], [email protected]
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.
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
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 <[email protected]>
X86-64
Acked-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Harvey Harrison <[email protected]>
---
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
> +/*
> + * 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;
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 <[email protected]>
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
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.
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.
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 <[email protected]>
---
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
> 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.
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 <[email protected]>
> ---
> 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: [email protected], [email protected]
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Tested on x86-64.
Acked-by: Masami Hiramatsu <[email protected]>
Thank you!
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected], [email protected]
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 <[email protected]>
I tested it on x86-64.
Acked-by: Masami Hiramatsu <[email protected]>
Thank you!
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [email protected], [email protected]
* Harvey Harrison <[email protected]> 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 <[email protected]>
Ingo