From patchwork Thu Jul 28 14:40:54 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Catalin Marinas X-Patchwork-Id: 72969 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp888722qga; Thu, 28 Jul 2016 07:41:19 -0700 (PDT) X-Received: by 10.66.135.40 with SMTP id pp8mr60448797pab.113.1469716879891; Thu, 28 Jul 2016 07:41:19 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id gw1si12743088pac.108.2016.07.28.07.41.19; Thu, 28 Jul 2016 07:41:19 -0700 (PDT) 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 S1758765AbcG1OlS (ORCPT + 29 others); Thu, 28 Jul 2016 10:41:18 -0400 Received: from foss.arm.com ([217.140.101.70]:49389 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758655AbcG1OlC (ORCPT ); Thu, 28 Jul 2016 10:41:02 -0400 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 ECD3B28; Thu, 28 Jul 2016 07:42:18 -0700 (PDT) Received: from e104818-lin.cambridge.arm.com (e104818-lin.cambridge.arm.com [10.1.206.48]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BC9453F21A; Thu, 28 Jul 2016 07:40:56 -0700 (PDT) Date: Thu, 28 Jul 2016 15:40:54 +0100 From: Catalin Marinas To: David Long Cc: Daniel Thompson , Mark Rutland , Yang Shi , Zi Shen Lim , Will Deacon , Andrey Ryabinin , yalin wang , Li Bin , Jisheng Zhang , John Blackwood , Pratyush Anand , Huang Shijie , Dave P Martin , Petr Mladek , Vladimir Murzin , Steve Capper , Suzuki K Poulose , Marc Zyngier , Mark Brown , Sandeepa Prabhu , William Cohen , Alex =?iso-8859-1?Q?Benn=E9e?= , Adam Buchbinder , linux-arm-kernel@lists.infradead.org, Ard Biesheuvel , linux-kernel@vger.kernel.org, James Morse , Masami Hiramatsu , Andrew Morton , Robin Murphy , Jens Wiklander , Christoffer Dall Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support Message-ID: <20160728144053.GA26510@e104818-lin.cambridge.arm.com> References: <578FA238.3050206@arm.com> <5790F960.5050007@linaro.org> <57910528.7070902@arm.com> <57911590.50305@linaro.org> <20160722101617.GA17821@e104818-lin.cambridge.arm.com> <57924104.1080202@linaro.org> <20160725171350.GE2423@e104818-lin.cambridge.arm.com> <57969234.1070201@linaro.org> <22b277ba-6812-a0dd-9e8e-c29bdb3aa672@linaro.org> <57993211.1040600@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57993211.1040600@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 Wed, Jul 27, 2016 at 06:13:37PM -0400, David Long wrote: > On 07/27/2016 07:50 AM, Daniel Thompson wrote: > >On 25/07/16 23:27, David Long wrote: > >>On 07/25/2016 01:13 PM, Catalin Marinas wrote: > >>>The problem is that the original design was done on x86 for its PCS and > >>>it doesn't always fit other architectures. So we could either ignore the > >>>problem, hoping that no probed function requires argument passing on > >>>stack or we copy all the valid data on the kernel stack: > >>> > >>>diff --git a/arch/arm64/include/asm/kprobes.h > >>>b/arch/arm64/include/asm/kprobes.h > >>>index 61b49150dfa3..157fd0d0aa08 100644 > >>>--- a/arch/arm64/include/asm/kprobes.h > >>>+++ b/arch/arm64/include/asm/kprobes.h > >>>@@ -22,7 +22,7 @@ > >>> > >>> #define __ARCH_WANT_KPROBES_INSN_SLOT > >>> #define MAX_INSN_SIZE 1 > >>>-#define MAX_STACK_SIZE 128 > >>>+#define MAX_STACK_SIZE THREAD_SIZE > >>> > >>> #define flush_insn_slot(p) do { } while (0) > >>> #define kretprobe_blacklist_size 0 > >> > >>I doubt the ARM PCS is unusual. At any rate I'm certain there are other > >>architectures that pass aggregate parameters on the stack. I suspect > >>other RISC(-ish) architectures have similar PCS issues and I think this > >>is at least a big part of where this simple copy with a 64/128 limit > >>comes from, or at least why it continues to exist. That said, I'm not > >>enthusiastic about researching that assertion in detail as it could be > >>time consuming. > > > >Given Mark shared a test program I *was* curious enough to take a look > >at this. > > > >The only architecture I can find that behaves like arm64 with the > >implicit pass-by-reference described by Catalin/Mark is sparc64. > > > >In contrast alpha, arm (32-bit), hppa64, mips64 and powerpc64 all use a > >hybrid approach where the first fragments of the structure are passed in > >registers and the remainder on the stack. > > That's interesting. It also looks like sparc64 does not copy any stack for > jprobes. I guess that approach at least makes it clear what will and won't > work. I suggest we do the same for arm64 - avoid the copying entirely as it's not safe anyway. We don't know how much to copy, nor can we be sure it is safe (see Dave's DMA to the stack example). This would need to be documented in the kprobes.txt file and MAX_STACK_SIZE removed from the arm64 kprobes support. There is also the case that Daniel was talking about - passing more than 8 arguments. I don't think it's worth handling this but we should at least add a warning and skip the probe: Unfortunately, we don't really have a way to detect large composite types passed as arguments, so we only have to rely on the documentation. Can you please submit a patch that removes MAX_STACK_SIZE for arm64, documents it and include the above hunk (once tested that it actually does what it intends to). Thanks. -- Catalin diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index bf9768588288..84e02606ec3d 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -491,6 +491,10 @@ int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs) struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); long stack_ptr = kernel_stack_pointer(regs); + /* do not allow arguments passed on the stack */ + if (WARN_ON_ONCE(regs->sp != regs->regs[29])) + return 0; + kcb->jprobe_saved_regs = *regs; /* * As Linus pointed out, gcc assumes that the callee