[PATCH v3 0/2] Modify some code about kprobe

[PATCH v3 0/2] Modify some code about kprobe

[Tiezhu Yang]

v3: no need to change post_kprobe_handler() in patch #2 now,
    sorry for the carelessness.

v2:
  -- only replace __kprobes with NOKPROBE_SYMBOL() in patch #2
  -- update the commit messages

Tiezhu Yang (2):
  selftests/ftrace: Save kprobe_events to test log
  MIPS: Use NOKPROBE_SYMBOL() instead of __kprobes annotation

 arch/mips/kernel/kprobes.c                         | 36 ++++++++++++++--------
 arch/mips/mm/fault.c                               |  6 ++--
 .../ftrace/test.d/kprobe/multiple_kprobes.tc       |  2 ++
 3 files changed, 30 insertions(+), 14 deletions(-)

-- 
2.1.0

[PATCH v3 1/2] selftests/ftrace: Save kprobe_events to test log

[Tiezhu Yang]

It may lead to kernel panic when execute the following testcase on mips:

  # cd tools/testing/selftests/ftrace
  # ./ftracetest test.d/kprobe/multiple_kprobes.tc

A preliminary analysis shows that the issue is related with

  echo 1 > events/kprobes/enable

after add the 256 probe points.

In order to find the root cause, I want to verify which probe point has
problem, so it is necessary to save kprobe_events to test log.

With this patch, we can get the 256 probe points in the test log through
the following command:

  # ./ftracetest test.d/kprobe/multiple_kprobes.tc -vvv -k

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
index 312d237..be754f5 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
@@ -25,6 +25,8 @@ if [ $L -ne 256 ]; then
   exit_fail
 fi
 
+cat kprobe_events >> $testlog
+
 echo 1 > events/kprobes/enable
 echo 0 > events/kprobes/enable
 echo > kprobe_events
-- 
2.1.0

[PATCH v3 2/2] MIPS: Use NOKPROBE_SYMBOL() instead of __kprobes annotation

[Tiezhu Yang]

If define CONFIG_KPROBES, __kprobes annotation forces the whole function
into the ".kprobes.text" section, NOKPROBE_SYMBOL() only stores the given
function address in the "_kprobe_blacklist" section which is introduced
to maintain kprobes blacklist.

Modify the related code to use NOKPROBE_SYMBOL() to protect functions from
kprobes instead of __kprobes annotation under arch/mips.

No obvious functional change in this patch, some more work needs to be done
to fix the kernel panic when execute the following testcase on mips:

  # cd tools/testing/selftests/ftrace
  # ./ftracetest test.d/kprobe/multiple_kprobes.tc

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/kprobes.c | 36 ++++++++++++++++++++++++------------
 arch/mips/mm/fault.c       |  6 ++++--
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
index 6c7f3b1..316b27d 100644
--- a/arch/mips/kernel/kprobes.c
+++ b/arch/mips/kernel/kprobes.c
@@ -44,10 +44,11 @@ static const union mips_instruction breakpoint2_insn = {
 DEFINE_PER_CPU(struct kprobe *, current_kprobe);
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
-static int __kprobes insn_has_delayslot(union mips_instruction insn)
+static int insn_has_delayslot(union mips_instruction insn)
 {
 	return __insn_has_delay_slot(insn);
 }
+NOKPROBE_SYMBOL(insn_has_delayslot);
 
 /*
  * insn_has_ll_or_sc function checks whether instruction is ll or sc
@@ -56,7 +57,7 @@ static int __kprobes insn_has_delayslot(union mips_instruction insn)
  * instructions; cannot do much about breakpoint in the middle of
  * ll/sc pair; it is upto user to avoid those places
  */
-static int __kprobes insn_has_ll_or_sc(union mips_instruction insn)
+static int insn_has_ll_or_sc(union mips_instruction insn)
 {
 	int ret = 0;
 
@@ -72,8 +73,9 @@ static int __kprobes insn_has_ll_or_sc(union mips_instruction insn)
 	}
 	return ret;
 }
+NOKPROBE_SYMBOL(insn_has_ll_or_sc);
 
-int __kprobes arch_prepare_kprobe(struct kprobe *p)
+int arch_prepare_kprobe(struct kprobe *p)
 {
 	union mips_instruction insn;
 	union mips_instruction prev_insn;
@@ -132,26 +134,30 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 out:
 	return ret;
 }
+NOKPROBE_SYMBOL(arch_prepare_kprobe);
 
-void __kprobes arch_arm_kprobe(struct kprobe *p)
+void arch_arm_kprobe(struct kprobe *p)
 {
 	*p->addr = breakpoint_insn;
 	flush_insn_slot(p);
 }
+NOKPROBE_SYMBOL(arch_arm_kprobe);
 
-void __kprobes arch_disarm_kprobe(struct kprobe *p)
+void arch_disarm_kprobe(struct kprobe *p)
 {
 	*p->addr = p->opcode;
 	flush_insn_slot(p);
 }
+NOKPROBE_SYMBOL(arch_disarm_kprobe);
 
-void __kprobes arch_remove_kprobe(struct kprobe *p)
+void arch_remove_kprobe(struct kprobe *p)
 {
 	if (p->ainsn.insn) {
 		free_insn_slot(p->ainsn.insn, 0);
 		p->ainsn.insn = NULL;
 	}
 }
+NOKPROBE_SYMBOL(arch_remove_kprobe);
 
 static void save_previous_kprobe(struct kprobe_ctlblk *kcb)
 {
@@ -257,7 +263,7 @@ static void prepare_singlestep(struct kprobe *p, struct pt_regs *regs,
  * breakpoint trap. In case of branch instructions, the target
  * epc to be restored.
  */
-static void __kprobes resume_execution(struct kprobe *p,
+static void resume_execution(struct kprobe *p,
 				       struct pt_regs *regs,
 				       struct kprobe_ctlblk *kcb)
 {
@@ -268,8 +274,9 @@ static void __kprobes resume_execution(struct kprobe *p,
 		regs->cp0_epc = orig_epc + 4;
 	}
 }
+NOKPROBE_SYMBOL(resume_execution);
 
-static int __kprobes kprobe_handler(struct pt_regs *regs)
+static int kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *p;
 	int ret = 0;
@@ -367,6 +374,7 @@ static int __kprobes kprobe_handler(struct pt_regs *regs)
 	return ret;
 
 }
+NOKPROBE_SYMBOL(kprobe_handler);
 
 static inline int post_kprobe_handler(struct pt_regs *regs)
 {
@@ -415,7 +423,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 /*
  * Wrapper routine for handling exceptions.
  */
-int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
+int kprobe_exceptions_notify(struct notifier_block *self,
 				       unsigned long val, void *data)
 {
 
@@ -446,6 +454,7 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
 	}
 	return ret;
 }
+NOKPROBE_SYMBOL(kprobe_exceptions_notify);
 
 /*
  * Function return probe trampoline:
@@ -469,7 +478,7 @@ static void __used kretprobe_trampoline_holder(void)
 
 void __kretprobe_trampoline(void);
 
-void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
+void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
 	ri->ret_addr = (kprobe_opcode_t *) regs->regs[31];
@@ -478,11 +487,12 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 	/* Replace the return addr with trampoline addr */
 	regs->regs[31] = (unsigned long)__kretprobe_trampoline;
 }
+NOKPROBE_SYMBOL(arch_prepare_kretprobe);
 
 /*
  * Called when the probe at kretprobe trampoline is hit
  */
-static int __kprobes trampoline_probe_handler(struct kprobe *p,
+static int trampoline_probe_handler(struct kprobe *p,
 						struct pt_regs *regs)
 {
 	instruction_pointer(regs) = __kretprobe_trampoline_handler(regs, NULL);
@@ -493,14 +503,16 @@ static int __kprobes trampoline_probe_handler(struct kprobe *p,
 	 */
 	return 1;
 }
+NOKPROBE_SYMBOL(trampoline_probe_handler);
 
-int __kprobes arch_trampoline_kprobe(struct kprobe *p)
+int arch_trampoline_kprobe(struct kprobe *p)
 {
 	if (p->addr == (kprobe_opcode_t *)__kretprobe_trampoline)
 		return 1;
 
 	return 0;
 }
+NOKPROBE_SYMBOL(arch_trampoline_kprobe);
 
 static struct kprobe trampoline_p = {
 	.addr = (kprobe_opcode_t *)__kretprobe_trampoline,
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index 44f9810..b08bc55 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -35,7 +35,7 @@ int show_unhandled_signals = 1;
  * and the problem, and then passes it off to one of the appropriate
  * routines.
  */
-static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
+static void __do_page_fault(struct pt_regs *regs, unsigned long write,
 	unsigned long address)
 {
 	struct vm_area_struct * vma = NULL;
@@ -322,8 +322,9 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
 	}
 #endif
 }
+NOKPROBE_SYMBOL(__do_page_fault);
 
-asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
+asmlinkage void do_page_fault(struct pt_regs *regs,
 	unsigned long write, unsigned long address)
 {
 	enum ctx_state prev_state;
@@ -332,3 +333,4 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 	__do_page_fault(regs, write, address);
 	exception_exit(prev_state);
 }
+NOKPROBE_SYMBOL(do_page_fault);
-- 
2.1.0

Re: [PATCH v3 2/2] MIPS: Use NOKPROBE_SYMBOL() instead of __kprobes annotation

[Masami Hiramatsu]

On Fri,  6 May 2022 19:02:03 +0800
Tiezhu Yang <yangtiezhu@loongson.cn> wrote:

> If define CONFIG_KPROBES, __kprobes annotation forces the whole function
> into the ".kprobes.text" section, NOKPROBE_SYMBOL() only stores the given
> function address in the "_kprobe_blacklist" section which is introduced
> to maintain kprobes blacklist.
> 
> Modify the related code to use NOKPROBE_SYMBOL() to protect functions from
> kprobes instead of __kprobes annotation under arch/mips.

OK, this looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

> 
> No obvious functional change in this patch, some more work needs to be done
> to fix the kernel panic when execute the following testcase on mips:
> 
>   # cd tools/testing/selftests/ftrace
>   # ./ftracetest test.d/kprobe/multiple_kprobes.tc

Hm, I recommend you to manually bisecting the candidates.

1) list up the problematic symbols ( Use --console option to ftracetest so that
you can get the list of symbols via console before panic)
2) split the symbols into two groups
3) manually tests the groups, e.g. 
  $ cat group.list | while read sym; do echo p:$sym >> kprobe_events; done; \
    echo 1 > events/kprobes/enable 
4) if you find one group causes the panic, split it into two groups and repeat 3.

Then, you can find the symbols to be annotated.

Thank you,

> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/mips/kernel/kprobes.c | 36 ++++++++++++++++++++++++------------
>  arch/mips/mm/fault.c       |  6 ++++--
>  2 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
> index 6c7f3b1..316b27d 100644
> --- a/arch/mips/kernel/kprobes.c
> +++ b/arch/mips/kernel/kprobes.c
> @@ -44,10 +44,11 @@ static const union mips_instruction breakpoint2_insn = {
>  DEFINE_PER_CPU(struct kprobe *, current_kprobe);
>  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  
> -static int __kprobes insn_has_delayslot(union mips_instruction insn)
> +static int insn_has_delayslot(union mips_instruction insn)
>  {
>  	return __insn_has_delay_slot(insn);
>  }
> +NOKPROBE_SYMBOL(insn_has_delayslot);
>  
>  /*
>   * insn_has_ll_or_sc function checks whether instruction is ll or sc
> @@ -56,7 +57,7 @@ static int __kprobes insn_has_delayslot(union mips_instruction insn)
>   * instructions; cannot do much about breakpoint in the middle of
>   * ll/sc pair; it is upto user to avoid those places
>   */
> -static int __kprobes insn_has_ll_or_sc(union mips_instruction insn)
> +static int insn_has_ll_or_sc(union mips_instruction insn)
>  {
>  	int ret = 0;
>  
> @@ -72,8 +73,9 @@ static int __kprobes insn_has_ll_or_sc(union mips_instruction insn)
>  	}
>  	return ret;
>  }
> +NOKPROBE_SYMBOL(insn_has_ll_or_sc);
>  
> -int __kprobes arch_prepare_kprobe(struct kprobe *p)
> +int arch_prepare_kprobe(struct kprobe *p)
>  {
>  	union mips_instruction insn;
>  	union mips_instruction prev_insn;
> @@ -132,26 +134,30 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  out:
>  	return ret;
>  }
> +NOKPROBE_SYMBOL(arch_prepare_kprobe);
>  
> -void __kprobes arch_arm_kprobe(struct kprobe *p)
> +void arch_arm_kprobe(struct kprobe *p)
>  {
>  	*p->addr = breakpoint_insn;
>  	flush_insn_slot(p);
>  }
> +NOKPROBE_SYMBOL(arch_arm_kprobe);
>  
> -void __kprobes arch_disarm_kprobe(struct kprobe *p)
> +void arch_disarm_kprobe(struct kprobe *p)
>  {
>  	*p->addr = p->opcode;
>  	flush_insn_slot(p);
>  }
> +NOKPROBE_SYMBOL(arch_disarm_kprobe);
>  
> -void __kprobes arch_remove_kprobe(struct kprobe *p)
> +void arch_remove_kprobe(struct kprobe *p)
>  {
>  	if (p->ainsn.insn) {
>  		free_insn_slot(p->ainsn.insn, 0);
>  		p->ainsn.insn = NULL;
>  	}
>  }
> +NOKPROBE_SYMBOL(arch_remove_kprobe);
>  
>  static void save_previous_kprobe(struct kprobe_ctlblk *kcb)
>  {
> @@ -257,7 +263,7 @@ static void prepare_singlestep(struct kprobe *p, struct pt_regs *regs,
>   * breakpoint trap. In case of branch instructions, the target
>   * epc to be restored.
>   */
> -static void __kprobes resume_execution(struct kprobe *p,
> +static void resume_execution(struct kprobe *p,
>  				       struct pt_regs *regs,
>  				       struct kprobe_ctlblk *kcb)
>  {
> @@ -268,8 +274,9 @@ static void __kprobes resume_execution(struct kprobe *p,
>  		regs->cp0_epc = orig_epc + 4;
>  	}
>  }
> +NOKPROBE_SYMBOL(resume_execution);
>  
> -static int __kprobes kprobe_handler(struct pt_regs *regs)
> +static int kprobe_handler(struct pt_regs *regs)
>  {
>  	struct kprobe *p;
>  	int ret = 0;
> @@ -367,6 +374,7 @@ static int __kprobes kprobe_handler(struct pt_regs *regs)
>  	return ret;
>  
>  }
> +NOKPROBE_SYMBOL(kprobe_handler);
>  
>  static inline int post_kprobe_handler(struct pt_regs *regs)
>  {
> @@ -415,7 +423,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>  /*
>   * Wrapper routine for handling exceptions.
>   */
> -int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
> +int kprobe_exceptions_notify(struct notifier_block *self,
>  				       unsigned long val, void *data)
>  {
>  
> @@ -446,6 +454,7 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
>  	}
>  	return ret;
>  }
> +NOKPROBE_SYMBOL(kprobe_exceptions_notify);
>  
>  /*
>   * Function return probe trampoline:
> @@ -469,7 +478,7 @@ static void __used kretprobe_trampoline_holder(void)
>  
>  void __kretprobe_trampoline(void);
>  
> -void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> +void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  				      struct pt_regs *regs)
>  {
>  	ri->ret_addr = (kprobe_opcode_t *) regs->regs[31];
> @@ -478,11 +487,12 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  	/* Replace the return addr with trampoline addr */
>  	regs->regs[31] = (unsigned long)__kretprobe_trampoline;
>  }
> +NOKPROBE_SYMBOL(arch_prepare_kretprobe);
>  
>  /*
>   * Called when the probe at kretprobe trampoline is hit
>   */
> -static int __kprobes trampoline_probe_handler(struct kprobe *p,
> +static int trampoline_probe_handler(struct kprobe *p,
>  						struct pt_regs *regs)
>  {
>  	instruction_pointer(regs) = __kretprobe_trampoline_handler(regs, NULL);
> @@ -493,14 +503,16 @@ static int __kprobes trampoline_probe_handler(struct kprobe *p,
>  	 */
>  	return 1;
>  }
> +NOKPROBE_SYMBOL(trampoline_probe_handler);
>  
> -int __kprobes arch_trampoline_kprobe(struct kprobe *p)
> +int arch_trampoline_kprobe(struct kprobe *p)
>  {
>  	if (p->addr == (kprobe_opcode_t *)__kretprobe_trampoline)
>  		return 1;
>  
>  	return 0;
>  }
> +NOKPROBE_SYMBOL(arch_trampoline_kprobe);
>  
>  static struct kprobe trampoline_p = {
>  	.addr = (kprobe_opcode_t *)__kretprobe_trampoline,
> diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
> index 44f9810..b08bc55 100644
> --- a/arch/mips/mm/fault.c
> +++ b/arch/mips/mm/fault.c
> @@ -35,7 +35,7 @@ int show_unhandled_signals = 1;
>   * and the problem, and then passes it off to one of the appropriate
>   * routines.
>   */
> -static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
> +static void __do_page_fault(struct pt_regs *regs, unsigned long write,
>  	unsigned long address)
>  {
>  	struct vm_area_struct * vma = NULL;
> @@ -322,8 +322,9 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
>  	}
>  #endif
>  }
> +NOKPROBE_SYMBOL(__do_page_fault);
>  
> -asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
> +asmlinkage void do_page_fault(struct pt_regs *regs,
>  	unsigned long write, unsigned long address)
>  {
>  	enum ctx_state prev_state;
> @@ -332,3 +333,4 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
>  	__do_page_fault(regs, write, address);
>  	exception_exit(prev_state);
>  }
> +NOKPROBE_SYMBOL(do_page_fault);
> -- 
> 2.1.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 0/2] Modify some code about kprobe

[Thomas Bogendoerfer]

On Fri, May 06, 2022 at 07:02:01PM +0800, Tiezhu Yang wrote:
> v3: no need to change post_kprobe_handler() in patch #2 now,
>     sorry for the carelessness.
> 
> v2:
>   -- only replace __kprobes with NOKPROBE_SYMBOL() in patch #2
>   -- update the commit messages
> 
> Tiezhu Yang (2):
>   selftests/ftrace: Save kprobe_events to test log
>   MIPS: Use NOKPROBE_SYMBOL() instead of __kprobes annotation
> 
>  arch/mips/kernel/kprobes.c                         | 36 ++++++++++++++--------
>  arch/mips/mm/fault.c                               |  6 ++--
>  .../ftrace/test.d/kprobe/multiple_kprobes.tc       |  2 ++
>  3 files changed, 30 insertions(+), 14 deletions(-)
> 
> -- 
> 2.1.0

seires applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]