From patchwork Thu Feb 11 13:54:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 61762 Delivered-To: patch@linaro.org Received: by 10.112.43.199 with SMTP id y7csp203841lbl; Thu, 11 Feb 2016 05:54:37 -0800 (PST) X-Received: by 10.66.55.6 with SMTP id n6mr66445895pap.33.1455198877373; Thu, 11 Feb 2016 05:54:37 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id rr5si12743097pab.188.2016.02.11.05.54.36; Thu, 11 Feb 2016 05:54:37 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932098AbcBKNyf (ORCPT + 30 others); Thu, 11 Feb 2016 08:54:35 -0500 Received: from foss.arm.com ([217.140.101.70]:54325 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752347AbcBKNyd (ORCPT ); Thu, 11 Feb 2016 08:54:33 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 594DA49; Thu, 11 Feb 2016 05:53:45 -0800 (PST) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A329C3F2E5; Thu, 11 Feb 2016 05:54:31 -0800 (PST) Date: Thu, 11 Feb 2016 13:54:36 +0000 From: Will Deacon To: "Shi, Yang" Cc: Thomas Gleixner , Catalin.Marinas@arm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linaro-kernel@lists.linaro.org, linux-rt-users@vger.kernel.org Subject: Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint Message-ID: <20160211135435.GG32084@arm.com> References: <5671CD5B.9030907@linaro.org> <20151221104818.GF23092@arm.com> <20151221170028.GT23092@arm.com> <56955B3A.5010303@linaro.org> <20160113102622.GC25458@arm.com> <569686BA.6050703@linaro.org> <20160113172303.GH25458@arm.com> <56969312.7070309@linaro.org> <56B5135B.3050801@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <56B5135B.3050801@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 Signed-off-by: Will Deacon --- arch/arm64/kernel/debug-monitors.c | 48 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 26 deletions(-) -- 2.1.4 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; }