On Fri, 6 May 2022 19:02:03 +0800
Tiezhu Yang <yangtiezhu@xxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
