| 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
| ||
|
Date: Wed, 2 Jan 2008 01:54:04 +0530
From: "Abhishek Sagar" <sagar.abhishek@...il.com>
To: "Masami Hiramatsu" <mhiramat@...hat.com>
Cc: "Harvey Harrison" <harvey.harrison@...il.com>,
"Ingo Molnar" <mingo@...e.hu>, "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
On 1/1/08, Masami Hiramatsu <mhiramat@...hat.com> wrote:
> Could you separate changing logic from cleanup and explain
> why the logic should be changed?
The major portion of logical changes is re-routing of code flow by
removing gotos. Hopefully all the cases have been covered. There are a
couple of changes that I'd like to address (see below).
> > +static __always_inline void setup_singlestep(struct kprobe *p,
> > + struct pt_regs *regs,
> > + struct kprobe_ctlblk *kcb)
> > +{
> > +#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->eip = (unsigned long)p->ainsn.insn;
> > + preempt_enable_no_resched();
> > + } else {
> > + prepare_singlestep(p, regs);
> > + kcb->kprobe_status = KPROBE_HIT_SS;
> > + }
> > +#else
> > + prepare_singlestep(p, regs);
> > + kcb->kprobe_status = KPROBE_HIT_SS;
>
> please avoid code duplication.
I've addressed it in the latest patch.
> > 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->eip - 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->eip -= sizeof(kprobe_opcode_t);
> > + return 1;
> > + }
I have moved the above breakpoint removal check at the beginning of
the function. The current check is for '!p' case only, whereas IMO
this check should be performed for all cases. An external debugger may
very well plant and remove a breakpoint on an address which has probe
on it. This check is a race nevertheless, so its relative placing
within kprobe_handler() would be best where its duplication can be
avoided.
> >
> > - /* Check we're not actually recursing */
> > - if (kprobe_running()) {
> > - p = get_kprobe(addr);
> > - if (p) {
> > - if (kcb->kprobe_status == KPROBE_HIT_SS &&
> > - *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> > - regs->eflags &= ~TF_MASK;
> > - regs->eflags |= kcb->kprobe_saved_eflags;
> > - goto no_kprobe;
> > - }
> > - /* We have reentered the kprobe_handler(), since
> > - * another probe was hit while within the handler.
> > - * We here save the original kprobes variables and
> > - * just single step on the instruction of the new probe
> > - * without calling any user handlers.
> > - */
> > - 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;
> > - } else {
> > - if (*addr != BREAKPOINT_INSTRUCTION) {
> > - /* The breakpoint instruction was removed by
> > - * another cpu right after we hit, no further
> > - * handling of this interrupt is appropriate
> > - */
> > - regs->eip -= sizeof(kprobe_opcode_t);
> > + if (p) {
> > + if (cur) {
> > + switch (kcb->kprobe_status) {
> > + case KPROBE_HIT_ACTIVE:
> > + case KPROBE_HIT_SSDONE:
> > + /* 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;
> > - goto no_kprobe;
> > - }
> > - p = __get_cpu_var(current_kprobe);
> > - if (p->break_handler && p->break_handler(p, regs)) {
> > - goto ss_probe;
> > + break;
> > + case KPROBE_HIT_SS:
> > + if (*p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
> > + regs->eflags &= ~TF_MASK;
> > + regs->eflags |=
> > + kcb->kprobe_saved_eflags;
> > + } else {
> > + /* BUG? */
> > + }
> > + break;
> > + default:
> > + /* impossible cases */
> > + BUG();
> > }
> > - }
Replaced deeply nested if-elses with a switch.
Please let me know if there are any changes on which you would like
further clarification.
> --
> Masami Hiramatsu
>
> Software Engineer
> Hitachi Computer Products (America) Inc.
> Software Solutions Division
>
> e-mail: mhiramat@...hat.com, masami.hiramatsu.pt@...achi.com
--
Thanks,
Abhishek Sagar
--
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