| Message ID | 570645CD.6030400@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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 > > --- > arch/arm64/include/asm/assembler.h | 11 +++++++++++ > arch/arm64/include/asm/debug-monitors.h | 2 +- > arch/arm64/include/asm/ptrace.h | 2 ++ > arch/arm64/kernel/asm-offsets.c | 1 + > arch/arm64/kernel/debug-monitors.c | 3 ++- > arch/arm64/kernel/entry.S | 18 ++++++++++++++++++ > arch/arm64/kernel/hw_breakpoint.c | 2 +- > arch/arm64/kernel/kgdb.c | 2 +- > 8 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index 70f7b9e..56a4335 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -60,6 +60,17 @@ > msr daifclr, #8 > .endm > > + .macro disable_step, orig > + bic \orig, \orig, #1 > + msr mdscr_el1, \orig > + .endm > + > + .macro enable_step, orig > + disable_dbg > + orr \orig, \orig, #1 > + msr mdscr_el1, \orig > + .endm > + > .macro disable_step_tsk, flgs, tmp > tbz \flgs, #TIF_SINGLESTEP, 9990f > mrs \tmp, mdscr_el1 > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h > index 2fcb9b7..50f114c 100644 > --- a/arch/arm64/include/asm/debug-monitors.h > +++ b/arch/arm64/include/asm/debug-monitors.h > @@ -115,7 +115,7 @@ void user_rewind_single_step(struct task_struct *task); > void user_fastforward_single_step(struct task_struct *task); > > void kernel_enable_single_step(struct pt_regs *regs); > -void kernel_disable_single_step(void); > +void kernel_disable_single_step(struct pt_regs *regs); > int kernel_active_single_step(void); > > #ifdef CONFIG_HAVE_HW_BREAKPOINT > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h > index a307eb6..da00cd8 100644 > --- a/arch/arm64/include/asm/ptrace.h > +++ b/arch/arm64/include/asm/ptrace.h > @@ -113,6 +113,8 @@ struct pt_regs { > u64 sp; > u64 pc; > u64 pstate; > + u64 flags; > + u64 padding; > }; > }; > u64 orig_x0; > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c > index 3ae6b31..7936ba9 100644 > --- a/arch/arm64/kernel/asm-offsets.c > +++ b/arch/arm64/kernel/asm-offsets.c > @@ -56,6 +56,7 @@ int main(void) > DEFINE(S_COMPAT_SP, offsetof(struct pt_regs, compat_sp)); > #endif > DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate)); > + DEFINE(S_FLAGS, offsetof(struct pt_regs, flags)); > DEFINE(S_PC, offsetof(struct pt_regs, pc)); > DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0)); > DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno)); > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index c45f296..9d2bdbf 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -403,9 +403,10 @@ void kernel_enable_single_step(struct pt_regs *regs) > enable_debug_monitors(DBG_ACTIVE_EL1); > } > > -void kernel_disable_single_step(void) > +void kernel_disable_single_step(struct pt_regs *regs) > { > WARN_ON(!irqs_disabled()); > + regs->flags = 0; > mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); > disable_debug_monitors(DBG_ACTIVE_EL1); > } > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 12e8d2b..753bef2 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -87,6 +87,15 @@ > stp x26, x27, [sp, #16 * 13] > stp x28, x29, [sp, #16 * 14] > > + .if \el == 1 > + mrs x19, mdscr_el1 > + and x20, x19, #1 > + str x20, [sp, #S_FLAGS] > + tbz x20, #0, 1f > + disable_step x19 > +1: > + .endif > + > .if \el == 0 > mrs x21, sp_el0 > mov tsk, sp > @@ -154,6 +163,15 @@ alternative_endif > .endif > msr elr_el1, x21 // set up the return data > msr spsr_el1, x22 > + > + .if \el == 1 > + ldr x19, [sp, #S_FLAGS] > + tbz x19, #0, 1f > + mrs x19, mdscr_el1 > + enable_step x19 > +1: > + .endif > + > ldp x0, x1, [sp, #16 * 0] > ldp x2, x3, [sp, #16 * 1] > ldp x4, x5, [sp, #16 * 2] > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c > index b45c95d..6afbb80 100644 > --- a/arch/arm64/kernel/hw_breakpoint.c > +++ b/arch/arm64/kernel/hw_breakpoint.c > @@ -802,7 +802,7 @@ int reinstall_suspended_bps(struct pt_regs *regs) > toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1); > > if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) { > - kernel_disable_single_step(); > + kernel_disable_single_step(regs); > handled_exception = 1; > } else { > handled_exception = 0; > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c > index b67531a..f92c5fb 100644 > --- a/arch/arm64/kernel/kgdb.c > +++ b/arch/arm64/kernel/kgdb.c > @@ -183,7 +183,7 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, > * Received continue command, disable single step > */ > if (kernel_active_single_step()) > - kernel_disable_single_step(); > + kernel_disable_single_step(linux_regs); > > err = 0; > break; > -- > 1.7.1 > > > > > [1] http://www.spinics.net/lists/arm-kernel/msg491844.html > > > > . > >
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 >>
On 08/04/2016:04:07:12 PM, Li Bin wrote: > > > 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: Yes, you are right, I missed it. All those aborts which has a fixup defined, will fail. While, I did not see any issue when running test individually, ie only hitting kprobe at __copy_to_user() instructions, because there is no fixup for them. I was able to trace instruction which was aborting. Problem occurred only when I run perf memory read to linux_proc_banner in parallel. Since I do not see failure due to fixup_exception in this test case, so I think we are missing some more pitfalls. But certainly it is going to fail in the case __get_user/__put_user etc are being traced, because there exists a fixup section for them. > 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 Yes, for the instructions with fixup defined. > 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? As of now, we are be going with blacklisting approach only. ~Pratyush
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index 70f7b9e..56a4335 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -60,6 +60,17 @@ msr daifclr, #8 .endm + .macro disable_step, orig + bic \orig, \orig, #1 + msr mdscr_el1, \orig + .endm + + .macro enable_step, orig + disable_dbg + orr \orig, \orig, #1 + msr mdscr_el1, \orig + .endm + .macro disable_step_tsk, flgs, tmp tbz \flgs, #TIF_SINGLESTEP, 9990f mrs \tmp, mdscr_el1 diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h index 2fcb9b7..50f114c 100644 --- a/arch/arm64/include/asm/debug-monitors.h +++ b/arch/arm64/include/asm/debug-monitors.h @@ -115,7 +115,7 @@ void user_rewind_single_step(struct task_struct *task); void user_fastforward_single_step(struct task_struct *task); void kernel_enable_single_step(struct pt_regs *regs); -void kernel_disable_single_step(void); +void kernel_disable_single_step(struct pt_regs *regs); int kernel_active_single_step(void); #ifdef CONFIG_HAVE_HW_BREAKPOINT diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index a307eb6..da00cd8 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -113,6 +113,8 @@ struct pt_regs { u64 sp; u64 pc; u64 pstate; + u64 flags; + u64 padding; }; }; u64 orig_x0; diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index 3ae6b31..7936ba9 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -56,6 +56,7 @@ int main(void) DEFINE(S_COMPAT_SP, offsetof(struct pt_regs, compat_sp)); #endif DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate)); + DEFINE(S_FLAGS, offsetof(struct pt_regs, flags)); DEFINE(S_PC, offsetof(struct pt_regs, pc)); DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0)); DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno)); diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index c45f296..9d2bdbf 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -403,9 +403,10 @@ void kernel_enable_single_step(struct pt_regs *regs) enable_debug_monitors(DBG_ACTIVE_EL1); } -void kernel_disable_single_step(void) +void kernel_disable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); + regs->flags = 0; mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); disable_debug_monitors(DBG_ACTIVE_EL1); } diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 12e8d2b..753bef2 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -87,6 +87,15 @@ stp x26, x27, [sp, #16 * 13] stp x28, x29, [sp, #16 * 14] + .if \el == 1 + mrs x19, mdscr_el1 + and x20, x19, #1 + str x20, [sp, #S_FLAGS] + tbz x20, #0, 1f + disable_step x19 +1: + .endif + .if \el == 0 mrs x21, sp_el0 mov tsk, sp @@ -154,6 +163,15 @@ alternative_endif .endif msr elr_el1, x21 // set up the return data msr spsr_el1, x22 + + .if \el == 1 + ldr x19, [sp, #S_FLAGS] + tbz x19, #0, 1f + mrs x19, mdscr_el1 + enable_step x19 +1: + .endif + ldp x0, x1, [sp, #16 * 0] ldp x2, x3, [sp, #16 * 1] ldp x4, x5, [sp, #16 * 2] diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index b45c95d..6afbb80 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -802,7 +802,7 @@ int reinstall_suspended_bps(struct pt_regs *regs) toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1); if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) { - kernel_disable_single_step(); + kernel_disable_single_step(regs); handled_exception = 1; } else { handled_exception = 0; diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index b67531a..f92c5fb 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -183,7 +183,7 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, * Received continue command, disable single step */ if (kernel_active_single_step()) - kernel_disable_single_step(); + kernel_disable_single_step(linux_regs); err = 0; break;