From patchwork Tue Jul 26 16:55:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Catalin Marinas X-Patchwork-Id: 72820 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp1792482qga; Tue, 26 Jul 2016 09:55:58 -0700 (PDT) X-Received: by 10.98.0.83 with SMTP id 80mr13723399pfa.78.1469552158602; Tue, 26 Jul 2016 09:55:58 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id e6si1467534pfb.236.2016.07.26.09.55.58; Tue, 26 Jul 2016 09:55:58 -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 S1757336AbcGZQz4 (ORCPT + 29 others); Tue, 26 Jul 2016 12:55:56 -0400 Received: from foss.arm.com ([217.140.101.70]:40744 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752785AbcGZQzw (ORCPT ); Tue, 26 Jul 2016 12:55:52 -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 B7F5C30C; Tue, 26 Jul 2016 09:57:06 -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 29EE23F213; Tue, 26 Jul 2016 09:55:46 -0700 (PDT) Date: Tue, 26 Jul 2016 17:55:43 +0100 From: Catalin Marinas To: Daniel Thompson Cc: David Long , 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: <20160726165543.GG2423@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> <20160725171350.GE2423@e104818-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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 Tue, Jul 26, 2016 at 10:50:08AM +0100, Daniel Thompson wrote: > On 25/07/16 18:13, Catalin Marinas wrote: > >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: > >>>[...] > >>>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). > > The PCS[1] seems (at least to me) to be pretty clear that "the address of > the first stacked argument is defined to be the initial value of SP". > > I think it is only the return value (when stacked via the x8 pointer) that > can be passed through an intermediate function in the way described above. > Isn't it OK for a jprobe to clobber this memory? The underlying function > will overwrite whatever the jprobe put there anyway. > > Am I overlooking some additional detail in the PCS? I'm not sure I fully understand the PCS. I played with some random hacks to test_kprobes.c (see below) and the address passed for a big struct didn't look like the bottom of the stack. -- Catalin diff --git a/kernel/test_kprobes.c b/kernel/test_kprobes.c index 0dbab6d1acb4..6ed7be02a560 100644 --- a/kernel/test_kprobes.c +++ b/kernel/test_kprobes.c @@ -22,14 +22,18 @@ #define div_factor 3 +struct dummy { + char dummy_array[MAX_STACK_SIZE * 2]; +}; + static u32 rand1, preh_val, posth_val, jph_val; static int errors, handler_errors, num_tests; -static u32 (*target)(u32 value); +static u32 (*target)(u32 value, struct dummy d); static u32 (*target2)(u32 value); -static noinline u32 kprobe_target(u32 value) +static noinline u32 kprobe_target(u32 value, struct dummy d) { - return (value / div_factor); + return (value / div_factor - d.dummy_array[0] + d.dummy_array[1]); } static int kp_pre_handler(struct kprobe *p, struct pt_regs *regs) @@ -54,9 +58,11 @@ static struct kprobe kp = { .post_handler = kp_post_handler }; -static int test_kprobe(void) +static int noinline test_kprobe(void) { int ret; + static struct dummy dummy; + memset(&dummy, 10, sizeof(dummy)); ret = register_kprobe(&kp); if (ret < 0) { @@ -64,7 +70,8 @@ static int test_kprobe(void) return ret; } - ret = target(rand1); + ret = target(rand1, dummy); + memset(&dummy, 10, sizeof(dummy)); unregister_kprobe(&kp); if (preh_val == 0) { @@ -111,6 +118,8 @@ static int test_kprobes(void) { int ret; struct kprobe *kps[2] = {&kp, &kp2}; + struct dummy dummy; + memset(&dummy, 10, sizeof(dummy)); /* addr and flags should be cleard for reusing kprobe. */ kp.addr = NULL; @@ -123,7 +132,7 @@ static int test_kprobes(void) preh_val = 0; posth_val = 0; - ret = target(rand1); + ret = target(rand1, dummy); if (preh_val == 0) { pr_err("kprobe pre_handler not called\n"); @@ -154,7 +163,7 @@ static int test_kprobes(void) } -static u32 j_kprobe_target(u32 value) +static u32 j_kprobe_target(u32 value, struct dummy d) { if (value != rand1) { handler_errors++; @@ -174,6 +183,8 @@ static struct jprobe jp = { static int test_jprobe(void) { int ret; + struct dummy dummy; + memset(&dummy, 10, sizeof(dummy)); ret = register_jprobe(&jp); if (ret < 0) { @@ -181,7 +192,7 @@ static int test_jprobe(void) return ret; } - ret = target(rand1); + ret = target(rand1, dummy); unregister_jprobe(&jp); if (jph_val == 0) { pr_err("jprobe handler not called\n"); @@ -200,6 +211,8 @@ static int test_jprobes(void) { int ret; struct jprobe *jps[2] = {&jp, &jp2}; + struct dummy dummy; + memset(&dummy, 10, sizeof(dummy)); /* addr and flags should be cleard for reusing kprobe. */ jp.kp.addr = NULL; @@ -211,7 +224,7 @@ static int test_jprobes(void) } jph_val = 0; - ret = target(rand1); + ret = target(rand1, dummy); if (jph_val == 0) { pr_err("jprobe handler not called\n"); handler_errors++; @@ -262,6 +275,8 @@ static struct kretprobe rp = { static int test_kretprobe(void) { int ret; + struct dummy dummy; + memset(&dummy, 10, sizeof(dummy)); ret = register_kretprobe(&rp); if (ret < 0) { @@ -269,7 +284,7 @@ static int test_kretprobe(void) return ret; } - ret = target(rand1); + ret = target(rand1, dummy); unregister_kretprobe(&rp); if (krph_val != rand1) { pr_err("kretprobe handler not called\n"); @@ -306,6 +321,8 @@ static int test_kretprobes(void) { int ret; struct kretprobe *rps[2] = {&rp, &rp2}; + struct dummy dummy; + memset(&dummy, 10, sizeof(dummy)); /* addr and flags should be cleard for reusing kprobe. */ rp.kp.addr = NULL; @@ -317,7 +334,7 @@ static int test_kretprobes(void) } krph_val = 0; - ret = target(rand1); + ret = target(rand1, dummy); if (krph_val != rand1) { pr_err("kretprobe handler not called\n"); handler_errors++;