From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752046AbdBJXVd (ORCPT ); Fri, 10 Feb 2017 18:21:33 -0500 Received: from pandora.armlinux.org.uk ([78.32.30.218]:33138 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751798AbdBJXVc (ORCPT ); Fri, 10 Feb 2017 18:21:32 -0500 Date: Fri, 10 Feb 2017 23:21:13 +0000 From: Russell King - ARM Linux To: Masami Hiramatsu Cc: Ingo Molnar , linux-kernel@vger.kernel.org, Peter Zijlstra , Ananth N Mavinakayanahalli , Thomas Gleixner , "H . Peter Anvin" , Jon Medhurst , Wang Nan , Catalin Marinas , Will Deacon , "David A . Long" , Sandeepa Prabhu Subject: Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe Message-ID: <20170210232113.GP27312@n2100.armlinux.org.uk> References: <148665771695.22817.16393459806489781531.stgit@devbox> <148665793195.22817.17284342421982011651.stgit@devbox> <20170209164859.GI27312@n2100.armlinux.org.uk> <20170210113445.dc7bc683e77dd7ba021663b1@kernel.org> <20170211073316.bde61bc3706ede8261bf991d@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170211073316.bde61bc3706ede8261bf991d@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Feb 11, 2017 at 07:33:16AM +0900, Masami Hiramatsu wrote: > On Fri, 10 Feb 2017 11:34:45 +0900 > Masami Hiramatsu wrote: > > Ah, in_nmi() means FIQ on arm :) > > OK, but actually it is too late to check it in the enter of > > trampoline_handler() since we don't know where is the real > > return address at that point. So I'll check that in setup site > > - kretprobe_pre_handler(). > > Hmm, pre_handler_kretprobe() already checked in_nmi(). > So, I think this will no problem on FIQ too. I don't blame you for missing that - the tracing and probes code is (at least to me) quite a maze of code. >>From what I can tell, you're right - pre_handler_kretprobe() checks in_nmi() early on, which prevents arch_prepare_kretprobe() (which replaces regs->ARM_lr with the trampoline address) being run. Hence, the trampoline should not be run if we were entered in FIQ mode. However, looking at kprobe_handler(), I'm much less convinced. This is called as a result of hitting a probe instruction via kprobe_trap_handler(). Now, if we have two kprobes, one in non-FIQ context and one in FIQ context, and the non-FIQ context one is hit, we set the current kprobe: } else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) { /* Probe hit and conditional execution check ok. */ set_current_kprobe(p); kcb->kprobe_status = KPROBE_HIT_ACTIVE; and call the pre-handler (which succeeds.) If we then take a FIQ and hit a kprobe in a function called from FIQ, we will re-enter this function. In this case, "cur" will be the non-FIQ kprobe, and "p" will be the FIQ kprobe. It looks to me like we will single-step over the kprobe, and resume. However, it will modify the kprobe_status to KPROBE_REENTER, which may not be desirable. However, there does seem to be a hole. Let's say that we have a similar scenario, except that the FIQ is well-timed to happen: if (!p->pre_handler || !p->pre_handler(p, regs)) { kcb->kprobe_status = KPROBE_HIT_SS; /* HERE */ singlestep(p, regs, kcb); if (p->post_handler) { kcb->kprobe_status = KPROBE_HIT_SSDONE; In that case: /* Kprobe is pending, so we're recursing. */ switch (kcb->kprobe_status) { case KPROBE_HIT_ACTIVE: case KPROBE_HIT_SSDONE: ... default: /* impossible cases */ BUG(); becomes not such an "impossible case", so the kernel is likely to explode. This doesn't look good to me, and the pre-handler does nothing to prevent this, so I still think we need some higher level protection in kprobe_handler() against being entered in FIQ context - not only to prevent that BUG() but also to prevent the kprobe status being changed to "re-enter". -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.