Messages in this thread | | | Date | Tue, 10 Sep 2019 16:14:17 +0530 | From | "Naveen N. Rao" <> | Subject | Re: [PATCH 2/2] powerpc/watchpoint: Disable watchpoint hit by larx/stcx instructions |
| |
Ravi Bangoria wrote: > If watchpoint exception is generated by larx/stcx instructions, the > reservation created by larx gets lost while handling exception, and > thus stcx instruction always fails. Generally these instructions are > used in a while(1) loop, for example spinlocks. And because stcx > never succeeds, it loops forever and ultimately hangs the system. > > Note that ptrace anyway works in one-shot mode and thus for ptrace > we don't change the behaviour. It's up to ptrace user to take care > of this. > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > --- > arch/powerpc/kernel/hw_breakpoint.c | 49 +++++++++++++++++++---------- > 1 file changed, 33 insertions(+), 16 deletions(-) > > diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c > index 28ad3171bb82..9fa496a598ce 100644 > --- a/arch/powerpc/kernel/hw_breakpoint.c > +++ b/arch/powerpc/kernel/hw_breakpoint.c > @@ -195,14 +195,32 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs) > tsk->thread.last_hit_ubp = NULL; > } > > +static bool is_larx_stcx_instr(struct pt_regs *regs, unsigned int instr) > +{ > + int ret, type; > + struct instruction_op op; > + > + ret = analyse_instr(&op, regs, instr); > + type = GETTYPE(op.type); > + return (!ret && (type == LARX || type == STCX)); > +} > + > /* > * Handle debug exception notifications. > */ > static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp, > unsigned long addr) > { > - int stepped; > - unsigned int instr; > + unsigned int instr = 0; > + > + if (__get_user_inatomic(instr, (unsigned int *)regs->nip)) > + goto fail; > + > + if (is_larx_stcx_instr(regs, instr)) { > + printk_ratelimited("Watchpoint: Can't emulate/single-step larx/" > + "stcx instructions. Disabling watchpoint.\n");
The below WARN() uses the term 'breakpoint'. Better to use consistent terminology. I would rewrite the above as: printk_ratelimited("Breakpoint hit on instruction that can't be emulated. " "Breakpoint at 0x%lx will be disabled.\n", addr);
Otherwise: Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
- Naveen
> + goto disable; > + } > > /* Do not emulate user-space instructions, instead single-step them */ > if (user_mode(regs)) { > @@ -211,23 +229,22 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp, > return false; > } > > - stepped = 0; > - instr = 0; > - if (!__get_user_inatomic(instr, (unsigned int *)regs->nip)) > - stepped = emulate_step(regs, instr); > + if (!emulate_step(regs, instr)) > + goto fail; > > + return true; > + > +fail: > /* > - * emulate_step() could not execute it. We've failed in reliably > - * handling the hw-breakpoint. Unregister it and throw a warning > - * message to let the user know about it. > + * We've failed in reliably handling the hw-breakpoint. Unregister > + * it and throw a warning message to let the user know about it. > */ > - if (!stepped) { > - WARN(1, "Unable to handle hardware breakpoint. Breakpoint at " > - "0x%lx will be disabled.", addr); > - perf_event_disable_inatomic(bp); > - return false; > - } > - return true; > + WARN(1, "Unable to handle hardware breakpoint. Breakpoint at " > + "0x%lx will be disabled.", addr); > + > +disable: > + perf_event_disable_inatomic(bp); > + return false; > } > > int hw_breakpoint_handler(struct die_args *args) > -- > 2.21.0 > >
| |