From: Masami Hiramatsu <mhiramat@kernel.org>
To: Wei Li <liwei391@huawei.com>
Cc: <daniel.thompson@linaro.org>, <jason.wessel@windriver.com>,
<dianders@chromium.org>, <maz@kernel.org>, <mark.rutland@arm.com>,
<davem@davemloft.net>, <will@kernel.org>,
<catalin.marinas@arm.com>, <linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <liwei1412@163.com>
Subject: Re: [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag()
Date: Sun, 10 May 2020 17:59:38 +0900 [thread overview]
Message-ID: <20200510175938.7c888bd01f82d29203995c63@kernel.org> (raw)
In-Reply-To: <20200509214159.19680-3-liwei391@huawei.com>
Hi Wei,
On Sun, 10 May 2020 05:41:57 +0800
Wei Li <liwei391@huawei.com> wrote:
> PSTATE.I and PSTATE.D are very important for single-step working.
>
> Without disabling interrupt on local CPU, there is a chance of
> interrupt occurrence in the period of exception return and start of
> out-of-line single-step, that result in wrongly single stepping
> into the interrupt handler. And if D bit is set then, it results into
> undefined exception and when it's handler enables dbg then single-step
> exception is generated, not as expected.
>
> As they are maintained well in kprobes_save_local_irqflag() and
> kprobes_restore_local_irqflag() for kprobe module, extract them as
> kernel_prepare_single_step() and kernel_cleanup_single_step() for
> general use.
Agreed, these prepare/cleanup should be generic.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you!
>
> Signed-off-by: Wei Li <liwei391@huawei.com>
> ---
> arch/arm64/include/asm/debug-monitors.h | 4 ++++
> arch/arm64/kernel/debug-monitors.c | 26 +++++++++++++++++++++++
> arch/arm64/kernel/probes/kprobes.c | 28 ++-----------------------
> 3 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 7619f473155f..b62469f3475b 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -113,6 +113,10 @@ void user_fastforward_single_step(struct task_struct *task);
> void kernel_enable_single_step(struct pt_regs *regs);
> void kernel_disable_single_step(void);
> int kernel_active_single_step(void);
> +void kernel_prepare_single_step(unsigned long *flags,
> + struct pt_regs *regs);
> +void kernel_cleanup_single_step(unsigned long flags,
> + struct pt_regs *regs);
>
> #ifdef CONFIG_HAVE_HW_BREAKPOINT
> int reinstall_suspended_bps(struct pt_regs *regs);
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 48222a4760c2..25ce6b5a52d2 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -429,6 +429,32 @@ int kernel_active_single_step(void)
> }
> NOKPROBE_SYMBOL(kernel_active_single_step);
>
> +/*
> + * Interrupts need to be disabled before single-step mode is set, and not
> + * reenabled until after single-step mode ends.
> + * Without disabling interrupt on local CPU, there is a chance of
> + * interrupt occurrence in the period of exception return and start of
> + * out-of-line single-step, that result in wrongly single stepping
> + * into the interrupt handler.
> + */
> +void kernel_prepare_single_step(unsigned long *flags,
> + struct pt_regs *regs)
> +{
> + *flags = regs->pstate & DAIF_MASK;
> + regs->pstate |= PSR_I_BIT;
> + /* Unmask PSTATE.D for enabling software step exceptions. */
> + regs->pstate &= ~PSR_D_BIT;
> +}
> +NOKPROBE_SYMBOL(kernel_prepare_single_step);
> +
> +void kernel_cleanup_single_step(unsigned long flags,
> + struct pt_regs *regs)
> +{
> + regs->pstate &= ~DAIF_MASK;
> + regs->pstate |= flags;
> +}
> +NOKPROBE_SYMBOL(kernel_cleanup_single_step);
> +
> /* ptrace API */
> void user_enable_single_step(struct task_struct *task)
> {
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d1c95dcf1d78..c655b6b543e3 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -168,30 +168,6 @@ static void __kprobes set_current_kprobe(struct kprobe *p)
> __this_cpu_write(current_kprobe, p);
> }
>
> -/*
> - * Interrupts need to be disabled before single-step mode is set, and not
> - * reenabled until after single-step mode ends.
> - * Without disabling interrupt on local CPU, there is a chance of
> - * interrupt occurrence in the period of exception return and start of
> - * out-of-line single-step, that result in wrongly single stepping
> - * into the interrupt handler.
> - */
> -static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
> - struct pt_regs *regs)
> -{
> - kcb->saved_irqflag = regs->pstate & DAIF_MASK;
> - regs->pstate |= PSR_I_BIT;
> - /* Unmask PSTATE.D for enabling software step exceptions. */
> - regs->pstate &= ~PSR_D_BIT;
> -}
> -
> -static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
> - struct pt_regs *regs)
> -{
> - regs->pstate &= ~DAIF_MASK;
> - regs->pstate |= kcb->saved_irqflag;
> -}
> -
> static void __kprobes
> set_ss_context(struct kprobe_ctlblk *kcb, unsigned long addr)
> {
> @@ -227,7 +203,7 @@ static void __kprobes setup_singlestep(struct kprobe *p,
> set_ss_context(kcb, slot); /* mark pending ss */
>
> /* IRQs and single stepping do not mix well. */
> - kprobes_save_local_irqflag(kcb, regs);
> + kernel_prepare_single_step(&kcb->saved_irqflag, regs);
> kernel_enable_single_step(regs);
> instruction_pointer_set(regs, slot);
> } else {
> @@ -414,7 +390,7 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
>
> if (retval == DBG_HOOK_HANDLED) {
> - kprobes_restore_local_irqflag(kcb, regs);
> + kernel_cleanup_single_step(kcb->saved_irqflag, regs);
> kernel_disable_single_step();
>
> post_kprobe_handler(kcb, regs);
> --
> 2.17.1
>
--
Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2020-05-10 8:59 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-09 21:41 [PATCH 0/4] arm64: kgdb/kdb: Fix single-step debugging issues Wei Li
2020-05-09 21:41 ` [PATCH 1/4] arm64: kgdb: Fix single-step exception handling oops Wei Li
2020-05-14 0:21 ` Doug Anderson
2020-05-09 21:41 ` [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag() Wei Li
2020-05-10 8:59 ` Masami Hiramatsu [this message]
2020-05-14 0:21 ` Doug Anderson
2020-05-16 8:47 ` liwei (GF)
2020-05-16 16:17 ` Doug Anderson
2020-05-18 15:14 ` Masami Hiramatsu
2020-05-09 21:41 ` [PATCH 3/4] arm64: kgdb: Fix single-stepping into the irq handler wrongly Wei Li
2020-05-14 0:21 ` Doug Anderson
2020-05-09 21:41 ` [PATCH 4/4] arm64: kgdb: Set PSTATE.SS to 1 to reenable single-step Wei Li
2020-05-14 0:23 ` Doug Anderson
2020-05-16 8:20 ` liwei (GF)
2020-05-14 0:34 ` [PATCH 0/4] arm64: kgdb/kdb: Fix single-step debugging issues Doug Anderson
2020-05-16 8:20 ` liwei (GF)
2020-06-29 21:20 ` Doug Anderson
2020-06-30 7:22 ` Will Deacon
2020-07-06 21:37 ` Doug Anderson
2020-07-08 22:06 ` Will Deacon
2020-07-08 22:22 ` Doug Anderson
2020-07-07 1:37 ` liwei (GF)
2020-07-08 22:02 ` Will Deacon
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=20200510175938.7c888bd01f82d29203995c63@kernel.org \
--to=mhiramat@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=daniel.thompson@linaro.org \
--cc=davem@davemloft.net \
--cc=dianders@chromium.org \
--cc=jason.wessel@windriver.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liwei1412@163.com \
--cc=liwei391@huawei.com \
--cc=mark.rutland@arm.com \
--cc=maz@kernel.org \
--cc=will@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).