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(-)
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
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);
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
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.
linux-kselftest-mirror@lists.linaro.org
-
Masami Hiramatsu -
Thomas Bogendoerfer -
Tiezhu Yang