From: Masami Hiramatsu <mhiramat@kernel.org>
To: Tiezhu Yang <yangtiezhu@loongson.cn>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
Steven Rostedt <rostedt@goodmis.org>,
Shuah Khan <shuah@kernel.org>, Xuefeng Li <lixuefeng@loongson.cn>,
linux-mips@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] MIPS: Use NOKPROBE_SYMBOL() instead of __kprobes annotation
Date: Fri, 6 May 2022 10:45:04 +0900 [thread overview]
Message-ID: <20220506104504.535c6ab065993b97604178fe@kernel.org> (raw)
In-Reply-To: <1651753148-1464-3-git-send-email-yangtiezhu@loongson.cn>
On Thu, 5 May 2022 20:19:08 +0800
Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> If define CONFIG_KPROBES, __kprobes annotation forces the whole function
> into the ".kprobes.text" section, NOKPROBE_SYMBOL() just 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.
So you added some non '__kprobes' annotated functions to NOKPROBE_SYMBOL()
in this patch. Those caused the kernel panic, right? If so, please add such
comment on this description too. Or, split this into 2 patches, one fixes
the kernel panic by adding those functions to NOKPROBE_SYMBOL() and the
other is replacing __kprobes with NOKPROBE_SYMBOL().
Also, could you also find the commit which introduces the kernel panic?
It is worth to backport such fix to stable trees.
Thank you,
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
> arch/mips/kernel/kprobes.c | 45 ++++++++++++++++++++++++++++++++-------------
> arch/mips/mm/fault.c | 6 ++++--
> 2 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
> index 6c7f3b1..21f9cec 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)
> {
> @@ -161,6 +167,7 @@ static void save_previous_kprobe(struct kprobe_ctlblk *kcb)
> kcb->prev_kprobe.saved_SR = kcb->kprobe_saved_SR;
> kcb->prev_kprobe.saved_epc = kcb->kprobe_saved_epc;
> }
> +NOKPROBE_SYMBOL(save_previous_kprobe);
>
> static void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
> {
> @@ -170,6 +177,7 @@ static void restore_previous_kprobe(struct kprobe_ctlblk *kcb)
> kcb->kprobe_saved_SR = kcb->prev_kprobe.saved_SR;
> kcb->kprobe_saved_epc = kcb->prev_kprobe.saved_epc;
> }
> +NOKPROBE_SYMBOL(restore_previous_kprobe);
>
> static void set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
> struct kprobe_ctlblk *kcb)
> @@ -178,6 +186,7 @@ static void set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
> kcb->kprobe_saved_SR = kcb->kprobe_old_SR = (regs->cp0_status & ST0_IE);
> kcb->kprobe_saved_epc = regs->cp0_epc;
> }
> +NOKPROBE_SYMBOL(set_current_kprobe);
>
> /**
> * evaluate_branch_instrucion -
> @@ -225,6 +234,7 @@ static int evaluate_branch_instruction(struct kprobe *p, struct pt_regs *regs,
> return -EFAULT;
>
> }
> +NOKPROBE_SYMBOL(evaluate_branch_instruction);
>
> static void prepare_singlestep(struct kprobe *p, struct pt_regs *regs,
> struct kprobe_ctlblk *kcb)
> @@ -244,6 +254,7 @@ static void prepare_singlestep(struct kprobe *p, struct pt_regs *regs,
> }
> regs->cp0_epc = (unsigned long)&p->ainsn.insn[0];
> }
> +NOKPROBE_SYMBOL(prepare_singlestep);
>
> /*
> * Called after single-stepping. p->addr is the address of the
> @@ -257,7 +268,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 +279,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,8 +379,9 @@ 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)
> +static int post_kprobe_handler(struct pt_regs *regs)
> {
> struct kprobe *cur = kprobe_running();
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> @@ -396,6 +409,7 @@ static inline int post_kprobe_handler(struct pt_regs *regs)
>
> return 1;
> }
> +NOKPROBE_SYMBOL(post_kprobe_handler);
>
> int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
> {
> @@ -411,11 +425,12 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
> }
> return 0;
> }
> +NOKPROBE_SYMBOL(kprobe_fault_handler);
>
> /*
> * 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 +461,7 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
> }
> return ret;
> }
> +NOKPROBE_SYMBOL(kprobe_exceptions_notify);
>
> /*
> * Function return probe trampoline:
> @@ -469,7 +485,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 +494,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 +510,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>
next prev parent reply other threads:[~2022-05-06 1:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-05 12:19 [PATCH 0/2] Modify some code about kprobe Tiezhu Yang
2022-05-05 12:19 ` [PATCH 1/2] selftests/ftrace: Save kprobe_events to test log Tiezhu Yang
2022-05-06 1:46 ` Masami Hiramatsu
2022-05-05 12:19 ` [PATCH 2/2] MIPS: Use NOKPROBE_SYMBOL() instead of __kprobes annotation Tiezhu Yang
2022-05-06 1:45 ` Masami Hiramatsu [this message]
2022-05-06 3:24 ` Tiezhu Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220506104504.535c6ab065993b97604178fe@kernel.org \
--to=mhiramat@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=lixuefeng@loongson.cn \
--cc=rostedt@goodmis.org \
--cc=shuah@kernel.org \
--cc=tsbogend@alpha.franken.de \
--cc=yangtiezhu@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.