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

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

[Hongyan Xia]

From: Hongyan Xia <hongyan.xia@transsion.com>

Convert all the sub-functions called by kprobe_brk_handler,
kprobe_ss_brk_handler and kretprobe_brk_handler noinstr. After this
commit, the entire Kprobe path (anything between Kprobe debug_exception
entry and exit) is now noinstr.

One big item that is missing is the instruction simulation path. We
leave it as future work as the GCS handling needs extra care.

The __kprobes attribute (notrace + .kprobes.text) is replaced by
noinstr, which is a strict superset for these functions.

Also mark the leaf helpers used inside these functions
__always_inline, as clang does not reliably inline plain static inline
functions into noinline (noinstr) callers.

Signed-off-by: Hongyan Xia <hongyan.xia@transsion.com>
---
 arch/arm64/include/asm/percpu.h    |  2 +-
 arch/arm64/include/asm/ptrace.h    |  4 ++--
 arch/arm64/kernel/probes/kprobes.c | 30 +++++++++++++++---------------
 include/linux/kprobes.h            |  8 ++++----
 4 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/include/asm/percpu.h b/arch/arm64/include/asm/percpu.h
index b57b2bb00967..d4cae47fde8c 100644
--- a/arch/arm64/include/asm/percpu.h
+++ b/arch/arm64/include/asm/percpu.h
@@ -29,7 +29,7 @@ static inline unsigned long __hyp_my_cpu_offset(void)
 	return read_sysreg(tpidr_el2);
 }
 
-static inline unsigned long __kern_my_cpu_offset(void)
+static __always_inline unsigned long __kern_my_cpu_offset(void)
 {
 	unsigned long off;
 
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 39582511ad72..460726224299 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -336,11 +336,11 @@ static inline unsigned long regs_get_kernel_argument(struct pt_regs *regs,
 struct task_struct;
 int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task);
 
-static inline unsigned long instruction_pointer(struct pt_regs *regs)
+static __always_inline unsigned long instruction_pointer(struct pt_regs *regs)
 {
 	return regs->pc;
 }
-static inline void instruction_pointer_set(struct pt_regs *regs,
+static __always_inline void instruction_pointer_set(struct pt_regs *regs,
 		unsigned long val)
 {
 	regs->pc = val;
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 0e66abf9958e..5ec5f4ef9985 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -39,7 +39,7 @@
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
-static void __kprobes
+static void noinstr
 post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
 
 void *alloc_insn_page(void)
@@ -170,7 +170,7 @@ void __kprobes arch_remove_kprobe(struct kprobe *p)
 	}
 }
 
-static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
+static void noinstr save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
 	kcb->prev_kprobe.kp = kprobe_running();
 	kcb->prev_kprobe.status = kcb->kprobe_status;
@@ -184,7 +184,7 @@ static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
 	kcb->prev_kprobe.saved_irqflag = kcb->saved_irqflag;
 }
 
-static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
+static void noinstr restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
 	__this_cpu_write(current_kprobe, kcb->prev_kprobe.kp);
 	kcb->kprobe_status = kcb->prev_kprobe.status;
@@ -197,7 +197,7 @@ static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
 	kcb->saved_irqflag = kcb->prev_kprobe.saved_irqflag;
 }
 
-static void __kprobes set_current_kprobe(struct kprobe *p)
+static void noinstr set_current_kprobe(struct kprobe *p)
 {
 	__this_cpu_write(current_kprobe, p);
 }
@@ -207,23 +207,23 @@ static void __kprobes set_current_kprobe(struct kprobe *p)
  * simple and avoid nesting exceptions. Interrupts do have to be disabled since
  * the kprobe state is per-CPU and doesn't get migrated.
  */
-static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
-						struct pt_regs *regs)
+static void noinstr kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
+					       struct pt_regs *regs)
 {
 	kcb->saved_irqflag = regs->pstate & DAIF_MASK;
 	regs->pstate |= DAIF_MASK;
 }
 
-static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
-						struct pt_regs *regs)
+static void noinstr kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
+						  struct pt_regs *regs)
 {
 	regs->pstate &= ~DAIF_MASK;
 	regs->pstate |= kcb->saved_irqflag;
 }
 
-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)
 {
 	unsigned long slot;
 
@@ -248,9 +248,9 @@ static void __kprobes setup_singlestep(struct kprobe *p,
 	}
 }
 
-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:
@@ -278,7 +278,7 @@ static int __kprobes reenter_kprobe(struct kprobe *p,
 	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 */
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 8c4f3bb24429..5880445ed0f0 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -384,17 +384,17 @@ static inline void kprobe_ftrace_kill(void) {}
 struct kprobe *get_kprobe(void *addr);
 
 /* kprobe_running() will just return the current_kprobe on this CPU */
-static inline struct kprobe *kprobe_running(void)
+static __always_inline struct kprobe *kprobe_running(void)
 {
 	return __this_cpu_read(current_kprobe);
 }
 
-static inline void reset_current_kprobe(void)
+static __always_inline void reset_current_kprobe(void)
 {
 	__this_cpu_write(current_kprobe, NULL);
 }
 
-static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
+static __always_inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
 {
 	return this_cpu_ptr(&kprobe_ctlblk);
 }
@@ -449,7 +449,7 @@ static inline struct kprobe *get_kprobe(void *addr)
 {
 	return NULL;
 }
-static inline struct kprobe *kprobe_running(void)
+static __always_inline struct kprobe *kprobe_running(void)
 {
 	return NULL;
 }
-- 
2.47.3

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

[sashiko-bot]

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 <hongyan.xia@transsion.com>
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/cover.1786603168.git.hongyan.xia@transsion.com?part=3