Date:	Tue, 26 Jul 2016 17:55:43 +0100
From:	Catalin Marinas <catalin.marinas@....com>
To:	Daniel Thompson <daniel.thompson@...aro.org>
Cc:	David Long <dave.long@...aro.org>,
	Mark Rutland <mark.rutland@....com>,
	Yang Shi <yang.shi@...aro.org>,
	Zi Shen Lim <zlim.lnx@...il.com>,
	Will Deacon <will.deacon@....com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	yalin wang <yalin.wang2010@...il.com>,
	Li Bin <huawei.libin@...wei.com>,
	Jisheng Zhang <jszhang@...vell.com>,
	John Blackwood <john.blackwood@...r.com>,
	Pratyush Anand <panand@...hat.com>,
	Huang Shijie <shijie.huang@....com>,
	Dave P Martin <Dave.Martin@....com>,
	Petr Mladek <pmladek@...e.com>,
	Vladimir Murzin <Vladimir.Murzin@....com>,
	Steve Capper <steve.capper@...aro.org>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Marc Zyngier <marc.zyngier@....com>,
	Mark Brown <broonie@...nel.org>,
	Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>,
	William Cohen <wcohen@...hat.com>,
	Alex Bennée <alex.bennee@...aro.org>,
	Adam Buchbinder <adam.buchbinder@...il.com>,
	linux-arm-kernel@...ts.infradead.org,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	linux-kernel@...r.kernel.org, James Morse <james.morse@....com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Robin Murphy <robin.murphy@....com>,
	Jens Wiklander <jens.wiklander@...aro.org>,
	Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support

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++;

-- 
Catalin

Powered by blists - more mailing lists