linux-kernel - Re: [PATCH] x86: kprobes change kprobe

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <863e9df20801011254g12f30cces58d452cfeeb4350e@mail.gmail.com>
Date:	Wed, 2 Jan 2008 02:24:09 +0530
From:	"Abhishek Sagar" <sagar.abhishek@...il.com>
To:	"Harvey Harrison" <harvey.harrison@...il.com>
Cc:	"Ingo Molnar" <mingo@...e.hu>,
	"Masami Hiramatsu" <mhiramat@...hat.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

On 1/2/08, Harvey Harrison <harvey.harrison@...il.com> wrote:
> > +#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
> > +
> In the kernel __always_inline == inline, also I think it's nicer to only
> have one function declaration, and then ifdef the body of the function.
>
> Something like:
>
> static inline int setup_boost(struct kprobe *p, struct pt_regs *regs)
> {
> #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;
>                 preempt_enable_no_resched();
>                 return 0;
>         }
> #endif
>         return 1;
> }

Ok...will include this after I pick up some more comments.

> >  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;
> > +     }
>
> This return is fine I guess, but after the preempt_disable() I like
> the goto approach as it will be easier to see what paths enable
> preemption again and which don't....bonus points if we can move this
> to the caller or make sure we reenable in all cases before returning
> and pull in the code in the caller that does this for us.
>
> But I guess your approach of using ret to test whether we need to
> reenable preemption or not would work as a signal to the caller that
> they need to reenable preemption.

Hmm...since enabling preemption is tied to 'ret', anyone reading
kprobe_handler will have to follow around all calls which modify it.
There are some checks in the current kprobe_handler definition made
just to do what you're saying, i.e, to push all preemption
enable/disables in krpobe_handler. LIke this one (from the current x86
kprobe_handler):

------------
ret = reenter_kprobe(p, regs, kcb);
if (kcb->kprobe_status == KPROBE_REENTER)
{
    ret = 1;
    goto out;
}
goto preempt_out;

-------------

This is just confusing because we're not actually making any
exceptions here for the KPROBE_REENTER case (which has been partially
handled in reenter_kprobe), rather just tricking our way out of
preemption enabling for a cpl of cases in reenter_kprobe.

> Cheers,
>
> Harvey

Thanks,
Abhishek
--
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/