| Message ID | 20200902042945.129369-6-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5b905d77987de065bdd3a2906816b5f143df087b |
| Headers | show |
| Series | powerpc/watchpoint: Bug fixes plus new feature flag | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (35f066fda170dde0a31f1447547a5d30b83c3920) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 89 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
On 9/2/20 1:29 AM, Ravi Bangoria wrote: > On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel > disables event every time it fires and user has to re-enable it. > Also, in case of ptrace watchpoint, kernel notifies ptrace user > before executing instruction. > > With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable > ptrace event and thus it's causing infinite loop of exceptions. > This is especially harmful when user watches on a data which is > also read/written by kernel, eg syscall parameters. In such case, > infinite exceptions happens in kernel mode which causes soft-lockup. > > Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers") > Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> Tested-by: Rogerio Alves <rcardoso@linux.ibm.com> > --- > arch/powerpc/include/asm/hw_breakpoint.h | 3 ++ > arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++ > arch/powerpc/kernel/ptrace/ptrace-noadv.c | 4 +- > 3 files changed, 54 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > index 81872c420476..abebfbee5b1c 100644 > --- a/arch/powerpc/include/asm/hw_breakpoint.h > +++ b/arch/powerpc/include/asm/hw_breakpoint.h > @@ -18,6 +18,7 @@ struct arch_hw_breakpoint { > u16 type; > u16 len; /* length of the target data symbol */ > u16 hw_len; /* length programmed in hw */ > + u8 flags; > }; > > /* Note: Don't change the first 6 bits below as they are in the same order > @@ -37,6 +38,8 @@ struct arch_hw_breakpoint { > #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \ > HW_BRK_TYPE_HYP) > > +#define HW_BRK_FLAG_DISABLED 0x1 > + > /* Minimum granularity */ > #ifdef CONFIG_PPC_8xx > #define HW_BREAKPOINT_SIZE 0x4 > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 016bd831908e..160fbbf41d40 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address, > (void __user *)address); > } > #else /* !CONFIG_PPC_ADV_DEBUG_REGS */ > + > +static void do_break_handler(struct pt_regs *regs) > +{ > + struct arch_hw_breakpoint null_brk = {0}; > + struct arch_hw_breakpoint *info; > + struct ppc_inst instr = ppc_inst(0); > + int type = 0; > + int size = 0; > + unsigned long ea; > + int i; > + > + /* > + * If underneath hw supports only one watchpoint, we know it > + * caused exception. 8xx also falls into this category. > + */ > + if (nr_wp_slots() == 1) { > + __set_breakpoint(0, &null_brk); > + current->thread.hw_brk[0] = null_brk; > + current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED; > + return; > + } > + > + /* Otherwise findout which DAWR caused exception and disable it. */ > + wp_get_instr_detail(regs, &instr, &type, &size, &ea); > + > + for (i = 0; i < nr_wp_slots(); i++) { > + info = ¤t->thread.hw_brk[i]; > + if (!info->address) > + continue; > + > + if (wp_check_constraints(regs, instr, ea, type, size, info)) { > + __set_breakpoint(i, &null_brk); > + current->thread.hw_brk[i] = null_brk; > + current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED; > + } > + } > +} > + > void do_break (struct pt_regs *regs, unsigned long address, > unsigned long error_code) > { > @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address, > if (debugger_break_match(regs)) > return; > > + /* > + * We reach here only when watchpoint exception is generated by ptrace > + * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set, > + * watchpoint is already handled by hw_breakpoint_handler() so we don't > + * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set, > + * we need to manually handle the watchpoint here. > + */ > + if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT)) > + do_break_handler(regs); > + > /* Deliver the signal to userspace */ > force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address); > } > diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c > index 57a0ab822334..c9122ed91340 100644 > --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c > +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c > @@ -286,11 +286,13 @@ long ppc_del_hwdebug(struct task_struct *child, long data) > } > return ret; > #else /* CONFIG_HAVE_HW_BREAKPOINT */ > - if (child->thread.hw_brk[data - 1].address == 0) > + if (!(child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) && > + child->thread.hw_brk[data - 1].address == 0) > return -ENOENT; > > child->thread.hw_brk[data - 1].address = 0; > child->thread.hw_brk[data - 1].type = 0; > + child->thread.hw_brk[data - 1].flags = 0; > #endif /* CONFIG_HAVE_HW_BREAKPOINT */ > > return 0; >
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h index 81872c420476..abebfbee5b1c 100644 --- a/arch/powerpc/include/asm/hw_breakpoint.h +++ b/arch/powerpc/include/asm/hw_breakpoint.h @@ -18,6 +18,7 @@ struct arch_hw_breakpoint { u16 type; u16 len; /* length of the target data symbol */ u16 hw_len; /* length programmed in hw */ + u8 flags; }; /* Note: Don't change the first 6 bits below as they are in the same order @@ -37,6 +38,8 @@ struct arch_hw_breakpoint { #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \ HW_BRK_TYPE_HYP) +#define HW_BRK_FLAG_DISABLED 0x1 + /* Minimum granularity */ #ifdef CONFIG_PPC_8xx #define HW_BREAKPOINT_SIZE 0x4 diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 016bd831908e..160fbbf41d40 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address, (void __user *)address); } #else /* !CONFIG_PPC_ADV_DEBUG_REGS */ + +static void do_break_handler(struct pt_regs *regs) +{ + struct arch_hw_breakpoint null_brk = {0}; + struct arch_hw_breakpoint *info; + struct ppc_inst instr = ppc_inst(0); + int type = 0; + int size = 0; + unsigned long ea; + int i; + + /* + * If underneath hw supports only one watchpoint, we know it + * caused exception. 8xx also falls into this category. + */ + if (nr_wp_slots() == 1) { + __set_breakpoint(0, &null_brk); + current->thread.hw_brk[0] = null_brk; + current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED; + return; + } + + /* Otherwise findout which DAWR caused exception and disable it. */ + wp_get_instr_detail(regs, &instr, &type, &size, &ea); + + for (i = 0; i < nr_wp_slots(); i++) { + info = ¤t->thread.hw_brk[i]; + if (!info->address) + continue; + + if (wp_check_constraints(regs, instr, ea, type, size, info)) { + __set_breakpoint(i, &null_brk); + current->thread.hw_brk[i] = null_brk; + current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED; + } + } +} + void do_break (struct pt_regs *regs, unsigned long address, unsigned long error_code) { @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address, if (debugger_break_match(regs)) return; + /* + * We reach here only when watchpoint exception is generated by ptrace + * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set, + * watchpoint is already handled by hw_breakpoint_handler() so we don't + * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set, + * we need to manually handle the watchpoint here. + */ + if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT)) + do_break_handler(regs); + /* Deliver the signal to userspace */ force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address); } diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c index 57a0ab822334..c9122ed91340 100644 --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c @@ -286,11 +286,13 @@ long ppc_del_hwdebug(struct task_struct *child, long data) } return ret; #else /* CONFIG_HAVE_HW_BREAKPOINT */ - if (child->thread.hw_brk[data - 1].address == 0) + if (!(child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) && + child->thread.hw_brk[data - 1].address == 0) return -ENOENT; child->thread.hw_brk[data - 1].address = 0; child->thread.hw_brk[data - 1].type = 0; + child->thread.hw_brk[data - 1].flags = 0; #endif /* CONFIG_HAVE_HW_BREAKPOINT */ return 0;
On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel disables event every time it fires and user has to re-enable it. Also, in case of ptrace watchpoint, kernel notifies ptrace user before executing instruction. With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable ptrace event and thus it's causing infinite loop of exceptions. This is especially harmful when user watches on a data which is also read/written by kernel, eg syscall parameters. In such case, infinite exceptions happens in kernel mode which causes soft-lockup. Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers") Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> --- arch/powerpc/include/asm/hw_breakpoint.h | 3 ++ arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++ arch/powerpc/kernel/ptrace/ptrace-noadv.c | 4 +- 3 files changed, 54 insertions(+), 1 deletion(-)