lkml.org 
[lkml]   [2022]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 3/3] arm64: kprobes: Return DBG_HOOK_ERROR if kprobes can not handle a BRK
On Thu, 1 Dec 2022 17:21:55 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> On Fri, Dec 02, 2022 at 01:07:13AM +0900, Masami Hiramatsu wrote:
> > On Thu, 1 Dec 2022 15:08:52 +0000
> > Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > > On Thu, Dec 01, 2022 at 11:39:21PM +0900, Masami Hiramatsu (Google) wrote:
> > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > >
> > > > Return DBG_HOOK_ERROR if kprobes can not handle a BRK because it
> > > > fails to find a kprobe corresponding to the address.
> > > >
> > > > Since arm64 kprobes uses stop_machine based text patching for removing
> > > > BRK, it ensures all running kprobe_break_handler() is done at that point.
> > > > And after removing the BRK, it removes the kprobe from its hash list.
> > > > Thus, if the kprobe_break_handler() fails to find kprobe from hash list,
> > > > there is a bug.
> > >
> > > IIUC this relies on BRK handling not being preemptible, which is something
> > > we've repeatedly considered changing along with a bunch of other debug
> > > exception handling.
> >
> > Interesting idea... and it also need many changes in kprobe itself.
> >
> > >
> > > In case we do try to change that in future, it would be good to have a comment
> > > somewhere to that effect.
> >
> > Hmm, it would fundamentally change the assumptions that kprobes relies on,
> > and would require a lot of thought again. (e.g. current running kprobe is
> > stored in per-cpu variable, it should be per-task. etc.)
>
> Ah; I had not considered that.
>
> Feel free to ignore the above; with the comments as below:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>

OK, Thanks!

>
> Thanks,
> Mark.
>
> >
> > >
> > > I think there are other ways we could synchronise against that (e.g. using RCU
> > > tasks rude) if we ever do that, and this patch looks good to me.
> > >
> > > >
> > > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > ---
> > > > arch/arm64/kernel/probes/kprobes.c | 79 +++++++++++++++++-------------------
> > > > 1 file changed, 37 insertions(+), 42 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > > > index d2ae37f89774..ea56b22d4da8 100644
> > > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > > @@ -298,7 +298,8 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
> > > > return 0;
> > > > }
> > > >
> > > > -static void __kprobes kprobe_handler(struct pt_regs *regs)
> > > > +static int __kprobes
> > > > +kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > > > {
> > > > struct kprobe *p, *cur_kprobe;
> > > > struct kprobe_ctlblk *kcb;
> > > > @@ -308,39 +309,45 @@ static void __kprobes kprobe_handler(struct pt_regs *regs)
> > > > cur_kprobe = kprobe_running();
> > > >
> > > > p = get_kprobe((kprobe_opcode_t *) addr);
> > > > + if (WARN_ON_ONCE(!p)) {
> > > > + /*
> > > > + * Something went wrong. This must be put by kprobe, but we
> > > > + * could not find corresponding kprobes. Let the kernel handle
> > > > + * this error case.
> > > > + */
> > >
> > > Could we make this:
> > >
> > > /*
> > > * Something went wrong. This BRK used an immediate reserved
> > > * for kprobes, but we couldn't find any corresponding probe.
> > > */
> >
> > OK.
> >
> > >
> > > > + return DBG_HOOK_ERROR;
> > > > + }
> > > >
> > > > - if (p) {
> > > > - if (cur_kprobe) {
> > > > - if (reenter_kprobe(p, regs, kcb))
> > > > - return;
> > > > - } else {
> > > > - /* Probe hit */
> > > > - set_current_kprobe(p);
> > > > - kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > > > -
> > > > - /*
> > > > - * 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 will
> > > > - * modify the execution path and no need to single
> > > > - * stepping. Let's just reset current kprobe and exit.
> > > > - */
> > > > - if (!p->pre_handler || !p->pre_handler(p, regs)) {
> > > > - setup_singlestep(p, regs, kcb, 0);
> > > > - } else
> > > > - reset_current_kprobe();
> > > > - }
> > > > + if (cur_kprobe) {
> > > > + /* Hit a kprobe inside another kprobe */
> > > > + if (!reenter_kprobe(p, regs, kcb))
> > > > + return DBG_HOOK_ERROR;
> > > > + } else {
> > > > + /* Probe hit */
> > > > + set_current_kprobe(p);
> > > > + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > > > +
> > > > + /*
> > > > + * 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 will
> > > > + * modify the execution path and no need to single
> > > > + * stepping. Let's just reset current kprobe and exit.
> > > > + */
> > >
> > > Minor wording nit: could we replace:
> > >
> > > no need to single stepping.
> > >
> > > With:
> > >
> > > not need to single-step.
> >
> > OK, I'll update both in v2.
> >
> > Thank you!
> >
> > >
> > > Thanks,
> > > Mark.
> > >
> > > > + if (!p->pre_handler || !p->pre_handler(p, regs))
> > > > + setup_singlestep(p, regs, kcb, 0);
> > > > + else
> > > > + reset_current_kprobe();
> > > > }
> > > > - /*
> > > > - * 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.
> > > > - * Return back to original instruction, and continue.
> > > > - */
> > > > +
> > > > + return DBG_HOOK_HANDLED;
> > > > }
> > > >
> > > > +static struct break_hook kprobes_break_hook = {
> > > > + .imm = KPROBES_BRK_IMM,
> > > > + .fn = kprobe_breakpoint_handler,
> > > > +};
> > > > +
> > > > static int __kprobes
> > > > kprobe_breakpoint_ss_handler(struct pt_regs *regs, unsigned long esr)
> > > > {
> > > > @@ -365,18 +372,6 @@ static struct break_hook kprobes_break_ss_hook = {
> > > > .fn = kprobe_breakpoint_ss_handler,
> > > > };
> > > >
> > > > -static int __kprobes
> > > > -kprobe_breakpoint_handler(struct pt_regs *regs, unsigned long esr)
> > > > -{
> > > > - kprobe_handler(regs);
> > > > - return DBG_HOOK_HANDLED;
> > > > -}
> > > > -
> > > > -static struct break_hook kprobes_break_hook = {
> > > > - .imm = KPROBES_BRK_IMM,
> > > > - .fn = kprobe_breakpoint_handler,
> > > > -};
> > > > -
> > > > /*
> > > > * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
> > > > * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
> > > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2022-12-02 01:44    [W:0.120 / U:1.968 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site