From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753520AbdBKJWw (ORCPT ); Sat, 11 Feb 2017 04:22:52 -0500 Received: from mail.kernel.org ([198.145.29.136]:34192 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750988AbdBKJWt (ORCPT ); Sat, 11 Feb 2017 04:22:49 -0500 Date: Sat, 11 Feb 2017 18:21:48 +0900 From: Masami Hiramatsu To: Russell King - ARM Linux 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: <20170211182148.01c3b8209a5749d2a5192914@kernel.org> In-Reply-To: <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> <20170210232113.GP27312@n2100.armlinux.org.uk> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 10 Feb 2017 23:21:13 +0000 Russell King - ARM Linux wrote: > 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. Right. > 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. OK, this one should be a bug on arm implementation. On x86, we also check status == KPROBE_HIT_SS too, see reenter_kprobe() at arch/x86/kernel/kprobes/core.c. (see commit 6a5022a56) It seems same issue on arm64. I'll fix that. > 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". What would you mean higher level? Thank you, -- Masami Hiramatsu