Re: [RFC PATCH v2 3/4] arm64/kprobes: Make the entire Kprobe noinstr

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