From patchwork Mon Jul 25 17:13:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Catalin Marinas X-Patchwork-Id: 72746 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp1254389qga; Mon, 25 Jul 2016 10:14:10 -0700 (PDT) X-Received: by 10.98.30.133 with SMTP id e127mr31607334pfe.104.1469466850794; Mon, 25 Jul 2016 10:14:10 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id oo4si34478785pac.59.2016.07.25.10.14.10; Mon, 25 Jul 2016 10:14:10 -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 S1753232AbcGYROC (ORCPT + 29 others); Mon, 25 Jul 2016 13:14:02 -0400 Received: from foss.arm.com ([217.140.101.70]:35087 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752265AbcGYRN7 (ORCPT ); Mon, 25 Jul 2016 13:13:59 -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 DCDC428; Mon, 25 Jul 2016 10:15:12 -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 200BA3F213; Mon, 25 Jul 2016 10:13:52 -0700 (PDT) Date: Mon, 25 Jul 2016 18:13:50 +0100 From: Catalin Marinas To: David Long Cc: Mark Rutland , Petr Mladek , Zi Shen Lim , Will Deacon , Andrey Ryabinin , yalin wang , Li Bin , John Blackwood , Pratyush Anand , Daniel Thompson , Huang Shijie , Dave P Martin , Jisheng Zhang , Vladimir Murzin , Steve Capper , Suzuki K Poulose , Marc Zyngier , Yang Shi , 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: <20160725171350.GE2423@e104818-lin.cambridge.arm.com> References: <1467995754-32508-1-git-send-email-dave.long@linaro.org> <1467995754-32508-5-git-send-email-dave.long@linaro.org> <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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57924104.1080202@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, Jul 22, 2016 at 11:51:32AM -0400, David Long wrote: > On 07/22/2016 06:16 AM, Catalin Marinas wrote: > >On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote: > >>On 07/21/2016 01:23 PM, Marc Zyngier wrote: > >>>On 21/07/16 17:33, David Long wrote: > >>>>On 07/20/2016 12:09 PM, Marc Zyngier wrote: > >>>>>On 08/07/16 17:35, David Long wrote: > >>>>>>+#define MAX_INSN_SIZE 1 > >>>>>>+#define MAX_STACK_SIZE 128 > >>>>> > >>>>>Where is that value coming from? Because even on my 6502, I have a 256 > >>>>>byte stack. > >>>>> > >>>> > >>>>Although I don't claim to know the original author's thoughts I would > >>>>guess it is based on the seven other existing implementations for > >>>>kprobes on various architectures, all of which appear to use either 64 > >>>>or 128 for MAX_STACK_SIZE. The code is not trying to duplicate the > >>>>whole stack. > >[...] > >>>My main worry is that whatever value you pick, it is always going to be > >>>wrong. This is used to preserve arguments that are passed on the stack, > >>>as opposed to passed by registers). We have no idea of what is getting > >>>passed there so saving nothing, 128 bytes or 2kB is about the same. It > >>>is always wrong. > >>> > >>>A much better solution would be to check the frame pointer, and copy the > >>>delta between FP and SP, assuming it fits inside the allocated buffer. > >>>If it doesn't, or if FP is invalid, we just skip the hook, because we > >>>can't reliably execute it. > >> > >>Well, this is the way it works literally everywhere else. It is a documented > >>limitation (Documentation/kprobes.txt). Said documentation may need to be > >>changed along with the suggested fix. > > > >The document states: "Up to MAX_STACK_SIZE bytes are copied". That means > >the arch code could always copy less but never more than MAX_STACK_SIZE. > >What we are proposing is that we should try to guess how much to copy > >based on the FP value (caller's frame) and, if larger than > >MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes > >against the kprobes.txt document but at least it (a) may improve the > >performance slightly by avoiding unnecessary copy and (b) it avoids > >undefined behaviour if we ever encounter a jprobe with arguments passed > >on the stack beyond MAX_STACK_SIZE. > > OK, it sounds like an improvement. I do worry a little about unexpected side > effects. You get more unexpected side effects by not saving/restoring the whole stack. We looked into this on Friday and came to the conclusion that there is no safe way for kprobes to know which arguments passed on the stack should be preserved, at least not with the current API. Basically the AArch64 PCS states that for arguments passed on the stack (e.g. they can't fit in registers), the caller allocates memory for them (on its own stack) and passes the pointer to the callee. Unfortunately, the frame pointer seems to be decremented correspondingly to cover the arguments, so we don't really have a way to tell how much to copy. Copying just the caller's stack frame isn't safe either since a callee/caller receiving such argument on the stack may passed it down to a callee without copying (I couldn't find anything in the PCS stating that this isn't allowed). > I'm just asking if we can accept the existing code as now complete > enough (in that I believe it matches the other implementations) and make > this enhancement something for the next release cycle, allowing the existing > code to be exercised by a wider audience and providing ample time to test > the new modification? I'd hate to get stuck in a mode where this patch gets > repeatedly delayed for changes that go above and beyond the original design. 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: -- Catalin 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