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: 9248471 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4D1A7607D8 for ; Tue, 26 Jul 2016 16:58:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3B410205AD for ; Tue, 26 Jul 2016 16:58:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2FF2A26B39; Tue, 26 Jul 2016 16:58:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6A5D4205AD for ; Tue, 26 Jul 2016 16:58:15 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bS5eT-00082s-KT; Tue, 26 Jul 2016 16:56:17 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bS5eP-00081A-PN for linux-arm-kernel@lists.infradead.org; Tue, 26 Jul 2016 16:56:14 +0000 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 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) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160726_095613_843913_38802E12 X-CRM114-Status: GOOD ( 28.52 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Petr Mladek , Zi Shen Lim , Will Deacon , Andrey Ryabinin , yalin wang , Li Bin , Jisheng Zhang , John Blackwood , Pratyush Anand , David Long , Huang Shijie , Dave P Martin , Yang Shi , 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 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP 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. 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++;