Query: arm64: hwbreakpoint: single stepping in case of custom overflow_handler

[Pratyush Anand]

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