| Message ID | 20200825043617.1073634-6-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 (d4ecce4dcc8f8820286cf4e0859850c555e89854) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 91 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
Le 25/08/2020 à 06:36, Ravi Bangoria a écrit : > 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 | 5 +++ > 3 files changed, 56 insertions(+) > > diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h > index 2eca3dd54b55..c72263214d3f 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..866597b407bc 100644 > --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c > +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c > @@ -286,11 +286,16 @@ long ppc_del_hwdebug(struct task_struct *child, long data) > } > return ret; > #else /* CONFIG_HAVE_HW_BREAKPOINT */ > + if (child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) I think child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED should go around additionnal () > + goto del; > + > if (child->thread.hw_brk[data - 1].address == 0) > return -ENOENT; What about replacing the above if by: if (!(child->thread.hw_brk[data - 1].flags) & HW_BRK_FLAG_DISABLED) && child->thread.hw_brk[data - 1].address == 0) return -ENOENT; That would avoid the goto and the label. > > +del: > 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; > Christophe
Hi Christophe, >> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c >> index 57a0ab822334..866597b407bc 100644 >> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c >> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c >> @@ -286,11 +286,16 @@ long ppc_del_hwdebug(struct task_struct *child, long data) >> } >> return ret; >> #else /* CONFIG_HAVE_HW_BREAKPOINT */ >> + if (child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) > > I think child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED should go around additionnal () Not sure I follow. > >> + goto del; >> + >> if (child->thread.hw_brk[data - 1].address == 0) >> return -ENOENT; > > What about replacing the above if by: > if (!(child->thread.hw_brk[data - 1].flags) & HW_BRK_FLAG_DISABLED) && > child->thread.hw_brk[data - 1].address == 0) > return -ENOENT; okay.. that's more compact. But more importantly, what I wanted to know is whether CONFIG_HAVE_HW_BREAKPOINT is set or not in production/distro builds for 8xx. Because I see it's not set in 8xx defconfigs. Thanks, Ravi
Le 25/08/2020 à 13:07, Ravi Bangoria a écrit : > Hi Christophe, > >>> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c >>> b/arch/powerpc/kernel/ptrace/ptrace-noadv.c >>> index 57a0ab822334..866597b407bc 100644 >>> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c >>> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c >>> @@ -286,11 +286,16 @@ long ppc_del_hwdebug(struct task_struct *child, >>> long data) >>> } >>> return ret; >>> #else /* CONFIG_HAVE_HW_BREAKPOINT */ >>> + if (child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) >> >> I think child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED >> should go around additionnal () > > Not sure I follow. Neither do I .... I thought that GCC would emit a warning for that, but in fact it only emit warnings for things like: if (flags & HW_BRK_FLAG_DISABLED == HW_BRK_FLAG_DISABLED) > >> >>> + goto del; >>> + >>> if (child->thread.hw_brk[data - 1].address == 0) >>> return -ENOENT; >> >> What about replacing the above if by: >> if (!(child->thread.hw_brk[data - 1].flags) & >> HW_BRK_FLAG_DISABLED) && >> child->thread.hw_brk[data - 1].address == 0) >> return -ENOENT; > okay.. that's more compact. > > But more importantly, what I wanted to know is whether > CONFIG_HAVE_HW_BREAKPOINT > is set or not in production/distro builds for 8xx. Because I see it's > not set in > 8xx defconfigs. Yes in our production configs with have CONFIG_PERF_EVENTS, that implies CONFIG_HAVE_HW_BREAKPOINT Christophe
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h index 2eca3dd54b55..c72263214d3f 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..866597b407bc 100644 --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c @@ -286,11 +286,16 @@ long ppc_del_hwdebug(struct task_struct *child, long data) } return ret; #else /* CONFIG_HAVE_HW_BREAKPOINT */ + if (child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) + goto del; + if (child->thread.hw_brk[data - 1].address == 0) return -ENOENT; +del: 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 | 5 +++ 3 files changed, 56 insertions(+)