Re: [PATCH] x86: Use is_kprobe_fault to better match usage
* Harvey Harrison <[EMAIL PROTECTED]> wrote: > Currently the notify_page_fault helper is used to test it the page > fault was caused by a kprobe causing an early return from > do_page_fault. > > Change the name of the helper to is_kprobe_fault to match the usage > and remove the preempt_disable/enable pair around kprobe_running() > with an explicit test for preemption. The idea for this comes from a > patch by Quentin Barnes to kprobes.c > > Signed-off-by: Harvey Harrison <[EMAIL PROTECTED]> hm, this doesnt apply: Hunk #3 FAILED at 426. Hunk #4 succeeded at 413 (offset -22 lines). 1 out of 4 hunks FAILED -- rejects in file arch/x86/mm/fault_32.c patching file arch/x86/mm/fault_64.c Hunk #3 FAILED at 476. Hunk #4 succeeded at 475 (offset -10 lines). 1 out of 4 hunks FAILED -- rejects in file arch/x86/mm/fault_64.c could you double-check x86.git#mm, perhaps we are out of sync with a patch somewhere? Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
* Harvey Harrison [EMAIL PROTECTED] wrote: Currently the notify_page_fault helper is used to test it the page fault was caused by a kprobe causing an early return from do_page_fault. Change the name of the helper to is_kprobe_fault to match the usage and remove the preempt_disable/enable pair around kprobe_running() with an explicit test for preemption. The idea for this comes from a patch by Quentin Barnes to kprobes.c Signed-off-by: Harvey Harrison [EMAIL PROTECTED] hm, this doesnt apply: Hunk #3 FAILED at 426. Hunk #4 succeeded at 413 (offset -22 lines). 1 out of 4 hunks FAILED -- rejects in file arch/x86/mm/fault_32.c patching file arch/x86/mm/fault_64.c Hunk #3 FAILED at 476. Hunk #4 succeeded at 475 (offset -10 lines). 1 out of 4 hunks FAILED -- rejects in file arch/x86/mm/fault_64.c could you double-check x86.git#mm, perhaps we are out of sync with a patch somewhere? Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
Harvey Harrison wrote: > On Wed, 2008-01-02 at 21:36 -0500, Masami Hiramatsu wrote: >> Hi Harvey, >> >> Harvey Harrison wrote: >>> Currently the notify_page_fault helper is used to test it the page >>> fault was caused by a kprobe causing an early return from do_page_fault. >>> >>> Change the name of the helper to is_kprobe_fault to match the usage and >>> remove the preempt_disable/enable pair around kprobe_running() with an >>> explicit test for preemption. The idea for this comes from a patch >>> by Quentin Barnes to kprobes.c >> Sure, that's right. >> However, since other architectures also have notify_page_fault(), >> I think all of those code might better be changed same time for >> maintainability. >> > > How about a static inline in linux/kprobes.h with a big comment above > about when/why the !preemptible() check is sufficient? Hmm, fault handling depends on the architecture. But current notify_page_fault()s are very similar. I think unifying it is good idea. We will be happy to review that if you send it. Many thanks! > > Harvey > > > -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [EMAIL PROTECTED], [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
On Wed, 2008-01-02 at 21:36 -0500, Masami Hiramatsu wrote: > Hi Harvey, > > Harvey Harrison wrote: > > Currently the notify_page_fault helper is used to test it the page > > fault was caused by a kprobe causing an early return from do_page_fault. > > > > Change the name of the helper to is_kprobe_fault to match the usage and > > remove the preempt_disable/enable pair around kprobe_running() with an > > explicit test for preemption. The idea for this comes from a patch > > by Quentin Barnes to kprobes.c > > Sure, that's right. > However, since other architectures also have notify_page_fault(), > I think all of those code might better be changed same time for > maintainability. > How about a static inline in linux/kprobes.h with a big comment above about when/why the !preemptible() check is sufficient? Harvey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
Hi Harvey, Harvey Harrison wrote: > Currently the notify_page_fault helper is used to test it the page > fault was caused by a kprobe causing an early return from do_page_fault. > > Change the name of the helper to is_kprobe_fault to match the usage and > remove the preempt_disable/enable pair around kprobe_running() with an > explicit test for preemption. The idea for this comes from a patch > by Quentin Barnes to kprobes.c Sure, that's right. However, since other architectures also have notify_page_fault(), I think all of those code might better be changed same time for maintainability. Thanks, > Signed-off-by: Harvey Harrison <[EMAIL PROTECTED]> > --- > Ingo, this may not be functionally equivalent, feel free to yank it out > if there is any trouble, but from what I've seen it should be OK. > > Did you ever find a good kprobes test? > > arch/x86/mm/fault_32.c | 30 ++ > arch/x86/mm/fault_64.c | 30 ++ > 2 files changed, 28 insertions(+), 32 deletions(-) > > diff --git a/arch/x86/mm/fault_32.c b/arch/x86/mm/fault_32.c > index 051a4ec..5c48cc2 100644 > --- a/arch/x86/mm/fault_32.c > +++ b/arch/x86/mm/fault_32.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -42,23 +43,20 @@ > #define PF_RSVD (1<<3) > #define PF_INSTR (1<<4) > > -static inline int notify_page_fault(struct pt_regs *regs) > +static inline int is_kprobe_fault(struct pt_regs *regs) > { > -#ifdef CONFIG_KPROBES > int ret = 0; > - > - /* kprobe_running() needs smp_processor_id() */ > - if (!user_mode_vm(regs)) { > - preempt_disable(); > - if (kprobe_running() && kprobe_fault_handler(regs, 14)) > - ret = 1; > - preempt_enable(); > - } > - > - return ret; > -#else > - return 0; > +#ifdef CONFIG_KPROBES > + /* > + * If it is a kprobe fault we can not be premptible so return before > + * calling kprobe_running() as it will assert on smp_processor_id if > + * preemption is enabled. > + */ > + if (!user_mode_vm(regs) && !preemptible() && kprobe_running() && > + kprobe_fault_handler(regs, 14)) > + ret = 1; > #endif > + return ret; > } > > #ifdef CONFIG_X86_32 > @@ -428,7 +426,7 @@ void __kprobes do_page_fault(struct pt_regs *regs, > unsigned long error_code) > return; > } > #endif > - if (notify_page_fault(regs)) > + if (is_kprobe_fault(regs)) > return; > /* >* Don't take the mm semaphore here. If we fixup a prefetch > @@ -437,7 +435,7 @@ void __kprobes do_page_fault(struct pt_regs *regs, > unsigned long error_code) > goto bad_area_nosemaphore; > } > > - if (notify_page_fault(regs)) > + if (is_kprobe_fault(regs)) > return; > > /* It's safe to allow irq's after cr2 has been saved and the vmalloc > diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c > index 97b92b6..09008e5 100644 > --- a/arch/x86/mm/fault_64.c > +++ b/arch/x86/mm/fault_64.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -45,23 +46,20 @@ > #define PF_RSVD (1<<3) > #define PF_INSTR (1<<4) > > -static inline int notify_page_fault(struct pt_regs *regs) > +static inline int is_kprobe_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; > +#ifdef CONFIG_KPROBES > + /* > + * If it is a kprobe fault we can not be premptible so return before > + * calling kprobe_running() as it will assert on smp_processor_id if > + * preemption is enabled. > + */ > + if (!user_mode_vm(regs) && !preemptible() && kprobe_running() && > + kprobe_fault_handler(regs, 14)) > + ret = 1; > #endif > + return ret; > } > > #ifdef CONFIG_X86_32 > @@ -478,7 +476,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs > *regs, > return; > } > #endif > - if (notify_page_fault(regs)) > + if (is_kprobe_fault(regs)) > return; > /* >* Don't take the mm semaphore here. If we fixup a prefetch > @@ -487,7 +485,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)) > return; > > if
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
Hi Harvey, Harvey Harrison wrote: Currently the notify_page_fault helper is used to test it the page fault was caused by a kprobe causing an early return from do_page_fault. Change the name of the helper to is_kprobe_fault to match the usage and remove the preempt_disable/enable pair around kprobe_running() with an explicit test for preemption. The idea for this comes from a patch by Quentin Barnes to kprobes.c Sure, that's right. However, since other architectures also have notify_page_fault(), I think all of those code might better be changed same time for maintainability. Thanks, Signed-off-by: Harvey Harrison [EMAIL PROTECTED] --- Ingo, this may not be functionally equivalent, feel free to yank it out if there is any trouble, but from what I've seen it should be OK. Did you ever find a good kprobes test? arch/x86/mm/fault_32.c | 30 ++ arch/x86/mm/fault_64.c | 30 ++ 2 files changed, 28 insertions(+), 32 deletions(-) diff --git a/arch/x86/mm/fault_32.c b/arch/x86/mm/fault_32.c index 051a4ec..5c48cc2 100644 --- a/arch/x86/mm/fault_32.c +++ b/arch/x86/mm/fault_32.c @@ -12,6 +12,7 @@ #include linux/mman.h #include linux/mm.h #include linux/smp.h +#include linux/hardirq.h #include linux/interrupt.h #include linux/init.h #include linux/tty.h @@ -42,23 +43,20 @@ #define PF_RSVD (13) #define PF_INSTR (14) -static inline int notify_page_fault(struct pt_regs *regs) +static inline int is_kprobe_fault(struct pt_regs *regs) { -#ifdef CONFIG_KPROBES int ret = 0; - - /* kprobe_running() needs smp_processor_id() */ - if (!user_mode_vm(regs)) { - preempt_disable(); - if (kprobe_running() kprobe_fault_handler(regs, 14)) - ret = 1; - preempt_enable(); - } - - return ret; -#else - return 0; +#ifdef CONFIG_KPROBES + /* + * If it is a kprobe fault we can not be premptible so return before + * calling kprobe_running() as it will assert on smp_processor_id if + * preemption is enabled. + */ + if (!user_mode_vm(regs) !preemptible() kprobe_running() + kprobe_fault_handler(regs, 14)) + ret = 1; #endif + return ret; } #ifdef CONFIG_X86_32 @@ -428,7 +426,7 @@ void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code) return; } #endif - if (notify_page_fault(regs)) + if (is_kprobe_fault(regs)) return; /* * Don't take the mm semaphore here. If we fixup a prefetch @@ -437,7 +435,7 @@ void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code) goto bad_area_nosemaphore; } - if (notify_page_fault(regs)) + if (is_kprobe_fault(regs)) return; /* It's safe to allow irq's after cr2 has been saved and the vmalloc diff --git a/arch/x86/mm/fault_64.c b/arch/x86/mm/fault_64.c index 97b92b6..09008e5 100644 --- a/arch/x86/mm/fault_64.c +++ b/arch/x86/mm/fault_64.c @@ -13,6 +13,7 @@ #include linux/mman.h #include linux/mm.h #include linux/smp.h +#include linux/hardirq.h #include linux/interrupt.h #include linux/init.h #include linux/tty.h @@ -45,23 +46,20 @@ #define PF_RSVD (13) #define PF_INSTR (14) -static inline int notify_page_fault(struct pt_regs *regs) +static inline int is_kprobe_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; +#ifdef CONFIG_KPROBES + /* + * If it is a kprobe fault we can not be premptible so return before + * calling kprobe_running() as it will assert on smp_processor_id if + * preemption is enabled. + */ + if (!user_mode_vm(regs) !preemptible() kprobe_running() + kprobe_fault_handler(regs, 14)) + ret = 1; #endif + return ret; } #ifdef CONFIG_X86_32 @@ -478,7 +476,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs, return; } #endif - if (notify_page_fault(regs)) + if (is_kprobe_fault(regs)) return; /* * Don't take the mm semaphore here. If we fixup a prefetch @@ -487,7 +485,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)) return; if (likely(regs-flags
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
On Wed, 2008-01-02 at 21:36 -0500, Masami Hiramatsu wrote: Hi Harvey, Harvey Harrison wrote: Currently the notify_page_fault helper is used to test it the page fault was caused by a kprobe causing an early return from do_page_fault. Change the name of the helper to is_kprobe_fault to match the usage and remove the preempt_disable/enable pair around kprobe_running() with an explicit test for preemption. The idea for this comes from a patch by Quentin Barnes to kprobes.c Sure, that's right. However, since other architectures also have notify_page_fault(), I think all of those code might better be changed same time for maintainability. How about a static inline in linux/kprobes.h with a big comment above about when/why the !preemptible() check is sufficient? Harvey -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86: Use is_kprobe_fault to better match usage
Harvey Harrison wrote: On Wed, 2008-01-02 at 21:36 -0500, Masami Hiramatsu wrote: Hi Harvey, Harvey Harrison wrote: Currently the notify_page_fault helper is used to test it the page fault was caused by a kprobe causing an early return from do_page_fault. Change the name of the helper to is_kprobe_fault to match the usage and remove the preempt_disable/enable pair around kprobe_running() with an explicit test for preemption. The idea for this comes from a patch by Quentin Barnes to kprobes.c Sure, that's right. However, since other architectures also have notify_page_fault(), I think all of those code might better be changed same time for maintainability. How about a static inline in linux/kprobes.h with a big comment above about when/why the !preemptible() check is sufficient? Hmm, fault handling depends on the architecture. But current notify_page_fault()s are very similar. I think unifying it is good idea. We will be happy to review that if you send it. Many thanks! Harvey -- Masami Hiramatsu Software Engineer Hitachi Computer Products (America) Inc. Software Solutions Division e-mail: [EMAIL PROTECTED], [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/