Messages in this thread | | | From | Doug Anderson <> | Date | Wed, 13 May 2020 17:21:38 -0700 | Subject | Re: [PATCH 2/4] arm64: Extract kprobes_save_local_irqflag() and kprobes_restore_local_irqflag() |
| |
Hi,
On Sat, May 9, 2020 at 6:49 AM 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. > > 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);
nit: why not just return unsigned long rather than passing by reference?
> + > +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);
Is there some reason to have two functions? It seems like every time you call kernel_enable_single_step() you'd want to call kernel_prepare_single_step(). ...and every time you call kernel_disable_single_step() you'd want to call kernel_cleanup_single_step().
Maybe you can just add the flags parameter to kernel_enable_single_step() / kernel_disable_single_step() and put the code in there?
-Doug
| |