Messages in this thread Patch in this message | | | Date | Fri, 2 Dec 2022 09:42:58 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [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>
| |