Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Functions converted to `noinstr` directly call non-noinstr and instrumentable code without using the required `instrumentation_begin()` / `instrumentation_end()` wrappers, violating strict non-instrumentation boundaries. --
--- Patch [3]: [RFC PATCH v2 3/4] arm64/kprobes: Make the entire Kprobe noinstr --- commit 3ef2956d1a4e614b73b64bbc64be2a2e20e577e4 Author: Hongyan Xia <[email protected]> arm64/kprobes: Make the entire Kprobe noinstr This commit converts the kprobe handling path to use noinstr instead of __kprobes, ensuring the code between debug_exception entry and exit is strictly non-instrumentable. It also marks leaf helpers as __always_inline and explicitly leaves the instruction simulation path as future work. > diff --git a/arch/arm64/kernel/probes/kprobes.c > b/arch/arm64/kernel/probes/kprobes.c > index 0e66abf9958e2..5ec5f4ef99852 100644 > --- a/arch/arm64/kernel/probes/kprobes.c > +++ b/arch/arm64/kernel/probes/kprobes.c [ ... ] > -static void __kprobes setup_singlestep(struct kprobe *p, > - struct pt_regs *regs, > - struct kprobe_ctlblk *kcb, int reenter) > +static void noinstr setup_singlestep(struct kprobe *p, > + struct pt_regs *regs, > + struct kprobe_ctlblk *kcb, int reenter) > { [ ... ] > if (p->ainsn.xol_insn) { > /* prepare for single stepping */ > slot = (unsigned long)p->ainsn.xol_insn; > > kprobes_save_local_irqflag(kcb, regs); > instruction_pointer_set(regs, slot); > } else { > /* insn simulation */ > arch_simulate_insn(p, regs); [Severity: Medium] Since setup_singlestep() is now marked noinstr, is it safe to call arch_simulate_insn() here without instrumentation_begin() and instrumentation_end()? The commit message notes the simulation path isn't noinstr yet, so calling it directly from a noinstr function could trigger objtool validation failures and issues with KASAN/KCOV. > } > } > > -static int __kprobes reenter_kprobe(struct kprobe *p, > - struct pt_regs *regs, > - struct kprobe_ctlblk *kcb) > +static int noinstr reenter_kprobe(struct kprobe *p, > + struct pt_regs *regs, > + struct kprobe_ctlblk *kcb) > { > switch (kcb->kprobe_status) { > case KPROBE_HIT_SSDONE: > case KPROBE_HIT_ACTIVE: > kprobes_inc_nmissed_count(p); > setup_singlestep(p, regs, kcb, 1); > break; > case KPROBE_HIT_SS: > case KPROBE_REENTER: > pr_warn("Failed to recover from reentered kprobes.\n"); > dump_kprobe(p); > BUG(); > break; > default: > WARN_ON(1); [Severity: Medium] Similarly, reenter_kprobe() is now noinstr but makes calls to instrumentable functions like pr_warn(), dump_kprobe(), BUG(), and WARN_ON(). Should these be wrapped in instrumentation_begin() / instrumentation_end() to prevent instrumentation from running in an un-instrumentable context? > return 0; > } > > return 1; > } > > -static void __kprobes > +static void noinstr > post_kprobe_handler(struct kprobe *cur, struct kprobe_ctlblk *kcb, struct > pt_regs *regs) > { > /* return addr restore if non-branching insn */ > if (cur->ainsn.xol_restore != 0) > instruction_pointer_set(regs, cur->ainsn.xol_restore); > > /* restore back original saved kprobe variables and continue */ > if (kcb->kprobe_status == KPROBE_REENTER) { > restore_previous_kprobe(kcb); > return; > } > /* call post handler */ > kcb->kprobe_status = KPROBE_HIT_SSDONE; > if (cur->post_handler) > cur->post_handler(cur, regs, 0); [Severity: Medium] Does the call to cur->post_handler() also need to be wrapped in instrumentation_begin() / instrumentation_end()? User-supplied post handlers are instrumentable, so executing them directly from a noinstr context might violate strict non-instrumentation boundaries if the probe triggers in early exception entry. > > reset_current_kprobe(); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
