Groups > linux.kernel > #1331963 > unrolled thread

Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint

Started by	Will Deacon <will.deacon@arm.com>
First post	2016-02-11 15:00 +0100
Last post	2016-02-11 18:40 +0100
Articles	2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint Will Deacon <will.deacon@arm.com> - 2016-02-11 15:00 +0100
    Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint "Shi, Yang" <yang.shi@linaro.org> - 2016-02-11 18:40 +0100

#1331963 — Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint

From	Will Deacon <will.deacon@arm.com>
Date	2016-02-11 15:00 +0100
Subject	Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint
Message-ID	<r0ZWP-17m-29@gated-at.bofh.it>

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

[toc] | [next] | [standalone]

#1332262

From	"Shi, Yang" <yang.shi@linaro.org>
Date	2016-02-11 18:40 +0100
Message-ID	<r13nI-3uV-23@gated-at.bofh.it>
In reply to	#1331963

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;
>   }
>
>

[toc] | [prev] | [standalone]

csiph-web

Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint

Contents

#1331963 — Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint

#1332262