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.dea...@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....@linaro.org>
Signed-off-by: Will Deacon <will.dea...@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
