Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq

[Li Bin]

on 2016/4/8 13:14, Pratyush Anand wrote:
> Hi Li,
> 
> On 07/04/2016:07:34:37 PM, Li Bin wrote:
>> Hi Pratyush,
>>
>> on 2016/4/4 13:17, Pratyush Anand wrote:
>>> Hi Li,
>>>
>>> On 31/03/2016:08:45:05 PM, Li Bin wrote:
>>>> Hi Pratyush,
>>>>
>>>> on 2016/3/21 18:24, Pratyush Anand wrote:
>>>>> On 21/03/2016:08:37:50 AM, He Kuang wrote:
>>>>>> On arm64, watchpoint handler enables single-step to bypass the next
>>>>>> instruction for not recursive enter. If an irq is triggered right
>>>>>> after the watchpoint, a single-step will be wrongly triggered in irq
>>>>>> handler, which causes the watchpoint address not stepped over and
>>>>>> system hang.
>>>>>
>>>>> Does patch [1] resolves this issue as well? I hope it should. Patch[1] has still
>>>>> not been sent for review. Your test result will be helpful.
>>>>>
>>>>> ~Pratyush
>>>>>
>>>>> [1] https://github.com/pratyushanand/linux/commit/7623c8099ac22eaa00e7e0f52430f7a4bd154652
>>>>
>>>> This patch did not consider that, when excetpion return, the singlestep flag
>>>> should be restored, otherwise the right singlestep will not triggered.
>>>> Right?
>>>
>>> Yes, you are right, and there are other problems as well. Will Deacon pointed
>>> out [1] that kernel debugging is per-cpu rather than per-task. So, I did thought
>>> of a per-cpu implementation by introducing a new element "flags" in struct
>>> pt_regs. But even with that I see issues. For example:
>>> - While executing single step instruction, we get a data abort
>>> - In the kernel_entry of data abort we disable single stepping based on "flags"
>>>   bit field
>>> - While handling data abort we receive anther interrupt, so we are again in
>>>   kernel_entry (for el1_irq). Single stepping will be disabled again (although
>>>   it does not matter).
>>>
>>> Now the issue is that, what condition should be verified in kernel_exit for
>>> enabling single step again? In the above scenario, kernel_exit for el1_irq
>>> should not enable single stepping, but how to prevent that elegantly?
>>
>> The condition for kernel_entry to disable the single step is that MDSCR_EL1.SS
>> has been set. And only when the corresponding kernel_entry has disabled the single
>> step, the kernel_exit should enable it, but the kernel_exit of single-step exception
>> should be handled specially, that when disable single step in single-step exception
>> handler, flag of pt_regs stored in stack should be cleard to prevent to be re-enabled
>> by kernel_exit.
> 
> Nice, :-)
> I had latter on almost similar patch [1], but it did fail when I merged two of
> the tests.
> -- I inserted kprobe to an instruction in function __copy_to_user() which could
> generate data abort.
> -- In parallel I also run test case which is defined here [2]
> -- As soon as I did `cat /proc/version`, kernel crashed.

Hi Pratyush,

Firstly, I have test this case, and it does not trigger failture as you describing.
But it indeed may trigger problem, and it is an another issue that if an exception
triggered before single-step exception, changes the regs->pc (data abort exception will
fixup_exception), the current implemetion of kprobes does not support, for example:
1. kprobes brk exception setup single-step, regs->pc points to the slot, MDSCR.SS=1,
SPSR_EL1.SS=1 (Inactive state)
2. brk exception eret (Active-not-pending state)
3. execute the slot instruction and trigger data abort exception, and this case the
return addr is also the slot instruction, so the SPSR_EL1.SS is set to 1 (Inactive state)
4. but in the data abort exception, fixup_exception will change the regs->pc to the fixup
code
5. data abort exception eret, going into Active-not-pending state, executing fixup code
without taking an exception, going into Active-pending state, triggering single-step
exception. But the single-step instruction is not the target instrution, so kprobe fails.

And so this case including copy_to/from_user should be added to kprobes blacklist.
Right, or am i missing something?

Thanks,
Li Bin

> 
> Although, just by comparing visually I do not see any functional difference with
> the patch you inlined below, still can you please try both of the above test
> case in parallel and see how does it behave? To insert kprobe within
> __copy_to_user() you need to revert "arm64: add copy_to/from_user to kprobes
> blacklist".
> 
> [1] https://github.com/pratyushanand/linux/commit/9182afbe640ea7caf52e209eb8e5f00bdf91b0c0
> [2] http://thread.gmane.org/gmane.linux.kernel/2167918
>>
>> Thanks,
>> Li Bin
>>