| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <477BD366.1060504@redhat.com>
Date: Wed, 02 Jan 2008 13:09:42 -0500
From: Masami Hiramatsu <mhiramat@...hat.com>
To: Abhishek Sagar <sagar.abhishek@...il.com>
CC: Ingo Molnar <mingo@...e.hu>,
Harvey Harrison <harvey.harrison@...il.com>,
"H. Peter Anvin" <hpa@...or.com>,
LKML <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>, qbarnes@...il.com,
ananth@...ibm.com, jkenisto@...ibm.com
Subject: Re: [PATCH] x86: kprobes change kprobe_handler flow
Hi Abhishek,
Thank you for good work.
Abhishek Sagar wrote:
> @@ -441,6 +441,26 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> /* Replace the return addr with trampoline addr */
> *sara = (unsigned long) &kretprobe_trampoline;
> }
> +
> +#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
> +static __always_inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> +{
> + if (p->ainsn.boostable == 1 && !p->post_handler) {
> + /* Boost up -- we can execute copied instructions directly */
> + reset_current_kprobe();
> + regs->ip = (unsigned long)p->ainsn.insn;
> + preempt_enable_no_resched();
> + return 0;
> + }
> + return 1;
> +}
> +#else
> +static __always_inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> +{
> + return 1;
> +}
> +#endif
I think setup_singlestep() in your first patch is better, because it avoided
code duplication(*).
> +
> /*
> * We have reentered the kprobe_handler(), since another probe was hit while
> * within the handler. We save the original kprobes variables and just single
> @@ -449,29 +469,47 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
> struct kprobe_ctlblk *kcb)
> {
> - if (kcb->kprobe_status == KPROBE_HIT_SS &&
> - *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> - regs->flags &= ~X86_EFLAGS_TF;
> - regs->flags |= kcb->kprobe_saved_flags;
> - return 0;
> + int ret = 0;
> +
> + switch (kcb->kprobe_status) {
> + case KPROBE_HIT_SSDONE:
> #ifdef CONFIG_X86_64
> - } else if (kcb->kprobe_status == KPROBE_HIT_SSDONE) {
> - /* TODO: Provide re-entrancy from post_kprobes_handler() and
> - * avoid exception stack corruption while single-stepping on
> + /* TODO: Provide re-entrancy from
> + * post_kprobes_handler() and avoid exception
> + * stack corruption while single-stepping on
Why would you modify it?
> * the instruction of the new probe.
> */
> arch_disarm_kprobe(p);
> regs->ip = (unsigned long)p->addr;
> reset_current_kprobe();
> - return 1;
> + preempt_enable_no_resched();
I think preepmt_enable here is good idea.
> + ret = 1;
> + break;
> #endif
> + case KPROBE_HIT_ACTIVE:
> + /* a probe has been hit inside a
> + * user handler */
> + save_previous_kprobe(kcb);
> + set_current_kprobe(p, regs, kcb);
> + kprobes_inc_nmissed_count(p);
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_REENTER;
> + ret = 1;
> + break;
> + case KPROBE_HIT_SS:
> + if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> + regs->flags &= ~TF_MASK;
> + regs->flags |= kcb->kprobe_saved_flags;
> + } else {
> + /* BUG? */
> + }
> + break;
If my thought is correct, we don't need to use swich-case here,
Because there are only 2 cases, KPROBE_HIT_SSDONE (x86-64 only)
or others.
As a result, this function just setups re-entrance.
> + default:
> + /* impossible cases */
> + BUG();
I think no need to check that here.
> }
> - save_previous_kprobe(kcb);
> - set_current_kprobe(p, regs, kcb);
> - kprobes_inc_nmissed_count(p);
> - prepare_singlestep(p, regs);
> - kcb->kprobe_status = KPROBE_REENTER;
> - return 1;
> +
> + return ret;
> }
>
> /*
> @@ -480,82 +518,67 @@ static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
> */
> static int __kprobes kprobe_handler(struct pt_regs *regs)
> {
> - struct kprobe *p;
> int ret = 0;
> kprobe_opcode_t *addr;
> + struct kprobe *p, *cur;
> struct kprobe_ctlblk *kcb;
>
> addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
> + if (*addr != BREAKPOINT_INSTRUCTION) {
> + /*
> + * The breakpoint instruction was removed right
> + * after we hit it. Another cpu has removed
> + * either a probepoint or a debugger breakpoint
> + * at this address. In either case, no further
> + * handling of this interrupt is appropriate.
> + * Back up over the (now missing) int3 and run
> + * the original instruction.
> + */
> + regs->ip = (unsigned long)addr;
> + return 1;
> + }
I think this block changing would better be separated from this patch,
because it changes code flow logically.
>
> - /*
> - * We don't want to be preempted for the entire
> - * duration of kprobe processing
> - */
> preempt_disable();
> kcb = get_kprobe_ctlblk();
> -
> + cur = kprobe_running();
I think you don't need "cur", because kprobe_running() is called
just once on each path.
> p = get_kprobe(addr);
> +
> if (p) {
> - /* Check we're not actually recursing */
> - if (kprobe_running()) {
> + if (cur) {
> ret = reenter_kprobe(p, regs, kcb);
> - if (kcb->kprobe_status == KPROBE_REENTER)
> - {
> - ret = 1;
> - goto out;
> - }
> - goto preempt_out;
> } else {
> set_current_kprobe(p, regs, kcb);
> kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> - if (p->pre_handler && p->pre_handler(p, regs))
> - {
> - /* handler set things up, skip ss setup */
> - ret = 1;
> - goto out;
> - }
> - }
> - } else {
> - if (*addr != BREAKPOINT_INSTRUCTION) {
> +
> /*
> - * The breakpoint instruction was removed right
> - * after we hit it. Another cpu has removed
> - * either a probepoint or a debugger breakpoint
> - * at this address. In either case, no further
> - * handling of this interrupt is appropriate.
> - * Back up over the (now missing) int3 and run
> - * the original instruction.
> + * If we have no pre-handler or it returned 0, we
> + * continue with normal processing. If we have a
> + * pre-handler and it returned non-zero, it prepped
> + * for calling the break_handler below on re-entry
> + * for jprobe processing, so get out doing nothing
> + * more here.
> */
> - regs->ip = (unsigned long)addr;
> + if (!p->pre_handler || !p->pre_handler(p, regs)) {
> + if (setup_boost(p, regs)) {
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_HIT_SS;
(*) duplication 1
> + }
> + }
> ret = 1;
> - goto preempt_out;
> }
> - if (kprobe_running()) {
> - p = __get_cpu_var(current_kprobe);
> - if (p->break_handler && p->break_handler(p, regs))
> - goto ss_probe;
> + } else if (cur) {
> + p = __get_cpu_var(current_kprobe);
> + if (p->break_handler && p->break_handler(p, regs)) {
> + if (setup_boost(p, regs)) {
> + prepare_singlestep(p, regs);
> + kcb->kprobe_status = KPROBE_HIT_SS;
(*) duplication 2
> + }
> + ret = 1;
> }
> - /* Not one of ours: let kernel handle it */
> - goto preempt_out;
> - }
> + } /* else: not a kprobe fault; let the kernel handle it */
>
> -ss_probe:
> - ret = 1;
> -#if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
> - if (p->ainsn.boostable == 1 && !p->post_handler) {
> - /* Boost up -- we can execute copied instructions directly */
> - reset_current_kprobe();
> - regs->ip = (unsigned long)p->ainsn.insn;
> - goto preempt_out;
> - }
> -#endif
> - prepare_singlestep(p, regs);
> - kcb->kprobe_status = KPROBE_HIT_SS;
> - goto out;
> -
> -preempt_out:
> - preempt_enable_no_resched();
> -out:
> + if (!ret)
> + preempt_enable_no_resched();
I think this is a bit ugly...
Why would you avoid using mutiple "return"s in this function?
I think you can remove "ret" from this function.
> return ret;
> }
>
>
Thank you,
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: mhiramat@...hat.com, masami.hiramatsu.pt@...achi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists