* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-16 0:18 ` Yang Shi 0 siblings, 0 replies; 31+ messages in thread From: Yang Shi @ 2015-12-16 0:18 UTC (permalink / raw) To: Will.Deacon, Catalin.Marinas Cc: linux-kernel, linux-arm-kernel, linaro-kernel, yang.shi The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in debug exception, so it sounds safe to have interrupt enabled if it is not disabled by the parent process. Signed-off-by: Yang Shi <yang.shi@linaro.org> --- arch/arm64/kernel/debug-monitors.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 8aee3ae..90d70e4 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -239,6 +239,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr, return 0; if (user_mode(regs)) { + if (interrupts_enabled(regs)) + local_irq_enable(); + info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_HWBKPT; @@ -310,6 +313,9 @@ static int brk_handler(unsigned long addr, unsigned int esr, siginfo_t info; if (user_mode(regs)) { + if (interrupts_enabled(regs)) + local_irq_enable(); + info = (siginfo_t) { .si_signo = SIGTRAP, .si_errno = 0, @@ -337,6 +343,10 @@ int aarch32_break_handler(struct pt_regs *regs) if (!compat_user_mode(regs)) return -EFAULT; + /* COMPAT_PSR_I_BIT has the same bit mask with non-compat one */ + if (interrupts_enabled(regs)) + local_irq_enable(); + if (compat_thumb_mode(regs)) { /* get 16-bit Thumb instruction */ get_user(thumb_instr, (u16 __user *)pc); -- 2.0.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-16 0:18 ` Yang Shi 0 siblings, 0 replies; 31+ messages in thread From: Yang Shi @ 2015-12-16 0:18 UTC (permalink / raw) To: linux-arm-kernel The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in debug exception, so it sounds safe to have interrupt enabled if it is not disabled by the parent process. Signed-off-by: Yang Shi <yang.shi@linaro.org> --- arch/arm64/kernel/debug-monitors.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 8aee3ae..90d70e4 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -239,6 +239,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr, return 0; if (user_mode(regs)) { + if (interrupts_enabled(regs)) + local_irq_enable(); + info.si_signo = SIGTRAP; info.si_errno = 0; info.si_code = TRAP_HWBKPT; @@ -310,6 +313,9 @@ static int brk_handler(unsigned long addr, unsigned int esr, siginfo_t info; if (user_mode(regs)) { + if (interrupts_enabled(regs)) + local_irq_enable(); + info = (siginfo_t) { .si_signo = SIGTRAP, .si_errno = 0, @@ -337,6 +343,10 @@ int aarch32_break_handler(struct pt_regs *regs) if (!compat_user_mode(regs)) return -EFAULT; + /* COMPAT_PSR_I_BIT has the same bit mask with non-compat one */ + if (interrupts_enabled(regs)) + local_irq_enable(); + if (compat_thumb_mode(regs)) { /* get 16-bit Thumb instruction */ get_user(thumb_instr, (u16 __user *)pc); -- 2.0.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-16 0:18 ` Yang Shi @ 2015-12-16 11:13 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2015-12-16 11:13 UTC (permalink / raw) To: Yang Shi; +Cc: Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel On Tue, Dec 15, 2015 at 04:18:08PM -0800, Yang Shi wrote: > The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in > debug exception, so it sounds safe to have interrupt enabled if it is not > disabled by the parent process. Is this actually fixing an issue you're seeing, or did you just spot this? Given that force_sig_info disable interrupts, I don't think this is really worth doing. > Signed-off-by: Yang Shi <yang.shi@linaro.org> > --- > arch/arm64/kernel/debug-monitors.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 8aee3ae..90d70e4 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -239,6 +239,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr, > return 0; > > if (user_mode(regs)) { > + if (interrupts_enabled(regs)) > + local_irq_enable(); > + My worry here is that we take an interrupt and, on the return path, decide to reschedule due to CONFIG_PREEMPT. If we somehow end up back in the debugger, I'm concerned that it could remove the breakpoint and then later see an unexpected SIGTRAP from the child. Having said that, I've failed to construct a non-racy scenario in which that can happen, but I'm just really uncomfortable making this change unless there's a real problem being solved. Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-16 11:13 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2015-12-16 11:13 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 15, 2015 at 04:18:08PM -0800, Yang Shi wrote: > The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in > debug exception, so it sounds safe to have interrupt enabled if it is not > disabled by the parent process. Is this actually fixing an issue you're seeing, or did you just spot this? Given that force_sig_info disable interrupts, I don't think this is really worth doing. > Signed-off-by: Yang Shi <yang.shi@linaro.org> > --- > arch/arm64/kernel/debug-monitors.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 8aee3ae..90d70e4 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -239,6 +239,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr, > return 0; > > if (user_mode(regs)) { > + if (interrupts_enabled(regs)) > + local_irq_enable(); > + My worry here is that we take an interrupt and, on the return path, decide to reschedule due to CONFIG_PREEMPT. If we somehow end up back in the debugger, I'm concerned that it could remove the breakpoint and then later see an unexpected SIGTRAP from the child. Having said that, I've failed to construct a non-racy scenario in which that can happen, but I'm just really uncomfortable making this change unless there's a real problem being solved. Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-16 11:13 ` Will Deacon @ 2015-12-16 20:45 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2015-12-16 20:45 UTC (permalink / raw) To: Will Deacon Cc: Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 12/16/2015 3:13 AM, Will Deacon wrote: > On Tue, Dec 15, 2015 at 04:18:08PM -0800, Yang Shi wrote: >> The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in >> debug exception, so it sounds safe to have interrupt enabled if it is not >> disabled by the parent process. > > Is this actually fixing an issue you're seeing, or did you just spot this? > Given that force_sig_info disable interrupts, I don't think this is really > worth doing. I should made more comments at the first place, sorry for the inconvenience. I did run into some problems on -rt kernel with CRIU restore, please see the below kernel bug log: BUG: sleeping function called from invalid context at /kernel-source/kernel/locking/rtmutex.c:917 in_atomic(): 0, irqs_disabled(): 128, pid: 551, name: test.sh CPU: 5 PID: 551 Comm: test.sh Not tainted 4.1.13-rt13 #7 Hardware name: Freescale Layerscape 2085a RDB Board (DT) Call trace: [<ffff8000000885f0>] dump_backtrace+0x0/0x128 [<ffff80000008873c>] show_stack+0x24/0x30 [<ffff800000798e84>] dump_stack+0x80/0xa0 [<ffff8000000bd858>] ___might_sleep+0x128/0x1a0 [<ffff8000007a072c>] rt_spin_lock+0x2c/0x40 [<ffff8000000a477c>] force_sig_info+0xcc/0x210 [<ffff800000085174>] brk_handler.part.2+0x6c/0x80 [<ffff800000085260>] brk_handler+0xd8/0xe8 [<ffff800000082368>] do_debug_exception+0x58/0xb8 Exception stack(0xffff80834b6e3e30 to 0xffff80834b6e3f50) 3e20: 00000000 00000000 004e6000 00000000 3e40: ffffffff ffffffff 00400004 00000000 0000d280 00000000 00000007 00000000 3e60: 00000021 00000000 ffffffff ffffffff 0000011a 00000000 000000de 00000000 3e80: 4ab9ef50 ffff8083 00086324 ffff8000 e587f780 0000ffff 000839b0 ffff8000 3ea0: 00000000 00000000 004e6000 00000000 ffffffff ffffffff 00400004 00000000 3ec0: 60000000 00000000 00000015 00000000 aa3e6000 0000ffff 0000d280 00000000 3ee0: 00000007 00000000 00000021 00000000 ffffffff ffffffff 00000000 00000000 3f00: 00000000 00000000 00000000 00000000 000000de 00000000 00000008 00000000 3f20: 004eff00 00000000 004e4ff0 00000000 004f0490 00000000 e587f730 0000ffff 3f40: 0046f508 00000000 00000028 00000000 It is because force_sig_info called spin_lock_irqsave which could sleep on -rt kernel with irq disabled. However, it just happens at brk_handler in my test. But, I saw single_step has the same code path, so I expanded it to single step too. Since this is rt related, cc to rt mailing list too. > >> Signed-off-by: Yang Shi <yang.shi@linaro.org> >> --- >> arch/arm64/kernel/debug-monitors.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c >> index 8aee3ae..90d70e4 100644 >> --- a/arch/arm64/kernel/debug-monitors.c >> +++ b/arch/arm64/kernel/debug-monitors.c >> @@ -239,6 +239,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr, >> return 0; >> >> if (user_mode(regs)) { >> + if (interrupts_enabled(regs)) >> + local_irq_enable(); >> + > > My worry here is that we take an interrupt and, on the return path, > decide to reschedule due to CONFIG_PREEMPT. If we somehow end up back > in the debugger, I'm concerned that it could remove the breakpoint and > then later see an unexpected SIGTRAP from the child. > > Having said that, I've failed to construct a non-racy scenario in which > that can happen, but I'm just really uncomfortable making this change > unless there's a real problem being solved. The patch is inspired by the similar code in other architectures, e.g. x86 and powerpc which have hardware debug exception to handle breakpoint and single step like arm64. And, they have interrupt enabled in both breakpoint and single step. So, I'm supposed arm64 could do the same thing. For the preempt case, it might be possible, but it sounds like a exception handling problem to me. The preempt should not be allowed in debug exception (current arm64 kernel does it), and in interrupt return path the code should check if debug is on or not. If debug is on, preempt should be just skipped. Or we could disable preempt in debug exception. I also checked the handling in x86 and powerpc, they go different way. 1. x86 Disable preempt in IST exception since it uses per CPU stack. 2. powerpc Check if debug is on in interrupt return path. Powerpc has DBCR0_IDM indicate if the processor is in debug mode. For ARM64, I don't find such bit. So, I may consider to have preempt disabled. Thanks, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-16 20:45 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2015-12-16 20:45 UTC (permalink / raw) To: linux-arm-kernel On 12/16/2015 3:13 AM, Will Deacon wrote: > On Tue, Dec 15, 2015 at 04:18:08PM -0800, Yang Shi wrote: >> The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in >> debug exception, so it sounds safe to have interrupt enabled if it is not >> disabled by the parent process. > > Is this actually fixing an issue you're seeing, or did you just spot this? > Given that force_sig_info disable interrupts, I don't think this is really > worth doing. I should made more comments at the first place, sorry for the inconvenience. I did run into some problems on -rt kernel with CRIU restore, please see the below kernel bug log: BUG: sleeping function called from invalid context at /kernel-source/kernel/locking/rtmutex.c:917 in_atomic(): 0, irqs_disabled(): 128, pid: 551, name: test.sh CPU: 5 PID: 551 Comm: test.sh Not tainted 4.1.13-rt13 #7 Hardware name: Freescale Layerscape 2085a RDB Board (DT) Call trace: [<ffff8000000885f0>] dump_backtrace+0x0/0x128 [<ffff80000008873c>] show_stack+0x24/0x30 [<ffff800000798e84>] dump_stack+0x80/0xa0 [<ffff8000000bd858>] ___might_sleep+0x128/0x1a0 [<ffff8000007a072c>] rt_spin_lock+0x2c/0x40 [<ffff8000000a477c>] force_sig_info+0xcc/0x210 [<ffff800000085174>] brk_handler.part.2+0x6c/0x80 [<ffff800000085260>] brk_handler+0xd8/0xe8 [<ffff800000082368>] do_debug_exception+0x58/0xb8 Exception stack(0xffff80834b6e3e30 to 0xffff80834b6e3f50) 3e20: 00000000 00000000 004e6000 00000000 3e40: ffffffff ffffffff 00400004 00000000 0000d280 00000000 00000007 00000000 3e60: 00000021 00000000 ffffffff ffffffff 0000011a 00000000 000000de 00000000 3e80: 4ab9ef50 ffff8083 00086324 ffff8000 e587f780 0000ffff 000839b0 ffff8000 3ea0: 00000000 00000000 004e6000 00000000 ffffffff ffffffff 00400004 00000000 3ec0: 60000000 00000000 00000015 00000000 aa3e6000 0000ffff 0000d280 00000000 3ee0: 00000007 00000000 00000021 00000000 ffffffff ffffffff 00000000 00000000 3f00: 00000000 00000000 00000000 00000000 000000de 00000000 00000008 00000000 3f20: 004eff00 00000000 004e4ff0 00000000 004f0490 00000000 e587f730 0000ffff 3f40: 0046f508 00000000 00000028 00000000 It is because force_sig_info called spin_lock_irqsave which could sleep on -rt kernel with irq disabled. However, it just happens at brk_handler in my test. But, I saw single_step has the same code path, so I expanded it to single step too. Since this is rt related, cc to rt mailing list too. > >> Signed-off-by: Yang Shi <yang.shi@linaro.org> >> --- >> arch/arm64/kernel/debug-monitors.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c >> index 8aee3ae..90d70e4 100644 >> --- a/arch/arm64/kernel/debug-monitors.c >> +++ b/arch/arm64/kernel/debug-monitors.c >> @@ -239,6 +239,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr, >> return 0; >> >> if (user_mode(regs)) { >> + if (interrupts_enabled(regs)) >> + local_irq_enable(); >> + > > My worry here is that we take an interrupt and, on the return path, > decide to reschedule due to CONFIG_PREEMPT. If we somehow end up back > in the debugger, I'm concerned that it could remove the breakpoint and > then later see an unexpected SIGTRAP from the child. > > Having said that, I've failed to construct a non-racy scenario in which > that can happen, but I'm just really uncomfortable making this change > unless there's a real problem being solved. The patch is inspired by the similar code in other architectures, e.g. x86 and powerpc which have hardware debug exception to handle breakpoint and single step like arm64. And, they have interrupt enabled in both breakpoint and single step. So, I'm supposed arm64 could do the same thing. For the preempt case, it might be possible, but it sounds like a exception handling problem to me. The preempt should not be allowed in debug exception (current arm64 kernel does it), and in interrupt return path the code should check if debug is on or not. If debug is on, preempt should be just skipped. Or we could disable preempt in debug exception. I also checked the handling in x86 and powerpc, they go different way. 1. x86 Disable preempt in IST exception since it uses per CPU stack. 2. powerpc Check if debug is on in interrupt return path. Powerpc has DBCR0_IDM indicate if the processor is in debug mode. For ARM64, I don't find such bit. So, I may consider to have preempt disabled. Thanks, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-16 20:45 ` Shi, Yang @ 2015-12-21 10:48 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2015-12-21 10:48 UTC (permalink / raw) To: Shi, Yang Cc: Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On Wed, Dec 16, 2015 at 12:45:15PM -0800, Shi, Yang wrote: > On 12/16/2015 3:13 AM, Will Deacon wrote: > >On Tue, Dec 15, 2015 at 04:18:08PM -0800, Yang Shi wrote: > >>The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in > >>debug exception, so it sounds safe to have interrupt enabled if it is not > >>disabled by the parent process. > > > >Is this actually fixing an issue you're seeing, or did you just spot this? > >Given that force_sig_info disable interrupts, I don't think this is really > >worth doing. > > I should made more comments at the first place, sorry for the inconvenience. > > I did run into some problems on -rt kernel with CRIU restore, please see the > below kernel bug log: Thanks. > >My worry here is that we take an interrupt and, on the return path, > >decide to reschedule due to CONFIG_PREEMPT. If we somehow end up back > >in the debugger, I'm concerned that it could remove the breakpoint and > >then later see an unexpected SIGTRAP from the child. > > > >Having said that, I've failed to construct a non-racy scenario in which > >that can happen, but I'm just really uncomfortable making this change > >unless there's a real problem being solved. > > The patch is inspired by the similar code in other architectures, e.g. x86 > and powerpc which have hardware debug exception to handle breakpoint and > single step like arm64. And, they have interrupt enabled in both breakpoint > and single step. So, I'm supposed arm64 could do the same thing. > > For the preempt case, it might be possible, but it sounds like a exception > handling problem to me. The preempt should not be allowed in debug exception > (current arm64 kernel does it), and in interrupt return path the code should > check if debug is on or not. If debug is on, preempt should be just skipped. > Or we could disable preempt in debug exception. Yeah, disabling preemption during the debug handler and then enabling interrupts if it came from userspace sounds like the best option. However, that's a fairly invasive change to entry.S at this point, so maybe we're better off with something like the patch below in the meantime? Will --->8 diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 8aee3aeec3e6..7f4913f2ea3c 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -226,11 +226,29 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr) return retval; } +static void send_user_sigtrap(int si_code) +{ + struct pt_regs *regs = current_pt_regs(); + siginfo_t info = { + .si_signo = SIGTRAP, + .si_errno = 0, + .si_code = si_code, + .si_addr = (void __user *)instruction_pointer(regs), + }; + + if (WARN_ON(!user_mode(regs))) + return; + + preempt_disable(); + local_irq_enable(); + force_sig_info(SIGTRAP, &info, current); + local_irq_disable(); + preempt_enable(); +} + static int single_step_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - /* * If we are stepping a pending breakpoint, call the hw_breakpoint * handler first. @@ -239,11 +257,7 @@ static int single_step_handler(unsigned long addr, unsigned int esr, return 0; if (user_mode(regs)) { - info.si_signo = SIGTRAP; - info.si_errno = 0; - info.si_code = TRAP_HWBKPT; - info.si_addr = (void __user *)instruction_pointer(regs); - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_HWBKPT); /* * ptrace will disable single step unless explicitly @@ -307,17 +321,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr) static int brk_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - if (user_mode(regs)) { - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = (void __user *)instruction_pointer(regs), - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) { pr_warning("Unexpected kernel BRK exception at EL1\n"); return -EFAULT; @@ -328,7 +333,6 @@ static int brk_handler(unsigned long addr, unsigned int esr, int aarch32_break_handler(struct pt_regs *regs) { - siginfo_t info; u32 arm_instr; u16 thumb_instr; bool bp = false; @@ -359,14 +363,7 @@ int aarch32_break_handler(struct pt_regs *regs) if (!bp) return -EFAULT; - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = pc, - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); return 0; } ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-21 10:48 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2015-12-21 10:48 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 16, 2015 at 12:45:15PM -0800, Shi, Yang wrote: > On 12/16/2015 3:13 AM, Will Deacon wrote: > >On Tue, Dec 15, 2015 at 04:18:08PM -0800, Yang Shi wrote: > >>The kernel just send out a SIGTRAP signal when handling ptrace breakpoint in > >>debug exception, so it sounds safe to have interrupt enabled if it is not > >>disabled by the parent process. > > > >Is this actually fixing an issue you're seeing, or did you just spot this? > >Given that force_sig_info disable interrupts, I don't think this is really > >worth doing. > > I should made more comments at the first place, sorry for the inconvenience. > > I did run into some problems on -rt kernel with CRIU restore, please see the > below kernel bug log: Thanks. > >My worry here is that we take an interrupt and, on the return path, > >decide to reschedule due to CONFIG_PREEMPT. If we somehow end up back > >in the debugger, I'm concerned that it could remove the breakpoint and > >then later see an unexpected SIGTRAP from the child. > > > >Having said that, I've failed to construct a non-racy scenario in which > >that can happen, but I'm just really uncomfortable making this change > >unless there's a real problem being solved. > > The patch is inspired by the similar code in other architectures, e.g. x86 > and powerpc which have hardware debug exception to handle breakpoint and > single step like arm64. And, they have interrupt enabled in both breakpoint > and single step. So, I'm supposed arm64 could do the same thing. > > For the preempt case, it might be possible, but it sounds like a exception > handling problem to me. The preempt should not be allowed in debug exception > (current arm64 kernel does it), and in interrupt return path the code should > check if debug is on or not. If debug is on, preempt should be just skipped. > Or we could disable preempt in debug exception. Yeah, disabling preemption during the debug handler and then enabling interrupts if it came from userspace sounds like the best option. However, that's a fairly invasive change to entry.S at this point, so maybe we're better off with something like the patch below in the meantime? Will --->8 diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 8aee3aeec3e6..7f4913f2ea3c 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -226,11 +226,29 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr) return retval; } +static void send_user_sigtrap(int si_code) +{ + struct pt_regs *regs = current_pt_regs(); + siginfo_t info = { + .si_signo = SIGTRAP, + .si_errno = 0, + .si_code = si_code, + .si_addr = (void __user *)instruction_pointer(regs), + }; + + if (WARN_ON(!user_mode(regs))) + return; + + preempt_disable(); + local_irq_enable(); + force_sig_info(SIGTRAP, &info, current); + local_irq_disable(); + preempt_enable(); +} + static int single_step_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - /* * If we are stepping a pending breakpoint, call the hw_breakpoint * handler first. @@ -239,11 +257,7 @@ static int single_step_handler(unsigned long addr, unsigned int esr, return 0; if (user_mode(regs)) { - info.si_signo = SIGTRAP; - info.si_errno = 0; - info.si_code = TRAP_HWBKPT; - info.si_addr = (void __user *)instruction_pointer(regs); - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_HWBKPT); /* * ptrace will disable single step unless explicitly @@ -307,17 +321,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr) static int brk_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - if (user_mode(regs)) { - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = (void __user *)instruction_pointer(regs), - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) { pr_warning("Unexpected kernel BRK exception at EL1\n"); return -EFAULT; @@ -328,7 +333,6 @@ static int brk_handler(unsigned long addr, unsigned int esr, int aarch32_break_handler(struct pt_regs *regs) { - siginfo_t info; u32 arm_instr; u16 thumb_instr; bool bp = false; @@ -359,14 +363,7 @@ int aarch32_break_handler(struct pt_regs *regs) if (!bp) return -EFAULT; - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = pc, - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); return 0; } ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-21 10:48 ` Will Deacon @ 2015-12-21 16:51 ` Thomas Gleixner -1 siblings, 0 replies; 31+ messages in thread From: Thomas Gleixner @ 2015-12-21 16:51 UTC (permalink / raw) To: Will Deacon Cc: Shi, Yang, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On Mon, 21 Dec 2015, Will Deacon wrote: > +static void send_user_sigtrap(int si_code) > +{ > + struct pt_regs *regs = current_pt_regs(); > + siginfo_t info = { > + .si_signo = SIGTRAP, > + .si_errno = 0, > + .si_code = si_code, > + .si_addr = (void __user *)instruction_pointer(regs), > + }; > + > + if (WARN_ON(!user_mode(regs))) > + return; > + > + preempt_disable(); That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, which is a 'sleeping' spinlock on RT. Why would we need to disable preemption here at all? What's the problem of being preempted or even migrated? Thanks, tglx ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-21 16:51 ` Thomas Gleixner 0 siblings, 0 replies; 31+ messages in thread From: Thomas Gleixner @ 2015-12-21 16:51 UTC (permalink / raw) To: linux-arm-kernel On Mon, 21 Dec 2015, Will Deacon wrote: > +static void send_user_sigtrap(int si_code) > +{ > + struct pt_regs *regs = current_pt_regs(); > + siginfo_t info = { > + .si_signo = SIGTRAP, > + .si_errno = 0, > + .si_code = si_code, > + .si_addr = (void __user *)instruction_pointer(regs), > + }; > + > + if (WARN_ON(!user_mode(regs))) > + return; > + > + preempt_disable(); That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, which is a 'sleeping' spinlock on RT. Why would we need to disable preemption here at all? What's the problem of being preempted or even migrated? Thanks, tglx ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-21 16:51 ` Thomas Gleixner @ 2015-12-21 17:00 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2015-12-21 17:00 UTC (permalink / raw) To: Thomas Gleixner Cc: Shi, Yang, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: > On Mon, 21 Dec 2015, Will Deacon wrote: > > +static void send_user_sigtrap(int si_code) > > +{ > > + struct pt_regs *regs = current_pt_regs(); > > + siginfo_t info = { > > + .si_signo = SIGTRAP, > > + .si_errno = 0, > > + .si_code = si_code, > > + .si_addr = (void __user *)instruction_pointer(regs), > > + }; > > + > > + if (WARN_ON(!user_mode(regs))) > > + return; > > + > > + preempt_disable(); > > That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, > which is a 'sleeping' spinlock on RT. Ah, I missed that :/ > Why would we need to disable preemption here at all? What's the problem of > being preempted or even migrated? There *might* not be a problem, I'm just really nervous about changing the behaviour on the debug path and subtly changing how ptrace behaves. My worry was that you could somehow get back into the tracer, and it could remove a software breakpoint in the knowledge that it wouldn't see any future (spurious) SIGTRAPs for that location. Without a concrete example, however, I guess I'll bite the bullet and enable irqs across the call to force_sig_info, since there is clearly a real issue here on RT. Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-21 17:00 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2015-12-21 17:00 UTC (permalink / raw) To: linux-arm-kernel On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: > On Mon, 21 Dec 2015, Will Deacon wrote: > > +static void send_user_sigtrap(int si_code) > > +{ > > + struct pt_regs *regs = current_pt_regs(); > > + siginfo_t info = { > > + .si_signo = SIGTRAP, > > + .si_errno = 0, > > + .si_code = si_code, > > + .si_addr = (void __user *)instruction_pointer(regs), > > + }; > > + > > + if (WARN_ON(!user_mode(regs))) > > + return; > > + > > + preempt_disable(); > > That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, > which is a 'sleeping' spinlock on RT. Ah, I missed that :/ > Why would we need to disable preemption here at all? What's the problem of > being preempted or even migrated? There *might* not be a problem, I'm just really nervous about changing the behaviour on the debug path and subtly changing how ptrace behaves. My worry was that you could somehow get back into the tracer, and it could remove a software breakpoint in the knowledge that it wouldn't see any future (spurious) SIGTRAPs for that location. Without a concrete example, however, I guess I'll bite the bullet and enable irqs across the call to force_sig_info, since there is clearly a real issue here on RT. Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-21 17:00 ` Will Deacon @ 2015-12-21 17:27 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2015-12-21 17:27 UTC (permalink / raw) To: Will Deacon, Thomas Gleixner Cc: Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 12/21/2015 9:00 AM, Will Deacon wrote: > On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: >> On Mon, 21 Dec 2015, Will Deacon wrote: >>> +static void send_user_sigtrap(int si_code) >>> +{ >>> + struct pt_regs *regs = current_pt_regs(); >>> + siginfo_t info = { >>> + .si_signo = SIGTRAP, >>> + .si_errno = 0, >>> + .si_code = si_code, >>> + .si_addr = (void __user *)instruction_pointer(regs), >>> + }; >>> + >>> + if (WARN_ON(!user_mode(regs))) >>> + return; >>> + >>> + preempt_disable(); >> >> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, >> which is a 'sleeping' spinlock on RT. > > Ah, I missed that :/ > >> Why would we need to disable preemption here at all? What's the problem of >> being preempted or even migrated? > > There *might* not be a problem, I'm just really nervous about changing > the behaviour on the debug path and subtly changing how ptrace behaves. > > My worry was that you could somehow get back into the tracer, and it > could remove a software breakpoint in the knowledge that it wouldn't > see any future (spurious) SIGTRAPs for that location. > > Without a concrete example, however, I guess I'll bite the bullet and > enable irqs across the call to force_sig_info, since there is clearly a > real issue here on RT. Thanks for the review. I don't have any concrete usecase for that race condition too since all ptrace tests from ltp passed. However, even though we need preemption disabled at some point in debug exception code path in the future, I think we could just extend the "signal delay send" approach from x86-64 to arm64, which is currently used by x86-64 on -rt kernel only. Regards, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2015-12-21 17:27 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2015-12-21 17:27 UTC (permalink / raw) To: linux-arm-kernel On 12/21/2015 9:00 AM, Will Deacon wrote: > On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: >> On Mon, 21 Dec 2015, Will Deacon wrote: >>> +static void send_user_sigtrap(int si_code) >>> +{ >>> + struct pt_regs *regs = current_pt_regs(); >>> + siginfo_t info = { >>> + .si_signo = SIGTRAP, >>> + .si_errno = 0, >>> + .si_code = si_code, >>> + .si_addr = (void __user *)instruction_pointer(regs), >>> + }; >>> + >>> + if (WARN_ON(!user_mode(regs))) >>> + return; >>> + >>> + preempt_disable(); >> >> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, >> which is a 'sleeping' spinlock on RT. > > Ah, I missed that :/ > >> Why would we need to disable preemption here at all? What's the problem of >> being preempted or even migrated? > > There *might* not be a problem, I'm just really nervous about changing > the behaviour on the debug path and subtly changing how ptrace behaves. > > My worry was that you could somehow get back into the tracer, and it > could remove a software breakpoint in the knowledge that it wouldn't > see any future (spurious) SIGTRAPs for that location. > > Without a concrete example, however, I guess I'll bite the bullet and > enable irqs across the call to force_sig_info, since there is clearly a > real issue here on RT. Thanks for the review. I don't have any concrete usecase for that race condition too since all ptrace tests from ltp passed. However, even though we need preemption disabled at some point in debug exception code path in the future, I think we could just extend the "signal delay send" approach from x86-64 to arm64, which is currently used by x86-64 on -rt kernel only. Regards, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2015-12-21 17:00 ` Will Deacon @ 2016-01-12 19:59 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-01-12 19:59 UTC (permalink / raw) To: Will Deacon, Thomas Gleixner Cc: Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 12/21/2015 9:00 AM, Will Deacon wrote: > On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: >> On Mon, 21 Dec 2015, Will Deacon wrote: >>> +static void send_user_sigtrap(int si_code) >>> +{ >>> + struct pt_regs *regs = current_pt_regs(); >>> + siginfo_t info = { >>> + .si_signo = SIGTRAP, >>> + .si_errno = 0, >>> + .si_code = si_code, >>> + .si_addr = (void __user *)instruction_pointer(regs), >>> + }; >>> + >>> + if (WARN_ON(!user_mode(regs))) >>> + return; >>> + >>> + preempt_disable(); >> >> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, >> which is a 'sleeping' spinlock on RT. > > Ah, I missed that :/ > >> Why would we need to disable preemption here at all? What's the problem of >> being preempted or even migrated? > > There *might* not be a problem, I'm just really nervous about changing > the behaviour on the debug path and subtly changing how ptrace behaves. > > My worry was that you could somehow get back into the tracer, and it > could remove a software breakpoint in the knowledge that it wouldn't > see any future (spurious) SIGTRAPs for that location. > > Without a concrete example, however, I guess I'll bite the bullet and > enable irqs across the call to force_sig_info, since there is clearly a > real issue here on RT. Hi Will, This might be buried in email storm during the holiday. Just want to double check the status. I'm supposed there is no objection for getting it merged in upstream? Thanks, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-01-12 19:59 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-01-12 19:59 UTC (permalink / raw) To: linux-arm-kernel On 12/21/2015 9:00 AM, Will Deacon wrote: > On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: >> On Mon, 21 Dec 2015, Will Deacon wrote: >>> +static void send_user_sigtrap(int si_code) >>> +{ >>> + struct pt_regs *regs = current_pt_regs(); >>> + siginfo_t info = { >>> + .si_signo = SIGTRAP, >>> + .si_errno = 0, >>> + .si_code = si_code, >>> + .si_addr = (void __user *)instruction_pointer(regs), >>> + }; >>> + >>> + if (WARN_ON(!user_mode(regs))) >>> + return; >>> + >>> + preempt_disable(); >> >> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, >> which is a 'sleeping' spinlock on RT. > > Ah, I missed that :/ > >> Why would we need to disable preemption here at all? What's the problem of >> being preempted or even migrated? > > There *might* not be a problem, I'm just really nervous about changing > the behaviour on the debug path and subtly changing how ptrace behaves. > > My worry was that you could somehow get back into the tracer, and it > could remove a software breakpoint in the knowledge that it wouldn't > see any future (spurious) SIGTRAPs for that location. > > Without a concrete example, however, I guess I'll bite the bullet and > enable irqs across the call to force_sig_info, since there is clearly a > real issue here on RT. Hi Will, This might be buried in email storm during the holiday. Just want to double check the status. I'm supposed there is no objection for getting it merged in upstream? Thanks, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-01-12 19:59 ` Shi, Yang (?) @ 2016-01-13 10:26 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-01-13 10:26 UTC (permalink / raw) To: Shi, Yang Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > On 12/21/2015 9:00 AM, Will Deacon wrote: > >On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: > >>On Mon, 21 Dec 2015, Will Deacon wrote: > >>>+static void send_user_sigtrap(int si_code) > >>>+{ > >>>+ struct pt_regs *regs = current_pt_regs(); > >>>+ siginfo_t info = { > >>>+ .si_signo = SIGTRAP, > >>>+ .si_errno = 0, > >>>+ .si_code = si_code, > >>>+ .si_addr = (void __user *)instruction_pointer(regs), > >>>+ }; > >>>+ > >>>+ if (WARN_ON(!user_mode(regs))) > >>>+ return; > >>>+ > >>>+ preempt_disable(); > >> > >>That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, > >>which is a 'sleeping' spinlock on RT. > > > >Ah, I missed that :/ > > > >>Why would we need to disable preemption here at all? What's the problem of > >>being preempted or even migrated? > > > >There *might* not be a problem, I'm just really nervous about changing > >the behaviour on the debug path and subtly changing how ptrace behaves. > > > >My worry was that you could somehow get back into the tracer, and it > >could remove a software breakpoint in the knowledge that it wouldn't > >see any future (spurious) SIGTRAPs for that location. > > > >Without a concrete example, however, I guess I'll bite the bullet and > >enable irqs across the call to force_sig_info, since there is clearly a > >real issue here on RT. > > This might be buried in email storm during the holiday. Just want to double > check the status. I'm supposed there is no objection for getting it merged > in upstream? Sorry, when you replied with: > I think we could just extend the "signal delay send" approach from x86-64 > to arm64, which is currently used by x86-64 on -rt kernel only. I understood that you were going to fix -rt, so I dropped this pending anything more from you. What's the plan? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-01-13 10:26 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-01-13 10:26 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > On 12/21/2015 9:00 AM, Will Deacon wrote: > >On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: > >>On Mon, 21 Dec 2015, Will Deacon wrote: > >>>+static void send_user_sigtrap(int si_code) > >>>+{ > >>>+ struct pt_regs *regs = current_pt_regs(); > >>>+ siginfo_t info = { > >>>+ .si_signo = SIGTRAP, > >>>+ .si_errno = 0, > >>>+ .si_code = si_code, > >>>+ .si_addr = (void __user *)instruction_pointer(regs), > >>>+ }; > >>>+ > >>>+ if (WARN_ON(!user_mode(regs))) > >>>+ return; > >>>+ > >>>+ preempt_disable(); > >> > >>That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, > >>which is a 'sleeping' spinlock on RT. > > > >Ah, I missed that :/ > > > >>Why would we need to disable preemption here at all? What's the problem of > >>being preempted or even migrated? > > > >There *might* not be a problem, I'm just really nervous about changing > >the behaviour on the debug path and subtly changing how ptrace behaves. > > > >My worry was that you could somehow get back into the tracer, and it > >could remove a software breakpoint in the knowledge that it wouldn't > >see any future (spurious) SIGTRAPs for that location. > > > >Without a concrete example, however, I guess I'll bite the bullet and > >enable irqs across the call to force_sig_info, since there is clearly a > >real issue here on RT. > > This might be buried in email storm during the holiday. Just want to double > check the status. I'm supposed there is no objection for getting it merged > in upstream? Sorry, when you replied with: > I think we could just extend the "signal delay send" approach from x86-64 > to arm64, which is currently used by x86-64 on -rt kernel only. I understood that you were going to fix -rt, so I dropped this pending anything more from you. What's the plan? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-01-13 10:26 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-01-13 10:26 UTC (permalink / raw) To: Shi, Yang Cc: linaro-kernel, linux-rt-users, Catalin.Marinas, linux-kernel, Thomas Gleixner, linux-arm-kernel On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > On 12/21/2015 9:00 AM, Will Deacon wrote: > >On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: > >>On Mon, 21 Dec 2015, Will Deacon wrote: > >>>+static void send_user_sigtrap(int si_code) > >>>+{ > >>>+ struct pt_regs *regs = current_pt_regs(); > >>>+ siginfo_t info = { > >>>+ .si_signo = SIGTRAP, > >>>+ .si_errno = 0, > >>>+ .si_code = si_code, > >>>+ .si_addr = (void __user *)instruction_pointer(regs), > >>>+ }; > >>>+ > >>>+ if (WARN_ON(!user_mode(regs))) > >>>+ return; > >>>+ > >>>+ preempt_disable(); > >> > >>That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, > >>which is a 'sleeping' spinlock on RT. > > > >Ah, I missed that :/ > > > >>Why would we need to disable preemption here at all? What's the problem of > >>being preempted or even migrated? > > > >There *might* not be a problem, I'm just really nervous about changing > >the behaviour on the debug path and subtly changing how ptrace behaves. > > > >My worry was that you could somehow get back into the tracer, and it > >could remove a software breakpoint in the knowledge that it wouldn't > >see any future (spurious) SIGTRAPs for that location. > > > >Without a concrete example, however, I guess I'll bite the bullet and > >enable irqs across the call to force_sig_info, since there is clearly a > >real issue here on RT. > > This might be buried in email storm during the holiday. Just want to double > check the status. I'm supposed there is no objection for getting it merged > in upstream? Sorry, when you replied with: > I think we could just extend the "signal delay send" approach from x86-64 > to arm64, which is currently used by x86-64 on -rt kernel only. I understood that you were going to fix -rt, so I dropped this pending anything more from you. What's the plan? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-01-13 10:26 ` Will Deacon @ 2016-01-13 17:17 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-01-13 17:17 UTC (permalink / raw) To: Will Deacon Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 1/13/2016 2:26 AM, Will Deacon wrote: > On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >> On 12/21/2015 9:00 AM, Will Deacon wrote: >>> On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: >>>> On Mon, 21 Dec 2015, Will Deacon wrote: >>>>> +static void send_user_sigtrap(int si_code) >>>>> +{ >>>>> + struct pt_regs *regs = current_pt_regs(); >>>>> + siginfo_t info = { >>>>> + .si_signo = SIGTRAP, >>>>> + .si_errno = 0, >>>>> + .si_code = si_code, >>>>> + .si_addr = (void __user *)instruction_pointer(regs), >>>>> + }; >>>>> + >>>>> + if (WARN_ON(!user_mode(regs))) >>>>> + return; >>>>> + >>>>> + preempt_disable(); >>>> >>>> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, >>>> which is a 'sleeping' spinlock on RT. >>> >>> Ah, I missed that :/ >>> >>>> Why would we need to disable preemption here at all? What's the problem of >>>> being preempted or even migrated? >>> >>> There *might* not be a problem, I'm just really nervous about changing >>> the behaviour on the debug path and subtly changing how ptrace behaves. >>> >>> My worry was that you could somehow get back into the tracer, and it >>> could remove a software breakpoint in the knowledge that it wouldn't >>> see any future (spurious) SIGTRAPs for that location. >>> >>> Without a concrete example, however, I guess I'll bite the bullet and >>> enable irqs across the call to force_sig_info, since there is clearly a >>> real issue here on RT. >> >> This might be buried in email storm during the holiday. Just want to double >> check the status. I'm supposed there is no objection for getting it merged >> in upstream? > > Sorry, when you replied with: > >> I think we could just extend the "signal delay send" approach from x86-64 >> to arm64, which is currently used by x86-64 on -rt kernel only. > > I understood that you were going to fix -rt, so I dropped this pending > anything more from you. > > What's the plan? Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt should be not necessary for arm64 right now. Reenabling interrupt is still the preferred approach. Since x86-64 has per-CPU IST exception stack, so preemption has to be disabled all the time. However, it is not applicable to other architectures for now, including arm64. Thanks, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-01-13 17:17 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-01-13 17:17 UTC (permalink / raw) To: linux-arm-kernel On 1/13/2016 2:26 AM, Will Deacon wrote: > On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >> On 12/21/2015 9:00 AM, Will Deacon wrote: >>> On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote: >>>> On Mon, 21 Dec 2015, Will Deacon wrote: >>>>> +static void send_user_sigtrap(int si_code) >>>>> +{ >>>>> + struct pt_regs *regs = current_pt_regs(); >>>>> + siginfo_t info = { >>>>> + .si_signo = SIGTRAP, >>>>> + .si_errno = 0, >>>>> + .si_code = si_code, >>>>> + .si_addr = (void __user *)instruction_pointer(regs), >>>>> + }; >>>>> + >>>>> + if (WARN_ON(!user_mode(regs))) >>>>> + return; >>>>> + >>>>> + preempt_disable(); >>>> >>>> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock, >>>> which is a 'sleeping' spinlock on RT. >>> >>> Ah, I missed that :/ >>> >>>> Why would we need to disable preemption here at all? What's the problem of >>>> being preempted or even migrated? >>> >>> There *might* not be a problem, I'm just really nervous about changing >>> the behaviour on the debug path and subtly changing how ptrace behaves. >>> >>> My worry was that you could somehow get back into the tracer, and it >>> could remove a software breakpoint in the knowledge that it wouldn't >>> see any future (spurious) SIGTRAPs for that location. >>> >>> Without a concrete example, however, I guess I'll bite the bullet and >>> enable irqs across the call to force_sig_info, since there is clearly a >>> real issue here on RT. >> >> This might be buried in email storm during the holiday. Just want to double >> check the status. I'm supposed there is no objection for getting it merged >> in upstream? > > Sorry, when you replied with: > >> I think we could just extend the "signal delay send" approach from x86-64 >> to arm64, which is currently used by x86-64 on -rt kernel only. > > I understood that you were going to fix -rt, so I dropped this pending > anything more from you. > > What's the plan? Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt should be not necessary for arm64 right now. Reenabling interrupt is still the preferred approach. Since x86-64 has per-CPU IST exception stack, so preemption has to be disabled all the time. However, it is not applicable to other architectures for now, including arm64. Thanks, Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-01-13 17:17 ` Shi, Yang @ 2016-01-13 17:23 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-01-13 17:23 UTC (permalink / raw) To: Shi, Yang Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: > On 1/13/2016 2:26 AM, Will Deacon wrote: > >On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > >>This might be buried in email storm during the holiday. Just want to double > >>check the status. I'm supposed there is no objection for getting it merged > >>in upstream? > > > >Sorry, when you replied with: > > > >>I think we could just extend the "signal delay send" approach from x86-64 > >>to arm64, which is currently used by x86-64 on -rt kernel only. > > > >I understood that you were going to fix -rt, so I dropped this pending > >anything more from you. > > > >What's the plan? > > Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt > should be not necessary for arm64 right now. Reenabling interrupt is still > the preferred approach. > > Since x86-64 has per-CPU IST exception stack, so preemption has to be > disabled all the time. However, it is not applicable to other architectures > for now, including arm64. Actually, we grew support for a separate IRQ stack in the recent merge window. Does that change things here, or are you referring to something else? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-01-13 17:23 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-01-13 17:23 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: > On 1/13/2016 2:26 AM, Will Deacon wrote: > >On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > >>This might be buried in email storm during the holiday. Just want to double > >>check the status. I'm supposed there is no objection for getting it merged > >>in upstream? > > > >Sorry, when you replied with: > > > >>I think we could just extend the "signal delay send" approach from x86-64 > >>to arm64, which is currently used by x86-64 on -rt kernel only. > > > >I understood that you were going to fix -rt, so I dropped this pending > >anything more from you. > > > >What's the plan? > > Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt > should be not necessary for arm64 right now. Reenabling interrupt is still > the preferred approach. > > Since x86-64 has per-CPU IST exception stack, so preemption has to be > disabled all the time. However, it is not applicable to other architectures > for now, including arm64. Actually, we grew support for a separate IRQ stack in the recent merge window. Does that change things here, or are you referring to something else? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-01-13 17:23 ` Will Deacon @ 2016-01-13 18:10 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-01-13 18:10 UTC (permalink / raw) To: Will Deacon Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 1/13/2016 9:23 AM, Will Deacon wrote: > On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: >> On 1/13/2016 2:26 AM, Will Deacon wrote: >>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >>>> This might be buried in email storm during the holiday. Just want to double >>>> check the status. I'm supposed there is no objection for getting it merged >>>> in upstream? >>> >>> Sorry, when you replied with: >>> >>>> I think we could just extend the "signal delay send" approach from x86-64 >>>> to arm64, which is currently used by x86-64 on -rt kernel only. >>> >>> I understood that you were going to fix -rt, so I dropped this pending >>> anything more from you. >>> >>> What's the plan? >> >> Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt >> should be not necessary for arm64 right now. Reenabling interrupt is still >> the preferred approach. >> >> Since x86-64 has per-CPU IST exception stack, so preemption has to be >> disabled all the time. However, it is not applicable to other architectures >> for now, including arm64. > > Actually, we grew support for a separate IRQ stack in the recent merge > window. Does that change things here, or are you referring to something > else? Had a quick look at the patches, it looks the irq stack is not nestable and it switches back to the original stack as long as irq handler is done before preempt happens. So, it sounds it won't change things here. Thanks., Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-01-13 18:10 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-01-13 18:10 UTC (permalink / raw) To: linux-arm-kernel On 1/13/2016 9:23 AM, Will Deacon wrote: > On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: >> On 1/13/2016 2:26 AM, Will Deacon wrote: >>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >>>> This might be buried in email storm during the holiday. Just want to double >>>> check the status. I'm supposed there is no objection for getting it merged >>>> in upstream? >>> >>> Sorry, when you replied with: >>> >>>> I think we could just extend the "signal delay send" approach from x86-64 >>>> to arm64, which is currently used by x86-64 on -rt kernel only. >>> >>> I understood that you were going to fix -rt, so I dropped this pending >>> anything more from you. >>> >>> What's the plan? >> >> Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt >> should be not necessary for arm64 right now. Reenabling interrupt is still >> the preferred approach. >> >> Since x86-64 has per-CPU IST exception stack, so preemption has to be >> disabled all the time. However, it is not applicable to other architectures >> for now, including arm64. > > Actually, we grew support for a separate IRQ stack in the recent merge > window. Does that change things here, or are you referring to something > else? Had a quick look at the patches, it looks the irq stack is not nestable and it switches back to the original stack as long as irq handler is done before preempt happens. So, it sounds it won't change things here. Thanks., Yang > > Will > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-01-13 18:10 ` Shi, Yang @ 2016-02-05 21:25 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-02-05 21:25 UTC (permalink / raw) To: Will Deacon Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 1/13/2016 10:10 AM, Shi, Yang wrote: > On 1/13/2016 9:23 AM, Will Deacon wrote: >> On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: >>> On 1/13/2016 2:26 AM, Will Deacon wrote: >>>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >>>>> This might be buried in email storm during the holiday. Just want >>>>> to double >>>>> check the status. I'm supposed there is no objection for getting it >>>>> merged >>>>> in upstream? >>>> >>>> Sorry, when you replied with: >>>> >>>>> I think we could just extend the "signal delay send" approach from >>>>> x86-64 >>>>> to arm64, which is currently used by x86-64 on -rt kernel only. >>>> >>>> I understood that you were going to fix -rt, so I dropped this pending >>>> anything more from you. >>>> >>>> What's the plan? >>> >>> Sorry for the confusion. The "signal delay send" approach used by >>> x86-64 -rt >>> should be not necessary for arm64 right now. Reenabling interrupt is >>> still >>> the preferred approach. >>> >>> Since x86-64 has per-CPU IST exception stack, so preemption has to be >>> disabled all the time. However, it is not applicable to other >>> architectures >>> for now, including arm64. >> >> Actually, we grew support for a separate IRQ stack in the recent merge >> window. Does that change things here, or are you referring to something >> else? > > Had a quick look at the patches, it looks the irq stack is not nestable > and it switches back to the original stack as long as irq handler is > done before preempt happens. So, it sounds it won't change things here. Just had a quick test on 4.5-rc1. It survives with kgdbts, ptrace and ltp. So, it sounds safe with the "separate IRQ stack" change. Thanks, Yang > > Thanks., > Yang > >> >> Will >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-02-05 21:25 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-02-05 21:25 UTC (permalink / raw) To: linux-arm-kernel On 1/13/2016 10:10 AM, Shi, Yang wrote: > On 1/13/2016 9:23 AM, Will Deacon wrote: >> On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: >>> On 1/13/2016 2:26 AM, Will Deacon wrote: >>>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >>>>> This might be buried in email storm during the holiday. Just want >>>>> to double >>>>> check the status. I'm supposed there is no objection for getting it >>>>> merged >>>>> in upstream? >>>> >>>> Sorry, when you replied with: >>>> >>>>> I think we could just extend the "signal delay send" approach from >>>>> x86-64 >>>>> to arm64, which is currently used by x86-64 on -rt kernel only. >>>> >>>> I understood that you were going to fix -rt, so I dropped this pending >>>> anything more from you. >>>> >>>> What's the plan? >>> >>> Sorry for the confusion. The "signal delay send" approach used by >>> x86-64 -rt >>> should be not necessary for arm64 right now. Reenabling interrupt is >>> still >>> the preferred approach. >>> >>> Since x86-64 has per-CPU IST exception stack, so preemption has to be >>> disabled all the time. However, it is not applicable to other >>> architectures >>> for now, including arm64. >> >> Actually, we grew support for a separate IRQ stack in the recent merge >> window. Does that change things here, or are you referring to something >> else? > > Had a quick look at the patches, it looks the irq stack is not nestable > and it switches back to the original stack as long as irq handler is > done before preempt happens. So, it sounds it won't change things here. Just had a quick test on 4.5-rc1. It survives with kgdbts, ptrace and ltp. So, it sounds safe with the "separate IRQ stack" change. Thanks, Yang > > Thanks., > Yang > >> >> Will >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-02-05 21:25 ` Shi, Yang @ 2016-02-11 13:54 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-02-11 13:54 UTC (permalink / raw) To: Shi, Yang Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On Fri, Feb 05, 2016 at 01:25:47PM -0800, Shi, Yang wrote: > On 1/13/2016 10:10 AM, Shi, Yang wrote: > >On 1/13/2016 9:23 AM, Will Deacon wrote: > >>On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: > >>>On 1/13/2016 2:26 AM, Will Deacon wrote: > >>>>On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > >>>>>This might be buried in email storm during the holiday. Just want > >>>>>to double > >>>>>check the status. I'm supposed there is no objection for getting it > >>>>>merged > >>>>>in upstream? > >>>> > >>>>Sorry, when you replied with: > >>>> > >>>>>I think we could just extend the "signal delay send" approach from > >>>>>x86-64 > >>>>>to arm64, which is currently used by x86-64 on -rt kernel only. > >>>> > >>>>I understood that you were going to fix -rt, so I dropped this pending > >>>>anything more from you. > >>>> > >>>>What's the plan? > >>> > >>>Sorry for the confusion. The "signal delay send" approach used by > >>>x86-64 -rt > >>>should be not necessary for arm64 right now. Reenabling interrupt is > >>>still > >>>the preferred approach. > >>> > >>>Since x86-64 has per-CPU IST exception stack, so preemption has to be > >>>disabled all the time. However, it is not applicable to other > >>>architectures > >>>for now, including arm64. > >> > >>Actually, we grew support for a separate IRQ stack in the recent merge > >>window. Does that change things here, or are you referring to something > >>else? > > > >Had a quick look at the patches, it looks the irq stack is not nestable > >and it switches back to the original stack as long as irq handler is > >done before preempt happens. So, it sounds it won't change things here. > > > Just had a quick test on 4.5-rc1. It survives with kgdbts, ptrace and ltp. > So, it sounds safe with the "separate IRQ stack" change. I quite liked the sigtrap consolidation in my earlier (broken) approach. Does the following work for you? Will --->8 >From e04a28d45ff343b47a4ffc4dee3a3e279e76ddfa Mon Sep 17 00:00:00 2001 From: Will Deacon <will.deacon@arm.com> Date: Wed, 10 Feb 2016 16:05:28 +0000 Subject: [PATCH] arm64: debug: re-enable irqs before sending breakpoint SIGTRAP force_sig_info can sleep under an -rt kernel, so attempting to send a breakpoint SIGTRAP with interrupts disabled yields the following BUG: BUG: sleeping function called from invalid context at /kernel-source/kernel/locking/rtmutex.c:917 in_atomic(): 0, irqs_disabled(): 128, pid: 551, name: test.sh CPU: 5 PID: 551 Comm: test.sh Not tainted 4.1.13-rt13 #7 Hardware name: Freescale Layerscape 2085a RDB Board (DT) Call trace: dump_backtrace+0x0/0x128 show_stack+0x24/0x30 dump_stack+0x80/0xa0 ___might_sleep+0x128/0x1a0 rt_spin_lock+0x2c/0x40 force_sig_info+0xcc/0x210 brk_handler.part.2+0x6c/0x80 brk_handler+0xd8/0xe8 do_debug_exception+0x58/0xb8 This patch fixes the problem by ensuring that interrupts are enabled prior to sending the SIGTRAP if they were already enabled in the user context. Reported-by: Yang Shi <yang.shi@linaro.org> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm64/kernel/debug-monitors.c | 48 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 8aee3aeec3e6..c536c9e307b9 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -226,11 +226,28 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr) return retval; } +static void send_user_sigtrap(int si_code) +{ + struct pt_regs *regs = current_pt_regs(); + siginfo_t info = { + .si_signo = SIGTRAP, + .si_errno = 0, + .si_code = si_code, + .si_addr = (void __user *)instruction_pointer(regs), + }; + + if (WARN_ON(!user_mode(regs))) + return; + + if (interrupts_enabled(regs)) + local_irq_enable(); + + force_sig_info(SIGTRAP, &info, current); +} + static int single_step_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - /* * If we are stepping a pending breakpoint, call the hw_breakpoint * handler first. @@ -239,11 +256,7 @@ static int single_step_handler(unsigned long addr, unsigned int esr, return 0; if (user_mode(regs)) { - info.si_signo = SIGTRAP; - info.si_errno = 0; - info.si_code = TRAP_HWBKPT; - info.si_addr = (void __user *)instruction_pointer(regs); - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_HWBKPT); /* * ptrace will disable single step unless explicitly @@ -307,17 +320,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr) static int brk_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - if (user_mode(regs)) { - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = (void __user *)instruction_pointer(regs), - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) { pr_warning("Unexpected kernel BRK exception at EL1\n"); return -EFAULT; @@ -328,7 +332,6 @@ static int brk_handler(unsigned long addr, unsigned int esr, int aarch32_break_handler(struct pt_regs *regs) { - siginfo_t info; u32 arm_instr; u16 thumb_instr; bool bp = false; @@ -359,14 +362,7 @@ int aarch32_break_handler(struct pt_regs *regs) if (!bp) return -EFAULT; - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = pc, - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-02-11 13:54 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-02-11 13:54 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 05, 2016 at 01:25:47PM -0800, Shi, Yang wrote: > On 1/13/2016 10:10 AM, Shi, Yang wrote: > >On 1/13/2016 9:23 AM, Will Deacon wrote: > >>On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: > >>>On 1/13/2016 2:26 AM, Will Deacon wrote: > >>>>On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: > >>>>>This might be buried in email storm during the holiday. Just want > >>>>>to double > >>>>>check the status. I'm supposed there is no objection for getting it > >>>>>merged > >>>>>in upstream? > >>>> > >>>>Sorry, when you replied with: > >>>> > >>>>>I think we could just extend the "signal delay send" approach from > >>>>>x86-64 > >>>>>to arm64, which is currently used by x86-64 on -rt kernel only. > >>>> > >>>>I understood that you were going to fix -rt, so I dropped this pending > >>>>anything more from you. > >>>> > >>>>What's the plan? > >>> > >>>Sorry for the confusion. The "signal delay send" approach used by > >>>x86-64 -rt > >>>should be not necessary for arm64 right now. Reenabling interrupt is > >>>still > >>>the preferred approach. > >>> > >>>Since x86-64 has per-CPU IST exception stack, so preemption has to be > >>>disabled all the time. However, it is not applicable to other > >>>architectures > >>>for now, including arm64. > >> > >>Actually, we grew support for a separate IRQ stack in the recent merge > >>window. Does that change things here, or are you referring to something > >>else? > > > >Had a quick look at the patches, it looks the irq stack is not nestable > >and it switches back to the original stack as long as irq handler is > >done before preempt happens. So, it sounds it won't change things here. > > > Just had a quick test on 4.5-rc1. It survives with kgdbts, ptrace and ltp. > So, it sounds safe with the "separate IRQ stack" change. I quite liked the sigtrap consolidation in my earlier (broken) approach. Does the following work for you? Will --->8 >From e04a28d45ff343b47a4ffc4dee3a3e279e76ddfa Mon Sep 17 00:00:00 2001 From: Will Deacon <will.deacon@arm.com> Date: Wed, 10 Feb 2016 16:05:28 +0000 Subject: [PATCH] arm64: debug: re-enable irqs before sending breakpoint SIGTRAP force_sig_info can sleep under an -rt kernel, so attempting to send a breakpoint SIGTRAP with interrupts disabled yields the following BUG: BUG: sleeping function called from invalid context at /kernel-source/kernel/locking/rtmutex.c:917 in_atomic(): 0, irqs_disabled(): 128, pid: 551, name: test.sh CPU: 5 PID: 551 Comm: test.sh Not tainted 4.1.13-rt13 #7 Hardware name: Freescale Layerscape 2085a RDB Board (DT) Call trace: dump_backtrace+0x0/0x128 show_stack+0x24/0x30 dump_stack+0x80/0xa0 ___might_sleep+0x128/0x1a0 rt_spin_lock+0x2c/0x40 force_sig_info+0xcc/0x210 brk_handler.part.2+0x6c/0x80 brk_handler+0xd8/0xe8 do_debug_exception+0x58/0xb8 This patch fixes the problem by ensuring that interrupts are enabled prior to sending the SIGTRAP if they were already enabled in the user context. Reported-by: Yang Shi <yang.shi@linaro.org> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm64/kernel/debug-monitors.c | 48 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 8aee3aeec3e6..c536c9e307b9 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -226,11 +226,28 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr) return retval; } +static void send_user_sigtrap(int si_code) +{ + struct pt_regs *regs = current_pt_regs(); + siginfo_t info = { + .si_signo = SIGTRAP, + .si_errno = 0, + .si_code = si_code, + .si_addr = (void __user *)instruction_pointer(regs), + }; + + if (WARN_ON(!user_mode(regs))) + return; + + if (interrupts_enabled(regs)) + local_irq_enable(); + + force_sig_info(SIGTRAP, &info, current); +} + static int single_step_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - /* * If we are stepping a pending breakpoint, call the hw_breakpoint * handler first. @@ -239,11 +256,7 @@ static int single_step_handler(unsigned long addr, unsigned int esr, return 0; if (user_mode(regs)) { - info.si_signo = SIGTRAP; - info.si_errno = 0; - info.si_code = TRAP_HWBKPT; - info.si_addr = (void __user *)instruction_pointer(regs); - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_HWBKPT); /* * ptrace will disable single step unless explicitly @@ -307,17 +320,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr) static int brk_handler(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - siginfo_t info; - if (user_mode(regs)) { - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = (void __user *)instruction_pointer(regs), - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) { pr_warning("Unexpected kernel BRK exception at EL1\n"); return -EFAULT; @@ -328,7 +332,6 @@ static int brk_handler(unsigned long addr, unsigned int esr, int aarch32_break_handler(struct pt_regs *regs) { - siginfo_t info; u32 arm_instr; u16 thumb_instr; bool bp = false; @@ -359,14 +362,7 @@ int aarch32_break_handler(struct pt_regs *regs) if (!bp) return -EFAULT; - info = (siginfo_t) { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_BRKPT, - .si_addr = pc, - }; - - force_sig_info(SIGTRAP, &info, current); + send_user_sigtrap(TRAP_BRKPT); return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint 2016-02-11 13:54 ` Will Deacon @ 2016-02-11 17:29 ` Shi, Yang -1 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-02-11 17:29 UTC (permalink / raw) To: Will Deacon Cc: Thomas Gleixner, Catalin.Marinas, linux-kernel, linux-arm-kernel, linaro-kernel, linux-rt-users On 2/11/2016 5:54 AM, Will Deacon wrote: > On Fri, Feb 05, 2016 at 01:25:47PM -0800, Shi, Yang wrote: >> On 1/13/2016 10:10 AM, Shi, Yang wrote: >>> On 1/13/2016 9:23 AM, Will Deacon wrote: >>>> On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: >>>>> On 1/13/2016 2:26 AM, Will Deacon wrote: >>>>>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >>>>>>> This might be buried in email storm during the holiday. Just want >>>>>>> to double >>>>>>> check the status. I'm supposed there is no objection for getting it >>>>>>> merged >>>>>>> in upstream? >>>>>> >>>>>> Sorry, when you replied with: >>>>>> >>>>>>> I think we could just extend the "signal delay send" approach from >>>>>>> x86-64 >>>>>>> to arm64, which is currently used by x86-64 on -rt kernel only. >>>>>> >>>>>> I understood that you were going to fix -rt, so I dropped this pending >>>>>> anything more from you. >>>>>> >>>>>> What's the plan? >>>>> >>>>> Sorry for the confusion. The "signal delay send" approach used by >>>>> x86-64 -rt >>>>> should be not necessary for arm64 right now. Reenabling interrupt is >>>>> still >>>>> the preferred approach. >>>>> >>>>> Since x86-64 has per-CPU IST exception stack, so preemption has to be >>>>> disabled all the time. However, it is not applicable to other >>>>> architectures >>>>> for now, including arm64. >>>> >>>> Actually, we grew support for a separate IRQ stack in the recent merge >>>> window. Does that change things here, or are you referring to something >>>> else? >>> >>> Had a quick look at the patches, it looks the irq stack is not nestable >>> and it switches back to the original stack as long as irq handler is >>> done before preempt happens. So, it sounds it won't change things here. >> >> >> Just had a quick test on 4.5-rc1. It survives with kgdbts, ptrace and ltp. >> So, it sounds safe with the "separate IRQ stack" change. > > I quite liked the sigtrap consolidation in my earlier (broken) approach. > Does the following work for you? Yes, sure. One minor comment below. > > Will > > --->8 > > From e04a28d45ff343b47a4ffc4dee3a3e279e76ddfa Mon Sep 17 00:00:00 2001 > From: Will Deacon <will.deacon@arm.com> > Date: Wed, 10 Feb 2016 16:05:28 +0000 > Subject: [PATCH] arm64: debug: re-enable irqs before sending breakpoint > SIGTRAP > > force_sig_info can sleep under an -rt kernel, so attempting to send a > breakpoint SIGTRAP with interrupts disabled yields the following BUG: > > BUG: sleeping function called from invalid context at > /kernel-source/kernel/locking/rtmutex.c:917 > in_atomic(): 0, irqs_disabled(): 128, pid: 551, name: test.sh > CPU: 5 PID: 551 Comm: test.sh Not tainted 4.1.13-rt13 #7 > Hardware name: Freescale Layerscape 2085a RDB Board (DT) > Call trace: > dump_backtrace+0x0/0x128 > show_stack+0x24/0x30 > dump_stack+0x80/0xa0 > ___might_sleep+0x128/0x1a0 > rt_spin_lock+0x2c/0x40 > force_sig_info+0xcc/0x210 > brk_handler.part.2+0x6c/0x80 > brk_handler+0xd8/0xe8 > do_debug_exception+0x58/0xb8 > > This patch fixes the problem by ensuring that interrupts are enabled > prior to sending the SIGTRAP if they were already enabled in the user > context. > > Reported-by: Yang Shi <yang.shi@linaro.org> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/kernel/debug-monitors.c | 48 +++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 8aee3aeec3e6..c536c9e307b9 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -226,11 +226,28 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr) > return retval; > } > > +static void send_user_sigtrap(int si_code) > +{ > + struct pt_regs *regs = current_pt_regs(); > + siginfo_t info = { > + .si_signo = SIGTRAP, > + .si_errno = 0, > + .si_code = si_code, > + .si_addr = (void __user *)instruction_pointer(regs), > + }; > + > + if (WARN_ON(!user_mode(regs))) Maybe BUG_ON should be used here? Since send_user_sigtrap is called only when user_mode is positive, and interrupt is disabled at this point, so if it is not positive, it sounds list a bug. Thanks, Yang > + return; > + > + if (interrupts_enabled(regs)) > + local_irq_enable(); > + > + force_sig_info(SIGTRAP, &info, current); > +} > + > static int single_step_handler(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - siginfo_t info; > - > /* > * If we are stepping a pending breakpoint, call the hw_breakpoint > * handler first. > @@ -239,11 +256,7 @@ static int single_step_handler(unsigned long addr, unsigned int esr, > return 0; > > if (user_mode(regs)) { > - info.si_signo = SIGTRAP; > - info.si_errno = 0; > - info.si_code = TRAP_HWBKPT; > - info.si_addr = (void __user *)instruction_pointer(regs); > - force_sig_info(SIGTRAP, &info, current); > + send_user_sigtrap(TRAP_HWBKPT); > > /* > * ptrace will disable single step unless explicitly > @@ -307,17 +320,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr) > static int brk_handler(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - siginfo_t info; > - > if (user_mode(regs)) { > - info = (siginfo_t) { > - .si_signo = SIGTRAP, > - .si_errno = 0, > - .si_code = TRAP_BRKPT, > - .si_addr = (void __user *)instruction_pointer(regs), > - }; > - > - force_sig_info(SIGTRAP, &info, current); > + send_user_sigtrap(TRAP_BRKPT); > } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) { > pr_warning("Unexpected kernel BRK exception at EL1\n"); > return -EFAULT; > @@ -328,7 +332,6 @@ static int brk_handler(unsigned long addr, unsigned int esr, > > int aarch32_break_handler(struct pt_regs *regs) > { > - siginfo_t info; > u32 arm_instr; > u16 thumb_instr; > bool bp = false; > @@ -359,14 +362,7 @@ int aarch32_break_handler(struct pt_regs *regs) > if (!bp) > return -EFAULT; > > - info = (siginfo_t) { > - .si_signo = SIGTRAP, > - .si_errno = 0, > - .si_code = TRAP_BRKPT, > - .si_addr = pc, > - }; > - > - force_sig_info(SIGTRAP, &info, current); > + send_user_sigtrap(TRAP_BRKPT); > return 0; > } > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] arm64: reenable interrupt when handling ptrace breakpoint @ 2016-02-11 17:29 ` Shi, Yang 0 siblings, 0 replies; 31+ messages in thread From: Shi, Yang @ 2016-02-11 17:29 UTC (permalink / raw) To: linux-arm-kernel On 2/11/2016 5:54 AM, Will Deacon wrote: > On Fri, Feb 05, 2016 at 01:25:47PM -0800, Shi, Yang wrote: >> On 1/13/2016 10:10 AM, Shi, Yang wrote: >>> On 1/13/2016 9:23 AM, Will Deacon wrote: >>>> On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote: >>>>> On 1/13/2016 2:26 AM, Will Deacon wrote: >>>>>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote: >>>>>>> This might be buried in email storm during the holiday. Just want >>>>>>> to double >>>>>>> check the status. I'm supposed there is no objection for getting it >>>>>>> merged >>>>>>> in upstream? >>>>>> >>>>>> Sorry, when you replied with: >>>>>> >>>>>>> I think we could just extend the "signal delay send" approach from >>>>>>> x86-64 >>>>>>> to arm64, which is currently used by x86-64 on -rt kernel only. >>>>>> >>>>>> I understood that you were going to fix -rt, so I dropped this pending >>>>>> anything more from you. >>>>>> >>>>>> What's the plan? >>>>> >>>>> Sorry for the confusion. The "signal delay send" approach used by >>>>> x86-64 -rt >>>>> should be not necessary for arm64 right now. Reenabling interrupt is >>>>> still >>>>> the preferred approach. >>>>> >>>>> Since x86-64 has per-CPU IST exception stack, so preemption has to be >>>>> disabled all the time. However, it is not applicable to other >>>>> architectures >>>>> for now, including arm64. >>>> >>>> Actually, we grew support for a separate IRQ stack in the recent merge >>>> window. Does that change things here, or are you referring to something >>>> else? >>> >>> Had a quick look at the patches, it looks the irq stack is not nestable >>> and it switches back to the original stack as long as irq handler is >>> done before preempt happens. So, it sounds it won't change things here. >> >> >> Just had a quick test on 4.5-rc1. It survives with kgdbts, ptrace and ltp. >> So, it sounds safe with the "separate IRQ stack" change. > > I quite liked the sigtrap consolidation in my earlier (broken) approach. > Does the following work for you? Yes, sure. One minor comment below. > > Will > > --->8 > > From e04a28d45ff343b47a4ffc4dee3a3e279e76ddfa Mon Sep 17 00:00:00 2001 > From: Will Deacon <will.deacon@arm.com> > Date: Wed, 10 Feb 2016 16:05:28 +0000 > Subject: [PATCH] arm64: debug: re-enable irqs before sending breakpoint > SIGTRAP > > force_sig_info can sleep under an -rt kernel, so attempting to send a > breakpoint SIGTRAP with interrupts disabled yields the following BUG: > > BUG: sleeping function called from invalid context at > /kernel-source/kernel/locking/rtmutex.c:917 > in_atomic(): 0, irqs_disabled(): 128, pid: 551, name: test.sh > CPU: 5 PID: 551 Comm: test.sh Not tainted 4.1.13-rt13 #7 > Hardware name: Freescale Layerscape 2085a RDB Board (DT) > Call trace: > dump_backtrace+0x0/0x128 > show_stack+0x24/0x30 > dump_stack+0x80/0xa0 > ___might_sleep+0x128/0x1a0 > rt_spin_lock+0x2c/0x40 > force_sig_info+0xcc/0x210 > brk_handler.part.2+0x6c/0x80 > brk_handler+0xd8/0xe8 > do_debug_exception+0x58/0xb8 > > This patch fixes the problem by ensuring that interrupts are enabled > prior to sending the SIGTRAP if they were already enabled in the user > context. > > Reported-by: Yang Shi <yang.shi@linaro.org> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > arch/arm64/kernel/debug-monitors.c | 48 +++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c > index 8aee3aeec3e6..c536c9e307b9 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -226,11 +226,28 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr) > return retval; > } > > +static void send_user_sigtrap(int si_code) > +{ > + struct pt_regs *regs = current_pt_regs(); > + siginfo_t info = { > + .si_signo = SIGTRAP, > + .si_errno = 0, > + .si_code = si_code, > + .si_addr = (void __user *)instruction_pointer(regs), > + }; > + > + if (WARN_ON(!user_mode(regs))) Maybe BUG_ON should be used here? Since send_user_sigtrap is called only when user_mode is positive, and interrupt is disabled at this point, so if it is not positive, it sounds list a bug. Thanks, Yang > + return; > + > + if (interrupts_enabled(regs)) > + local_irq_enable(); > + > + force_sig_info(SIGTRAP, &info, current); > +} > + > static int single_step_handler(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - siginfo_t info; > - > /* > * If we are stepping a pending breakpoint, call the hw_breakpoint > * handler first. > @@ -239,11 +256,7 @@ static int single_step_handler(unsigned long addr, unsigned int esr, > return 0; > > if (user_mode(regs)) { > - info.si_signo = SIGTRAP; > - info.si_errno = 0; > - info.si_code = TRAP_HWBKPT; > - info.si_addr = (void __user *)instruction_pointer(regs); > - force_sig_info(SIGTRAP, &info, current); > + send_user_sigtrap(TRAP_HWBKPT); > > /* > * ptrace will disable single step unless explicitly > @@ -307,17 +320,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr) > static int brk_handler(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - siginfo_t info; > - > if (user_mode(regs)) { > - info = (siginfo_t) { > - .si_signo = SIGTRAP, > - .si_errno = 0, > - .si_code = TRAP_BRKPT, > - .si_addr = (void __user *)instruction_pointer(regs), > - }; > - > - force_sig_info(SIGTRAP, &info, current); > + send_user_sigtrap(TRAP_BRKPT); > } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) { > pr_warning("Unexpected kernel BRK exception at EL1\n"); > return -EFAULT; > @@ -328,7 +332,6 @@ static int brk_handler(unsigned long addr, unsigned int esr, > > int aarch32_break_handler(struct pt_regs *regs) > { > - siginfo_t info; > u32 arm_instr; > u16 thumb_instr; > bool bp = false; > @@ -359,14 +362,7 @@ int aarch32_break_handler(struct pt_regs *regs) > if (!bp) > return -EFAULT; > > - info = (siginfo_t) { > - .si_signo = SIGTRAP, > - .si_errno = 0, > - .si_code = TRAP_BRKPT, > - .si_addr = pc, > - }; > - > - force_sig_info(SIGTRAP, &info, current); > + send_user_sigtrap(TRAP_BRKPT); > return 0; > } > > ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2016-02-11 17:30 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-16 0:18 [PATCH] arm64: reenable interrupt when handling ptrace breakpoint Yang Shi 2015-12-16 0:18 ` Yang Shi 2015-12-16 11:13 ` Will Deacon 2015-12-16 11:13 ` Will Deacon 2015-12-16 20:45 ` Shi, Yang 2015-12-16 20:45 ` Shi, Yang 2015-12-21 10:48 ` Will Deacon 2015-12-21 10:48 ` Will Deacon 2015-12-21 16:51 ` Thomas Gleixner 2015-12-21 16:51 ` Thomas Gleixner 2015-12-21 17:00 ` Will Deacon 2015-12-21 17:00 ` Will Deacon 2015-12-21 17:27 ` Shi, Yang 2015-12-21 17:27 ` Shi, Yang 2016-01-12 19:59 ` Shi, Yang 2016-01-12 19:59 ` Shi, Yang 2016-01-13 10:26 ` Will Deacon 2016-01-13 10:26 ` Will Deacon 2016-01-13 10:26 ` Will Deacon 2016-01-13 17:17 ` Shi, Yang 2016-01-13 17:17 ` Shi, Yang 2016-01-13 17:23 ` Will Deacon 2016-01-13 17:23 ` Will Deacon 2016-01-13 18:10 ` Shi, Yang 2016-01-13 18:10 ` Shi, Yang 2016-02-05 21:25 ` Shi, Yang 2016-02-05 21:25 ` Shi, Yang 2016-02-11 13:54 ` Will Deacon 2016-02-11 13:54 ` Will Deacon 2016-02-11 17:29 ` Shi, Yang 2016-02-11 17:29 ` Shi, Yang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.