| Message ID | d3b1a128-b97c-7e52-b30c-56ec3990381a@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 26, 2017 at 04:42:33PM +0530, Pratyush Anand wrote: > Hi Will, > > When we run test from samples/hw_breakpoint/data_breakpoint.c, it > triggers continuous watchpoint exception handler. > > Reproducer: > # insmod data_breakpoint.ko ksym=__sysrq_enabled > # cat /proc/sys/kernel/sysrq > > So, wanted to understand that why do we not allow single stepping in > case overflow_handler is a customized one and not from perf > infrastructure? > > Patch [1] allows to work with a custom overflow_handler, but I am > not sure if that could be an acceptable choice. Changing this would break userspace, such as GDB, as Will noted last time this came up: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/425363.html I don't beleive that this is something we can change. Thanks, Mark. > There are issues with samples/hw_breakpoint/data_breakpoint.c, even > when using patch [1],because overflow_handler of test calls > dump_stack(). I am not yet sure,what happened here..my guess is that > dump_stack() triggered a SW BRK exception somewhere. Anyway,thats a > secondary problem,I can look into if patch [1] makes sense. > > > ~Pratyush > > [1] > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c > index 749f81779420..ea8ab0656dd0 100644 > --- a/arch/arm64/kernel/hw_breakpoint.c > +++ b/arch/arm64/kernel/hw_breakpoint.c > @@ -661,7 +661,7 @@ static int breakpoint_handler(unsigned long > unused, unsigned int esr, > perf_bp_event(bp, regs); > > /* Do we need to handle the stepping? */ > - if (is_default_overflow_handler(bp)) > + if (bp->overflow_handler) > step = 1; > unlock: > rcu_read_unlock(); > @@ -789,7 +789,7 @@ static int watchpoint_handler(unsigned long > addr, unsigned int esr, > perf_bp_event(wp, regs); > > /* Do we need to handle the stepping? */ > - if (is_default_overflow_handler(wp)) > + if (wp->overflow_handler) > step = 1; > } > if (min_dist > 0 && min_dist != -1) { > @@ -800,7 +800,7 @@ static int watchpoint_handler(unsigned long > addr, unsigned int esr, > perf_bp_event(wp, regs); > > /* Do we need to handle the stepping? */ > - if (is_default_overflow_handler(wp)) > + if (wp->overflow_handler) > step = 1; > } > rcu_read_unlock(); > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Mark, Thanks for your quick response. On Friday 26 May 2017 04:56 PM, Mark Rutland wrote: > On Fri, May 26, 2017 at 04:42:33PM +0530, Pratyush Anand wrote: >> Hi Will, >> >> When we run test from samples/hw_breakpoint/data_breakpoint.c, it >> triggers continuous watchpoint exception handler. >> >> Reproducer: >> # insmod data_breakpoint.ko ksym=__sysrq_enabled >> # cat /proc/sys/kernel/sysrq >> >> So, wanted to understand that why do we not allow single stepping in >> case overflow_handler is a customized one and not from perf >> infrastructure? >> >> Patch [1] allows to work with a custom overflow_handler, but I am >> not sure if that could be an acceptable choice. > > Changing this would break userspace, such as GDB, as Will noted last > time this came up: If it is only userspace concern, then probably we can take care of that. We can additionally check for !user_mode(regs). > > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/425363.html > > I don't beleive that this is something we can change. > > Thanks, > Mark. > >> There are issues with samples/hw_breakpoint/data_breakpoint.c, even >> when using patch [1],because overflow_handler of test calls >> dump_stack(). I am not yet sure,what happened here..my guess is that >> dump_stack() triggered a SW BRK exception somewhere. Anyway,thats a >> secondary problem,I can look into if patch [1] makes sense. >> >> >> ~Pratyush >> >> [1] >> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c >> index 749f81779420..ea8ab0656dd0 100644 >> --- a/arch/arm64/kernel/hw_breakpoint.c >> +++ b/arch/arm64/kernel/hw_breakpoint.c >> @@ -661,7 +661,7 @@ static int breakpoint_handler(unsigned long >> unused, unsigned int esr, >> perf_bp_event(bp, regs); >> >> /* Do we need to handle the stepping? */ >> - if (is_default_overflow_handler(bp)) >> + if (bp->overflow_handler) Like + if (!user_mode(regs) && bp->overflow_handler) >> step = 1; >> unlock: >> rcu_read_unlock(); >> @@ -789,7 +789,7 @@ static int watchpoint_handler(unsigned long >> addr, unsigned int esr, >> perf_bp_event(wp, regs); >> >> /* Do we need to handle the stepping? */ >> - if (is_default_overflow_handler(wp)) >> + if (wp->overflow_handler) >> step = 1; >> } >> if (min_dist > 0 && min_dist != -1) { >> @@ -800,7 +800,7 @@ static int watchpoint_handler(unsigned long >> addr, unsigned int esr, >> perf_bp_event(wp, regs); >> >> /* Do we need to handle the stepping? */ >> - if (is_default_overflow_handler(wp)) >> + if (wp->overflow_handler) + if (!user_mode(regs) && wp->overflow_handler) >> step = 1; >> } >> rcu_read_unlock(); >> ~Pratyush
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index 749f81779420..ea8ab0656dd0 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -661,7 +661,7 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr, perf_bp_event(bp, regs); /* Do we need to handle the stepping? */ - if (is_default_overflow_handler(bp)) + if (bp->overflow_handler) step = 1; unlock:
Hi Will, When we run test from samples/hw_breakpoint/data_breakpoint.c, it triggers continuous watchpoint exception handler. Reproducer: # insmod data_breakpoint.ko ksym=__sysrq_enabled # cat /proc/sys/kernel/sysrq So, wanted to understand that why do we not allow single stepping in case overflow_handler is a customized one and not from perf infrastructure? Patch [1] allows to work with a custom overflow_handler, but I am not sure if that could be an acceptable choice. There are issues with samples/hw_breakpoint/data_breakpoint.c, even when using patch [1],because overflow_handler of test calls dump_stack(). I am not yet sure,what happened here..my guess is that dump_stack() triggered a SW BRK exception somewhere. Anyway,thats a secondary problem,I can look into if patch [1] makes sense. ~Pratyush [1] rcu_read_unlock(); @@ -789,7 +789,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr, perf_bp_event(wp, regs); /* Do we need to handle the stepping? */ - if (is_default_overflow_handler(wp)) + if (wp->overflow_handler) step = 1; } if (min_dist > 0 && min_dist != -1) { @@ -800,7 +800,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr, perf_bp_event(wp, regs); /* Do we need to handle the stepping? */ - if (is_default_overflow_handler(wp)) + if (wp->overflow_handler) step = 1; } rcu_read_unlock();