lkml.org 
[lkml]   [2019]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>
>

\
 
 \ /
  Last update: 2019-09-10 12:45    [W:0.038 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site