kprobes: Introduce is_kprobe_fault()
PATCH v2 1/1 kprobes: Introduce is_kprobe_fault()
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.
---
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(-)
arch/avr32/mm/fault.c +1 -20
@@ -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 );
arch/ia64/mm/fault.c +1 -23
@@ -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);
arch/powerpc/mm/fault.c +1 -24
@@ -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 ) {
arch/s390/mm/fault.c +1 -24
@@ -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;
arch/sparc64/mm/fault.c +1 -22
@@ -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 ;
arch/x86/mm/fault_64.c +2 -24
@@ -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 ))
include/linux/kprobes.h +19 -0
@@ -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 ;
Signature · 1 line
1.5.4.rc2.1117.ge5809
Quoted 4 lines · Harvey Harrison
Use a central
is_kprobe_fault()inline inkprobes.hto remove all of the arch-dependant, practically identical implementations in avr32, ia64, powerpc, s390, sparc64, and x86.
Quoted 8 lines
avr32 was the only arch without the preempt_disable/enable pair in its
notify_page_faultimplementation.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_codeparameter.Signed-off-by Harvey Harrison <[email protected]>
Tested on powerpc.
Hi,
Thank you for good work!
Quoted 4 lines · Harvey Harrison
Use a central
is_kprobe_fault()inline inkprobes.hto remove all of the arch-dependant, practically identical implementations in avr32, ia64, powerpc, s390, sparc64, and x86.
Quoted 8 lines
avr32 was the only arch without the preempt_disable/enable pair in its
notify_page_faultimplementation.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_codeparameter.Signed-off-by Harvey Harrison <[email protected]>
I tested it on x86-64.
Signature · 7 lines
Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [email protected], [email protected]
Quoted 3 lines · Harvey Harrison
Use a central
is_kprobe_fault()inline inkprobes.hto 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.
Quoted 11 lines · Paul Mackerras
Quoted 3 lines · Harvey Harrison
Use a central
is_kprobe_fault()inline inkprobes.hto 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
PATCH v3 1/1 kprobes: Introduce kprobe_handle_fault()
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:
X86-64
--- 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(-)
arch/avr32/mm/fault.c +1 -20
@@ -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 );
arch/ia64/mm/fault.c +1 -23
@@ -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);
arch/powerpc/mm/fault.c +1 -24
@@ -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 ) {
arch/s390/mm/fault.c +1 -24
@@ -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;
arch/sparc64/mm/fault.c +1 -22
@@ -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 ;
arch/x86/mm/fault_64.c +2 -24
@@ -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 ))
include/linux/kprobes.h +19 -0
@@ -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 ;
Signature · 1 line
1.5.4.rc2.1164.g6451
Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
Quoted 2 lines
+/*
+ * 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.
include/linux/kprobes.h static inline struct kprobe *kprobe_running(void) · line 218
Quoted 10 lines
+ * calling
kprobe_running()as it will assert onsmp_processor_idif
+ * preemption is enabled.
+ */include/linux/kprobes.h +7
-0+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;
Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
Quoted 4 lines · Heiko Carstens
Quoted 2 lines
+/*
+ * If it is a kprobe pagefault we can not be premptible so return beforeMissing 'e' in preemptible.
OK.
Quoted 8 lines
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.
Quoted 5 lines
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 callsmp_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
PATCH v4 1/1 kprobes: Introduce kprobe_handle_fault()
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.
--- 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(-)
arch/avr32/mm/fault.c +1 -20
@@ -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 );
arch/ia64/mm/fault.c +1 -23
@@ -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);
arch/powerpc/mm/fault.c +1 -24
@@ -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 ) {
arch/s390/mm/fault.c +1 -24
@@ -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;
arch/sparc64/mm/fault.c +1 -22
@@ -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 ;
arch/x86/mm/fault_64.c +2 -21
@@ -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 ))
include/linux/kprobes.h +17 -0
@@ -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 ;
Signature · 1 line
1.5.4.rc2.1164.g6451
Re: [PATCHv4] kprobes: Introduce kprobe_handle_fault()
Quoted 8 lines
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 :)
Quoted 39 lines
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_codeparameter.arch/s390/mm/fault.c +0
-23@@ -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.
arch/s390/mm/fault.c do_exception(struct pt_regs *regs, unsigned long error_code, int write) · line 290
Quoted 6 lines
arch/s390/mm/fault.c +1
-1@@ -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.
Re: [PATCHv4] kprobes: Introduce kprobe_handle_fault()
Quoted 11 lines · Heiko Carstens
Quoted 8 lines
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.
Quoted 9 lines
Quoted 3 lines
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_codeparameter.Uhm.. yes. 14 is HFP-Significance exception. That doesn't make too much sense. Passing
error_codehere 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
PATCH 1/2 kprobes: Introduce kprobe_handle_fault()
In reply to Heiko CarstensUse 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.
--- 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(-)
arch/avr32/mm/fault.c +1 -20
@@ -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 );
arch/ia64/mm/fault.c +1 -23
@@ -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);
arch/powerpc/mm/fault.c +1 -24
@@ -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 ) {
arch/s390/mm/fault.c +1 -24
@@ -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;
arch/sparc64/mm/fault.c +1 -22
@@ -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 ;
arch/x86/mm/fault_64.c +2 -24
@@ -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 ))
include/linux/kprobes.h +20 -0
@@ -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 ;
Signature · 1 line
1.5.4.rc2.1164.g6451
Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
Quoted 7 lines · Harvey Harrison
Use a central
kprobe_handle_fault()inline inkprobes.hto 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_faultimplementation.
Quoted 6 lines
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_codeparameter. s390 is changed to passerror_codein this patch.Signed-off-by Harvey Harrison <[email protected]>
I tested it on x86-64.
Thank you!
Signature · 7 lines
Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [email protected], [email protected]
Re: [PATCH 1/2] kprobes: Introduce kprobe_handle_fault()
In reply to Harvey HarrisonQuoted 3 lines · Harvey Harrison
Use a central
kprobe_handle_fault()inline inkprobes.hto 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.
Ingo
PATCH 2/2 kprobe: remove preempt_enable/disable from kprobe_handle_fault()
In reply to Heiko CarstensThis 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().
--- include/linux/kprobes.h | 19 ++++++++----------- 1 files changed, 8 insertions(+), 11 deletions(-)
include/linux/kprobes.h +8 -11
@@ -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 )
Signature · 1 line
1.5.4.rc2.1164.g6451
Re: [PATCH 2/2] kprobe: remove preempt_enable/disable from kprobe_handle_fault()
Quoted 8 lines · Harvey Harrison
This patch removes the preempt_disable/enable pair around
kprobe_runningwhich was originally added to avoid the assertion fromsmp_processor_idwhich 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.
Thank you!
Signature · 7 lines
Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [email protected], [email protected]
Re: [PATCHv4] kprobes: Introduce kprobe_handle_fault()
In reply to Harvey HarrisonHi Harvey,
Quoted 8 lines · Harvey Harrison
Use a central
kprobe_handle_fault()inline inkprobes.hto 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_runningwhich was originally added to avoid the assertion fromsmp_processor_idwhich would be unreliable if preemption was enabled.
Quoted 5 lines
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.
Quoted 317 lines
avr32 was the only arch without the preempt_disable/enable pair in its
notify_page_faultimplementation.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_codeparameter.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(-)arch/avr32/mm/fault.c +1
-20@@ -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 );arch/ia64/mm/fault.c +1
-23@@ -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);arch/powerpc/mm/fault.c +1
-24@@ -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 ) {arch/s390/mm/fault.c +1
-24@@ -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;arch/sparc64/mm/fault.c +1
-22@@ -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 ;arch/x86/mm/fault_64.c +2
-21@@ -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 ))include/linux/kprobes.h +17
-0@@ -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 ;
Signature · 7 lines
Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [email protected], [email protected]
Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
In reply to Heiko CarstensQuoted 4 lines · Heiko Carstens
I like this better (not including any other changes):
if (!user_mode(regs) && !preemptible() && kprobe_running()) return kprobe_fault_handler(regs, trapnr);
seconded.
Re: [PATCHv3] kprobes: Introduce kprobe_handle_fault()
In reply to Harvey HarrisonQuoted 10 lines · Harvey Harrison
Use a central
kprobe_handle_fault()inline inkprobes.hto 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_faultimplementation.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_codeparameter.
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.