From: Keshavamurthy Anil S <anil.s.keshavamurthy@intel.com>
To: Prasanna S Panchamukhi <prasanna@in.ibm.com>
Cc: "Keshavamurthy, Anil S" <anil.s.keshavamurthy@intel.com>,
systemtap@sources.redhat.com
Subject: Re: [PATCH] Kprobes- robust fault handling for i386
Date: Tue, 28 Feb 2006 20:25:00 -0000 [thread overview]
Message-ID: <20060228122526.A16881@unix-os.sc.intel.com> (raw)
In-Reply-To: <20060228143836.GA24545@in.ibm.com>; from prasanna@in.ibm.com on Tue, Feb 28, 2006 at 06:38:36AM -0800
On Tue, Feb 28, 2006 at 06:38:36AM -0800, Prasanna S Panchamukhi wrote:
>
> Anil,
>
> Thanks for your review comments. Please see the updated patch
> below, this patch is only for i386 architecture and once
> we are ok with it, we will port it to other architectures.
This version looks good with no new Kprobes states.
Makes life easy to understand :-)
> [..]The main reason to avoid post_handler execution in this
> case is to avoid any incosistant data references between pre and post
> handlers.
Okay, I got that point, but if the fault recovery in pre_handler
is *successful*, then in this case you *should* permit calling
post_handler. See my inline comments to address this issue.
Thanks,
Anil
>
> Signed-off-by: Prasanna S Panchamukhi <prasanna@in.ibm.com>
>
> arch/i386/kernel/kprobes.c | 66
> +++++++++++++++++++++++++++++++++++++++------
> include/asm-i386/kprobes.h | 1
> kernel/kprobes.c | 14 ++++++++-
> 3 files changed, 72 insertions(+), 9 deletions(-)
>
> diff -puN include/asm-i386/kprobes.h~kprobes-i386-pagefault-handling
> include/asm-i386/kprobes.h
> ---
> linux-2.6.16-rc4-mm2/include/asm-i386/kprobes.h~kprobes-i386-pagefault
> -handling 2006-02-28 18:00:20.000000000 +0530
>
> +++ linux-2.6.16-rc4-mm2-prasanna/include/asm-i386/kprobes.h
> 2006-02-28 18:01:16.000000000 +0530
> @@ -74,6 +74,7 @@ struct kprobe_ctlblk {
> long *jprobe_saved_esp;
> struct pt_regs jprobe_saved_regs;
> kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE];
> + struct kprobe *kprobe_faulted;
> struct prev_kprobe prev_kprobe;
> };
Good approach.
>
> diff -puN arch/i386/kernel/kprobes.c~kprobes-i386-pagefault-handling
> arch/i386/kernel/kprobes.c
> ---
> linux-2.6.16-rc4-mm2/arch/i386/kernel/kprobes.c~kprobes-i386-pagefault
> -handling 2006-02-28 09:47:48.000000000 +0530
>
> +++ linux-2.6.16-rc4-mm2-prasanna/arch/i386/kernel/kprobes.c
> 2006-02-28 19:34:20.000000000 +0530
> @@ -35,6 +35,7 @@
> #include <asm/cacheflush.h>
> #include <asm/kdebug.h>
> #include <asm/desc.h>
> +#include <asm/uaccess.h>
>
> void jprobe_return_end(void);
>
> @@ -523,7 +524,8 @@ static inline int post_kprobe_handler(st
>
> if ((kcb->kprobe_status != KPROBE_REENTER) &&
> cur->post_handler) {
> kcb->kprobe_status = KPROBE_HIT_SSDONE;
> - cur->post_handler(cur, regs, 0);
> + if (kcb->kprobe_faulted != cur)
> + cur->post_handler(cur, regs, 0);
> }
>
> resume_execution(cur, regs, kcb);
> @@ -554,15 +556,63 @@ static inline int kprobe_fault_handler(s
> struct kprobe *cur = kprobe_running();
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>
> - if (cur->fault_handler && cur->fault_handler(cur, regs,
> trapnr))
> - return 1;
> -
> - if (kcb->kprobe_status & KPROBE_HIT_SS) {
> - resume_execution(cur, regs, kcb);
> + switch(kcb->kprobe_status) {
> + case KPROBE_HIT_SS:
> + case KPROBE_REENTER:
> + /*
> + * We are here because the instruction being single
> + * stepped caused a page fault. We reset the current
> + * kprobe and the eip points back to the probe address
> + * and allow the page fault handler to continue as a
> + * normal page fault.
> + */
> + regs->eip = (unsigned long)cur->addr;
> regs->eflags |= kcb->kprobe_old_eflags;
> -
> - reset_current_kprobe();
> + if (kcb->kprobe_status == KPROBE_REENTER)
> + restore_previous_kprobe(kcb);
> + else
> + reset_current_kprobe();
> preempt_enable_no_resched();
> + break;
> + case KPROBE_HIT_ACTIVE:
> + /*
> + * Set appropriate kprobe instance, so that
> corresponding
> + * post_handler can be skipped in order to avoid any
> + * inconsistant data.
> + */
> + kcb->kprobe_faulted = cur;
> + case KPROBE_HIT_SSDONE:
> + /*
> + * We increment the nmissed count for accounting,
> + * we can also use npre/npostfault count for accouting
> + * these specific fault cases.
> + */
> + kprobes_inc_nmissed_count(cur);
> +
> + /*
> + * We come here because instructions in the pre/post
> + * handler caused the page_fault, this could happen
> + * if handler tries to access user space by
> + * copy_from_user(), get_user() etc. Let the
> + * user-specified handler try to fix it first.
> + */
> + if (cur->fault_handler && cur->fault_handler(cur,
> regs, trapnr))
> + return 1;
if the fault recovery is successful, before returning 1, you
need to reset kcb->kprobe_faulted to NULL;
> +
> + /*
> + * In case the user-specified fault handler returned
> + * zero, try to fix up.
> + */
> + if (fixup_exception(regs))
> + return 1;
same here, before returning 1, you need to reset kcb->kprobe_faulted to NULL;
> +
> + /*
> + * fixup_exception() could not handle it,
> + * Let do_page_fault() fix it.
> + */
> + break;
> + default:
> + break;
> }
> return 0;
> }
> diff -puN kernel/kprobes.c~kprobes-i386-pagefault-handling
> kernel/kprobes.c
> ---
> linux-2.6.16-rc4-mm2/kernel/kprobes.c~kprobes-i386-pagefault-handling
> 2006-02-28 18:04:09.000000000 +0530
> +++ linux-2.6.16-rc4-mm2-prasanna/kernel/kprobes.c 2006-02-28
> 19:27:33.000000000 +0530
> @@ -208,9 +208,14 @@ static void __kprobes aggr_post_handler(
> unsigned long flags)
> {
> struct kprobe *kp;
> + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>
> list_for_each_entry_rcu(kp, &p->list, list) {
> - if (kp->post_handler) {
> + /*
> + * Check if the corresponding pre_handler had faulted,
> avoid
> + * the post_handler in such a case.
> + */
> + if (kp->post_handler && (kcb->kprobe_faulted != kp)) {
> set_kprobe_instance(kp);
> kp->post_handler(kp, regs, flags);
> reset_kprobe_instance();
> @@ -223,12 +228,19 @@ static int __kprobes aggr_fault_handler(
> int trapnr)
> {
> struct kprobe *cur = __get_cpu_var(kprobe_instance);
> + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>
> /*
> * if we faulted "during" the execution of a user specified
> * probe handler, invoke just that probe's fault handler
> */
> if (cur && cur->fault_handler) {
> + /*
> + * Set kprobe_faulted to appropriate kprobe instance,
> so that
> + * corresponding post handler can be skipped if the
> fault
> + * happened due to pre_handler.
> + */
> + kcb->kprobe_faulted = cur;
Move this kcb->kprobe_faulted = cur; before if(curr && cur->handler) {}
The reason is, irrespective of cur->fault_handler, we need to save
kcb->kprobe_faulted, so post handler can be skipped properly.
> if (cur->fault_handler(cur, regs, trapnr))
> return 1;
> }
>
> _
> --
> Prasanna S Panchamukhi
> Linux Technology Center
> India Software Labs, IBM Bangalore
> Email: prasanna@in.ibm.com
> Ph: 91-80-51776329
next prev parent reply other threads:[~2006-02-28 20:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-02-24 19:17 Keshavamurthy, Anil S
2006-02-27 9:24 ` Prasanna S Panchamukhi
2006-02-27 9:25 ` [PATCH] Kprobes- robust fault handling for i386 post_handler changes Prasanna S Panchamukhi
2006-02-28 1:02 ` [PATCH] Kprobes- robust fault handling for i386 Keshavamurthy Anil S
2006-02-28 14:37 ` Prasanna S Panchamukhi
2006-02-28 20:25 ` Keshavamurthy Anil S [this message]
2006-03-01 14:49 ` Prasanna S Panchamukhi
-- strict thread matches above, loose matches on Subject: below --
2006-02-23 0:44 Keshavamurthy, Anil S
2006-02-22 10:41 Mao, Bibo
2006-02-23 8:58 ` Prasanna S Panchamukhi
2006-02-23 12:40 ` Frank Ch. Eigler
2006-02-23 13:17 ` Prasanna S Panchamukhi
2006-02-22 7:11 Prasanna S Panchamukhi
2006-02-24 1:33 ` Jim Keniston
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060228122526.A16881@unix-os.sc.intel.com \
--to=anil.s.keshavamurthy@intel.com \
--cc=prasanna@in.ibm.com \
--cc=systemtap@sources.redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).