[PATCH 0/8] kprobes: Make KUnit and add stacktrace on kretprobe tests

[PATCH 0/8] kprobes: Make KUnit and add stacktrace on kretprobe tests

[Masami Hiramatsu]

Hi,

Here is the series to change the kprobes selftest to KUnit and
add testcases for stacktrace on kretprobes, which has been fixed
recently on x86.

KUnit conversion work has been done by Sven. (Thanks!)
I found that if there are testcases for the stacktrace on kretprobes
we can make sure that the stacktrace can work or not while booting.
So I added 2 additinal Kunit test cases for stacktrace on kretprobe.
One is for a simple stacktrace on kretprobe, the other is for the
stacktrace with kretprobes on nested functions.

But actually, this is currently only for the x86. Thus I also added
some fixes to enable stacktrace on kretprobe on arm and arm64.

Note that for the arm, this series only supports framepointer
based stacktrace. To support ARM unwinder, will need more work.

Thank you,

---

Masami Hiramatsu (7):
      kprobes: Add a test case for stacktrace from kretprobe handler
      arm64: kprobes: Record frame pointer with kretprobe instance
      arm64: kprobes: Make a frame pointer on __kretprobe_trampoline
      arm64: Recover kretprobe modified return address in stacktrace
      ARM: clang: Do not relay on lr register for stacktrace
      ARM: kprobes: Make a frame pointer on __kretprobe_trampoline
      ARM: Recover kretprobe modified return address in stacktrace

Sven Schnelle (1):
      kprobes: convert tests to kunit


 arch/Kconfig                                  |    8 +
 arch/arm/Kconfig                              |    1 
 arch/arm/include/asm/stacktrace.h             |    5 
 arch/arm/kernel/return_address.c              |    2 
 arch/arm/kernel/stacktrace.c                  |   11 +
 arch/arm/probes/kprobes/core.c                |   29 ++
 arch/arm64/Kconfig                            |    1 
 arch/arm64/include/asm/stacktrace.h           |    2 
 arch/arm64/kernel/probes/kprobes.c            |    4 
 arch/arm64/kernel/probes/kprobes_trampoline.S |    4 
 arch/arm64/kernel/stacktrace.c                |    3 
 arch/x86/Kconfig                              |    1 
 kernel/kprobes.c                              |    3 
 kernel/test_kprobes.c                         |  374 ++++++++++++++-----------
 lib/Kconfig.debug                             |    3 
 15 files changed, 278 insertions(+), 173 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/8] kprobes: convert tests to kunit

[Masami Hiramatsu]

From: Sven Schnelle <svens@linux.ibm.com>

This converts the kprobes testcases to use the kunit framework.
It adds a dependency on CONFIG_KUNIT, and the output will change
to TAP:

TAP version 14
1..1
    # Subtest: kprobes_test
    1..4
random: crng init done
    ok 1 - test_kprobe
    ok 2 - test_kprobes
    ok 3 - test_kretprobe
    ok 4 - test_kretprobes
ok 1 - kprobes_test

Note that the kprobes testcases are no longer run immediately after
kprobes initialization, but as a late initcall when kunit is
initialized. kprobes itself is initialized with an early initcall,
so the order is still correct.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c      |    3 -
 kernel/test_kprobes.c |  222 +++++++++++++------------------------------------
 lib/Kconfig.debug     |    3 -
 3 files changed, 61 insertions(+), 167 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b62af9fc3607..4676627cb066 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2581,9 +2581,6 @@ static int __init init_kprobes(void)
 		err = register_module_notifier(&kprobe_module_nb);
 
 	kprobes_initialized = (err == 0);
-
-	if (!err)
-		init_test_probes();
 	return err;
 }
 early_initcall(init_kprobes);
diff --git a/kernel/test_kprobes.c b/kernel/test_kprobes.c
index 76c997fdbc9d..e78f18144145 100644
--- a/kernel/test_kprobes.c
+++ b/kernel/test_kprobes.c
@@ -5,18 +5,17 @@
  * Copyright IBM Corp. 2008
  */
 
-#define pr_fmt(fmt) "Kprobe smoke test: " fmt
-
 #include <linux/kernel.h>
 #include <linux/kprobes.h>
 #include <linux/random.h>
+#include <kunit/test.h>
 
 #define div_factor 3
 
 static u32 rand1, preh_val, posth_val;
-static int errors, handler_errors, num_tests;
 static u32 (*target)(u32 value);
 static u32 (*target2)(u32 value);
+static struct kunit *current_test;
 
 static noinline u32 kprobe_target(u32 value)
 {
@@ -25,10 +24,7 @@ static noinline u32 kprobe_target(u32 value)
 
 static int kp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
-	if (preemptible()) {
-		handler_errors++;
-		pr_err("pre-handler is preemptible\n");
-	}
+	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	preh_val = (rand1 / div_factor);
 	return 0;
 }
@@ -36,14 +32,8 @@ static int kp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 static void kp_post_handler(struct kprobe *p, struct pt_regs *regs,
 		unsigned long flags)
 {
-	if (preemptible()) {
-		handler_errors++;
-		pr_err("post-handler is preemptible\n");
-	}
-	if (preh_val != (rand1 / div_factor)) {
-		handler_errors++;
-		pr_err("incorrect value in post_handler\n");
-	}
+	KUNIT_EXPECT_FALSE(current_test, preemptible());
+	KUNIT_EXPECT_EQ(current_test, preh_val, (rand1 / div_factor));
 	posth_val = preh_val + div_factor;
 }
 
@@ -53,30 +43,14 @@ static struct kprobe kp = {
 	.post_handler = kp_post_handler
 };
 
-static int test_kprobe(void)
+static void test_kprobe(struct kunit *test)
 {
-	int ret;
-
-	ret = register_kprobe(&kp);
-	if (ret < 0) {
-		pr_err("register_kprobe returned %d\n", ret);
-		return ret;
-	}
-
-	ret = target(rand1);
+	current_test = test;
+	KUNIT_EXPECT_EQ(test, 0, register_kprobe(&kp));
+	target(rand1);
 	unregister_kprobe(&kp);
-
-	if (preh_val == 0) {
-		pr_err("kprobe pre_handler not called\n");
-		handler_errors++;
-	}
-
-	if (posth_val == 0) {
-		pr_err("kprobe post_handler not called\n");
-		handler_errors++;
-	}
-
-	return 0;
+	KUNIT_EXPECT_NE(test, 0, preh_val);
+	KUNIT_EXPECT_NE(test, 0, posth_val);
 }
 
 static noinline u32 kprobe_target2(u32 value)
@@ -93,10 +67,7 @@ static int kp_pre_handler2(struct kprobe *p, struct pt_regs *regs)
 static void kp_post_handler2(struct kprobe *p, struct pt_regs *regs,
 		unsigned long flags)
 {
-	if (preh_val != (rand1 / div_factor) + 1) {
-		handler_errors++;
-		pr_err("incorrect value in post_handler2\n");
-	}
+	KUNIT_EXPECT_EQ(current_test, preh_val, (rand1 / div_factor) + 1);
 	posth_val = preh_val + div_factor;
 }
 
@@ -106,51 +77,31 @@ static struct kprobe kp2 = {
 	.post_handler = kp_post_handler2
 };
 
-static int test_kprobes(void)
+static void test_kprobes(struct kunit *test)
 {
-	int ret;
 	struct kprobe *kps[2] = {&kp, &kp2};
 
+	current_test = test;
+
 	/* addr and flags should be cleard for reusing kprobe. */
 	kp.addr = NULL;
 	kp.flags = 0;
-	ret = register_kprobes(kps, 2);
-	if (ret < 0) {
-		pr_err("register_kprobes returned %d\n", ret);
-		return ret;
-	}
 
+	KUNIT_EXPECT_EQ(test, 0, register_kprobes(kps, 2));
 	preh_val = 0;
 	posth_val = 0;
-	ret = target(rand1);
+	target(rand1);
 
-	if (preh_val == 0) {
-		pr_err("kprobe pre_handler not called\n");
-		handler_errors++;
-	}
-
-	if (posth_val == 0) {
-		pr_err("kprobe post_handler not called\n");
-		handler_errors++;
-	}
+	KUNIT_EXPECT_NE(test, 0, preh_val);
+	KUNIT_EXPECT_NE(test, 0, posth_val);
 
 	preh_val = 0;
 	posth_val = 0;
-	ret = target2(rand1);
-
-	if (preh_val == 0) {
-		pr_err("kprobe pre_handler2 not called\n");
-		handler_errors++;
-	}
-
-	if (posth_val == 0) {
-		pr_err("kprobe post_handler2 not called\n");
-		handler_errors++;
-	}
+	target2(rand1);
 
+	KUNIT_EXPECT_NE(test, 0, preh_val);
+	KUNIT_EXPECT_NE(test, 0, posth_val);
 	unregister_kprobes(kps, 2);
-	return 0;
-
 }
 
 #ifdef CONFIG_KRETPROBES
@@ -158,10 +109,7 @@ static u32 krph_val;
 
 static int entry_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
-	if (preemptible()) {
-		handler_errors++;
-		pr_err("kretprobe entry handler is preemptible\n");
-	}
+	KUNIT_EXPECT_FALSE(current_test, preemptible());
 	krph_val = (rand1 / div_factor);
 	return 0;
 }
@@ -170,19 +118,9 @@ static int return_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
 	unsigned long ret = regs_return_value(regs);
 
-	if (preemptible()) {
-		handler_errors++;
-		pr_err("kretprobe return handler is preemptible\n");
-	}
-	if (ret != (rand1 / div_factor)) {
-		handler_errors++;
-		pr_err("incorrect value in kretprobe handler\n");
-	}
-	if (krph_val == 0) {
-		handler_errors++;
-		pr_err("call to kretprobe entry handler failed\n");
-	}
-
+	KUNIT_EXPECT_FALSE(current_test, preemptible());
+	KUNIT_EXPECT_EQ(current_test, ret, rand1 / div_factor);
+	KUNIT_EXPECT_NE(current_test, krph_val, 0);
 	krph_val = rand1;
 	return 0;
 }
@@ -193,39 +131,21 @@ static struct kretprobe rp = {
 	.kp.symbol_name = "kprobe_target"
 };
 
-static int test_kretprobe(void)
+static void test_kretprobe(struct kunit *test)
 {
-	int ret;
-
-	ret = register_kretprobe(&rp);
-	if (ret < 0) {
-		pr_err("register_kretprobe returned %d\n", ret);
-		return ret;
-	}
-
-	ret = target(rand1);
+	current_test = test;
+	KUNIT_EXPECT_EQ(test, 0, register_kretprobe(&rp));
+	target(rand1);
 	unregister_kretprobe(&rp);
-	if (krph_val != rand1) {
-		pr_err("kretprobe handler not called\n");
-		handler_errors++;
-	}
-
-	return 0;
+	KUNIT_EXPECT_EQ(test, krph_val, rand1);
 }
 
 static int return_handler2(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
 	unsigned long ret = regs_return_value(regs);
 
-	if (ret != (rand1 / div_factor) + 1) {
-		handler_errors++;
-		pr_err("incorrect value in kretprobe handler2\n");
-	}
-	if (krph_val == 0) {
-		handler_errors++;
-		pr_err("call to kretprobe entry handler failed\n");
-	}
-
+	KUNIT_EXPECT_EQ(current_test, ret, (rand1 / div_factor) + 1);
+	KUNIT_EXPECT_NE(current_test, krph_val, 0);
 	krph_val = rand1;
 	return 0;
 }
@@ -236,78 +156,54 @@ static struct kretprobe rp2 = {
 	.kp.symbol_name = "kprobe_target2"
 };
 
-static int test_kretprobes(void)
+static void test_kretprobes(struct kunit *test)
 {
-	int ret;
 	struct kretprobe *rps[2] = {&rp, &rp2};
 
+	current_test = test;
 	/* addr and flags should be cleard for reusing kprobe. */
 	rp.kp.addr = NULL;
 	rp.kp.flags = 0;
-	ret = register_kretprobes(rps, 2);
-	if (ret < 0) {
-		pr_err("register_kretprobe returned %d\n", ret);
-		return ret;
-	}
+	KUNIT_EXPECT_EQ(test, 0, register_kretprobes(rps, 2));
 
 	krph_val = 0;
-	ret = target(rand1);
-	if (krph_val != rand1) {
-		pr_err("kretprobe handler not called\n");
-		handler_errors++;
-	}
+	target(rand1);
+	KUNIT_EXPECT_EQ(test, krph_val, rand1);
 
 	krph_val = 0;
-	ret = target2(rand1);
-	if (krph_val != rand1) {
-		pr_err("kretprobe handler2 not called\n");
-		handler_errors++;
-	}
+	target2(rand1);
+	KUNIT_EXPECT_EQ(test, krph_val, rand1);
 	unregister_kretprobes(rps, 2);
-	return 0;
 }
 #endif /* CONFIG_KRETPROBES */
 
-int init_test_probes(void)
+static int kprobes_test_init(struct kunit *test)
 {
-	int ret;
-
 	target = kprobe_target;
 	target2 = kprobe_target2;
 
 	do {
 		rand1 = prandom_u32();
 	} while (rand1 <= div_factor);
+	return 0;
+}
 
-	pr_info("started\n");
-	num_tests++;
-	ret = test_kprobe();
-	if (ret < 0)
-		errors++;
-
-	num_tests++;
-	ret = test_kprobes();
-	if (ret < 0)
-		errors++;
-
+static struct kunit_case kprobes_testcases[] = {
+	KUNIT_CASE(test_kprobe),
+	KUNIT_CASE(test_kprobes),
 #ifdef CONFIG_KRETPROBES
-	num_tests++;
-	ret = test_kretprobe();
-	if (ret < 0)
-		errors++;
-
-	num_tests++;
-	ret = test_kretprobes();
-	if (ret < 0)
-		errors++;
-#endif /* CONFIG_KRETPROBES */
+	KUNIT_CASE(test_kretprobe),
+	KUNIT_CASE(test_kretprobes),
+#endif
+	{}
+};
 
-	if (errors)
-		pr_err("BUG: %d out of %d tests failed\n", errors, num_tests);
-	else if (handler_errors)
-		pr_err("BUG: %d error(s) running handlers\n", handler_errors);
-	else
-		pr_info("passed successfully\n");
+static struct kunit_suite kprobes_test_suite = {
+	.name = "kprobes_test",
+	.init = kprobes_test_init,
+	.test_cases = kprobes_testcases,
+};
 
-	return 0;
-}
+kunit_test_suites(&kprobes_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2a9b6dcdac4f..6ceb11a43e4c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2080,9 +2080,10 @@ config TEST_DIV64
 	  If unsure, say N.
 
 config KPROBES_SANITY_TEST
-	bool "Kprobes sanity tests"
+	tristate "Kprobes sanity tests"
 	depends on DEBUG_KERNEL
 	depends on KPROBES
+	depends on KUNIT
 	help
 	  This option provides for testing basic kprobes functionality on
 	  boot. Samples of kprobe and kretprobe are inserted and

[PATCH 2/8] kprobes: Add a test case for stacktrace from kretprobe handler

[Masami Hiramatsu]

Add a test case for stacktrace from kretprobe handler and
nested kretprobe handlers.

This test checks both of stack trace inside kretprobe handler
and stack trace from pt_regs. Those stack trace must include
actual function return address instead of kretprobe trampoline.
The nested kretprobe stacktrace test checks whether the unwinder
can correctly unwind the call frame on the stack which has been
modified by the kretprobe.

Since the stacktrace on kretprobe is correctly fixed only on x86,
this introduces a meta kconfig ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
which tells user that the stacktrace on kretprobe is correct or not.

The test results will be shown like below;

 TAP version 14
 1..1
     # Subtest: kprobes_test
     1..6
     ok 1 - test_kprobe
     ok 2 - test_kprobes
     ok 3 - test_kretprobe
     ok 4 - test_kretprobes
     ok 5 - test_stacktrace_on_kretprobe
     ok 6 - test_stacktrace_on_nested_kretprobe
 # kprobes_test: pass:6 fail:0 skip:0 total:6
 # Totals: pass:6 fail:0 skip:0 total:6
 ok 1 - kprobes_test

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/Kconfig          |    8 ++
 arch/x86/Kconfig      |    1 
 kernel/test_kprobes.c |  158 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index 8df1c7102643..8378f83b462c 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -191,6 +191,14 @@ config HAVE_OPTPROBES
 config HAVE_KPROBES_ON_FTRACE
 	bool
 
+config ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
+	bool
+	help
+	  Since kretprobes modifies return address on the stack, the
+	  stacktrace may see the kretprobe trampoline address instead
+	  of correct one. If the architecture stacktrace code and
+	  unwinder can adjust such entries, select this configuration.
+
 config HAVE_FUNCTION_ERROR_INJECTION
 	bool
 
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ab83c22d274e..2049364b3981 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -61,6 +61,7 @@ config X86
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT	if ACPI
 	select ARCH_32BIT_OFF_T			if X86_32
 	select ARCH_CLOCKSOURCE_INIT
+	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION
 	select ARCH_ENABLE_MEMORY_HOTPLUG if X86_64 || (X86_32 && HIGHMEM)
 	select ARCH_ENABLE_MEMORY_HOTREMOVE if MEMORY_HOTPLUG
diff --git a/kernel/test_kprobes.c b/kernel/test_kprobes.c
index e78f18144145..a902be1f4a96 100644
--- a/kernel/test_kprobes.c
+++ b/kernel/test_kprobes.c
@@ -17,6 +17,11 @@ static u32 (*target)(u32 value);
 static u32 (*target2)(u32 value);
 static struct kunit *current_test;
 
+static unsigned long (*internal_target)(void);
+static unsigned long (*stacktrace_target)(void);
+static unsigned long (*stacktrace_driver)(void);
+static unsigned long target_return_address[2];
+
 static noinline u32 kprobe_target(u32 value)
 {
 	return (value / div_factor);
@@ -58,6 +63,33 @@ static noinline u32 kprobe_target2(u32 value)
 	return (value / div_factor) + 1;
 }
 
+static noinline unsigned long kprobe_stacktrace_internal_target(void)
+{
+	if (!target_return_address[0])
+		target_return_address[0] = (unsigned long)__builtin_return_address(0);
+	return target_return_address[0];
+}
+
+static noinline unsigned long kprobe_stacktrace_target(void)
+{
+	if (!target_return_address[1])
+		target_return_address[1] = (unsigned long)__builtin_return_address(0);
+
+	if (internal_target)
+		internal_target();
+
+	return target_return_address[1];
+}
+
+static noinline unsigned long kprobe_stacktrace_driver(void)
+{
+	if (stacktrace_target)
+		stacktrace_target();
+
+	/* This is for preventing inlining the function */
+	return (unsigned long)__builtin_return_address(0);
+}
+
 static int kp_pre_handler2(struct kprobe *p, struct pt_regs *regs)
 {
 	preh_val = (rand1 / div_factor) + 1;
@@ -175,12 +207,134 @@ static void test_kretprobes(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, krph_val, rand1);
 	unregister_kretprobes(rps, 2);
 }
+
+#ifdef CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
+#define STACK_BUF_SIZE 16
+static unsigned long stack_buf[STACK_BUF_SIZE];
+
+static int stacktrace_return_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
+{
+	unsigned long retval = regs_return_value(regs);
+	int i, ret;
+
+	KUNIT_EXPECT_FALSE(current_test, preemptible());
+	KUNIT_EXPECT_EQ(current_test, retval, target_return_address[1]);
+
+	/*
+	 * Test stacktrace inside the kretprobe handler, this will involves
+	 * kretprobe trampoline, but must include correct return address
+	 * of the target function.
+	 */
+	ret = stack_trace_save(stack_buf, STACK_BUF_SIZE, 0);
+	KUNIT_EXPECT_NE(current_test, ret, 0);
+
+	for (i = 0; i < ret; i++) {
+		if (stack_buf[i] == target_return_address[1])
+			break;
+	}
+	KUNIT_EXPECT_NE(current_test, i, ret);
+
+	/*
+	 * Test stacktrace from pt_regs at the return address. Thus the stack
+	 * trace must start from the target return address.
+	 */
+	ret = stack_trace_save_regs(regs, stack_buf, STACK_BUF_SIZE, 0);
+	KUNIT_EXPECT_NE(current_test, ret, 0);
+	KUNIT_EXPECT_EQ(current_test, stack_buf[0], target_return_address[1]);
+
+	return 0;
+}
+
+static struct kretprobe rp3 = {
+	.handler	= stacktrace_return_handler,
+	.kp.symbol_name = "kprobe_stacktrace_target"
+};
+
+static void test_stacktrace_on_kretprobe(struct kunit *test)
+{
+	unsigned long myretaddr = (unsigned long)__builtin_return_address(0);
+
+	current_test = test;
+	rp3.kp.addr = NULL;
+	rp3.kp.flags = 0;
+
+	/*
+	 * Run the stacktrace_driver() to record correct return address in
+	 * stacktrace_target() and ensure stacktrace_driver() call is not
+	 * inlined by checking the return address of stacktrace_driver()
+	 * and the return address of this function is different.
+	 */
+	KUNIT_ASSERT_NE(test, myretaddr, stacktrace_driver());
+
+	KUNIT_ASSERT_EQ(test, 0, register_kretprobe(&rp3));
+	KUNIT_ASSERT_NE(test, myretaddr, stacktrace_driver());
+	unregister_kretprobe(&rp3);
+}
+
+static int stacktrace_internal_return_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
+{
+	unsigned long retval = regs_return_value(regs);
+	int i, ret;
+
+	KUNIT_EXPECT_FALSE(current_test, preemptible());
+	KUNIT_EXPECT_EQ(current_test, retval, target_return_address[0]);
+
+	/*
+	 * Test stacktrace inside the kretprobe handler for nested case.
+	 * The unwinder will find the kretprobe_trampoline address on the
+	 * return address, and kretprobe must solve that.
+	 */
+	ret = stack_trace_save(stack_buf, STACK_BUF_SIZE, 0);
+	KUNIT_EXPECT_NE(current_test, ret, 0);
+
+	for (i = 0; i < ret - 1; i++) {
+		if (stack_buf[i] == target_return_address[0]) {
+			KUNIT_EXPECT_EQ(current_test, stack_buf[i + 1], target_return_address[1]);
+			break;
+		}
+	}
+	KUNIT_EXPECT_NE(current_test, i, ret);
+
+	/* Ditto for the regs version. */
+	ret = stack_trace_save_regs(regs, stack_buf, STACK_BUF_SIZE, 0);
+	KUNIT_EXPECT_NE(current_test, ret, 0);
+	KUNIT_EXPECT_EQ(current_test, stack_buf[0], target_return_address[0]);
+	KUNIT_EXPECT_EQ(current_test, stack_buf[1], target_return_address[1]);
+
+	return 0;
+}
+
+static struct kretprobe rp4 = {
+	.handler	= stacktrace_internal_return_handler,
+	.kp.symbol_name = "kprobe_stacktrace_internal_target"
+};
+
+static void test_stacktrace_on_nested_kretprobe(struct kunit *test)
+{
+	unsigned long myretaddr = (unsigned long)__builtin_return_address(0);
+	struct kretprobe *rps[2] = {&rp3, &rp4};
+
+	current_test = test;
+	rp3.kp.addr = NULL;
+	rp3.kp.flags = 0;
+
+	//KUNIT_ASSERT_NE(test, myretaddr, stacktrace_driver());
+
+	KUNIT_ASSERT_EQ(test, 0, register_kretprobes(rps, 2));
+	KUNIT_ASSERT_NE(test, myretaddr, stacktrace_driver());
+	unregister_kretprobes(rps, 2);
+}
+#endif /* CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE */
+
 #endif /* CONFIG_KRETPROBES */
 
 static int kprobes_test_init(struct kunit *test)
 {
 	target = kprobe_target;
 	target2 = kprobe_target2;
+	stacktrace_target = kprobe_stacktrace_target;
+	internal_target = kprobe_stacktrace_internal_target;
+	stacktrace_driver = kprobe_stacktrace_driver;
 
 	do {
 		rand1 = prandom_u32();
@@ -194,6 +348,10 @@ static struct kunit_case kprobes_testcases[] = {
 #ifdef CONFIG_KRETPROBES
 	KUNIT_CASE(test_kretprobe),
 	KUNIT_CASE(test_kretprobes),
+#ifdef CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
+	KUNIT_CASE(test_stacktrace_on_kretprobe),
+	KUNIT_CASE(test_stacktrace_on_nested_kretprobe),
+#endif
 #endif
 	{}
 };

[PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Masami Hiramatsu]

Record the frame pointer instead of stack address with kretprobe
instance as the identifier on the instance list.
Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
actual frame pointer (x29).

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index e7ad6da980e8..d9dfa82c1f18 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -401,14 +401,14 @@ int __init arch_populate_kprobe_blacklist(void)
 
 void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
 {
-	return (void *)kretprobe_trampoline_handler(regs, (void *)kernel_stack_pointer(regs));
+	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
 }
 
 void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
 	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
-	ri->fp = (void *)kernel_stack_pointer(regs);
+	ri->fp = (void *)regs->regs[29];
 
 	/* replace return addr (x30) with trampoline */
 	regs->regs[30] = (long)&__kretprobe_trampoline;

[PATCH 4/8] arm64: kprobes: Make a frame pointer on __kretprobe_trampoline

[Masami Hiramatsu]

Make a frame pointer (make the x29 register points the
address of pt_regs->regs[29]) on __kretprobe_trampoline.

This frame pointer will be used by the stacktracer when it is
called from the kretprobe handlers. In this case, the stack
tracer will unwind stack to trampoline_probe_handler() and
find the next frame pointer in the stack frame of the
__kretprobe_trampoline().

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes_trampoline.S |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/probes/kprobes_trampoline.S b/arch/arm64/kernel/probes/kprobes_trampoline.S
index 520ee8711db1..9a6499bed58b 100644
--- a/arch/arm64/kernel/probes/kprobes_trampoline.S
+++ b/arch/arm64/kernel/probes/kprobes_trampoline.S
@@ -66,6 +66,9 @@ SYM_CODE_START(__kretprobe_trampoline)
 
 	save_all_base_regs
 
+	/* Setup a frame pointer. */
+	add x29, sp, #S_FP
+
 	mov x0, sp
 	bl trampoline_probe_handler
 	/*
@@ -74,6 +77,7 @@ SYM_CODE_START(__kretprobe_trampoline)
 	 */
 	mov lr, x0
 
+	/* The frame pointer (x29) is restored with other registers. */
 	restore_all_base_regs
 
 	add sp, sp, #PT_REGS_SIZE

[PATCH 5/8] arm64: Recover kretprobe modified return address in stacktrace

[Masami Hiramatsu]

Since the kretprobe replaces the function return address with
the kretprobe_trampoline on the stack, stack unwinder shows it
instead of the correct return address.

This checks whether the next return address is the
__kretprobe_trampoline(), and if so, try to find the correct
return address from the kretprobe instance list.

With this fix, now arm64 can enable
CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE, and pass the
kprobe self tests.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/Kconfig                  |    1 +
 arch/arm64/include/asm/stacktrace.h |    2 ++
 arch/arm64/kernel/stacktrace.c      |    3 +++
 3 files changed, 6 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5c7ae4c3954b..edde5171ffb2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -11,6 +11,7 @@ config ARM64
 	select ACPI_PPTT if ACPI
 	select ARCH_HAS_DEBUG_WX
 	select ARCH_BINFMT_ELF_STATE
+	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
 	select ARCH_ENABLE_MEMORY_HOTPLUG
 	select ARCH_ENABLE_MEMORY_HOTREMOVE
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 8aebc00c1718..8f997a602651 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -9,6 +9,7 @@
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
 #include <linux/types.h>
+#include <linux/llist.h>
 
 #include <asm/memory.h>
 #include <asm/ptrace.h>
@@ -59,6 +60,7 @@ struct stackframe {
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	int graph;
 #endif
+	struct llist_node *kr_cur;
 };
 
 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 8982a2b78acf..f1eef5745542 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -129,6 +129,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 		frame->pc = ret_stack->ret;
 	}
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+	if (is_kretprobe_trampoline(frame->pc))
+		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
 
 	frame->pc = ptrauth_strip_insn_pac(frame->pc);
 
@@ -224,6 +226,7 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 {
 	struct stackframe frame;
 
+	memset(&frame, 0, sizeof(frame));
 	if (regs)
 		start_backtrace(&frame, regs->regs[29], regs->pc);
 	else if (task == current)

[PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Masami Hiramatsu]

Currently the stacktrace on clang compiled arm kernel uses the 'lr'
register to find the first frame address from pt_regs. However, that
is wrong after calling another function, because the 'lr' register
is used by 'bl' instruction and never be recovered.

As same as gcc arm kernel, directly use the frame pointer (x11) of
the pt_regs to find the first frame address.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/kernel/stacktrace.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 76ea4178a55c..db798eac7431 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -54,8 +54,7 @@ int notrace unwind_frame(struct stackframe *frame)
 
 	frame->sp = frame->fp;
 	frame->fp = *(unsigned long *)(fp);
-	frame->pc = frame->lr;
-	frame->lr = *(unsigned long *)(fp + 4);
+	frame->pc = *(unsigned long *)(fp + 4);
 #else
 	/* check current frame pointer is within bounds */
 	if (fp < low + 12 || fp > high - 4)

[PATCH 7/8] ARM: kprobes: Make a frame pointer on __kretprobe_trampoline

[Masami Hiramatsu]

Currently kretprobe on ARM just fills r0-r11 of pt_regs, but
that is not enough for the stacktrace. Moreover, from the user
kretprobe handler, stacktrace needs a frame pointer on the
__kretprobe_trampoline.

This adds a frame pointer on __kretprobe_trampoline for both gcc
and clang case. Those have different frame pointer so we need
different but similar stack on pt_regs.

Gcc makes the frame pointer (fp) to point the 'pc' address of
the {fp, ip (=sp), lr, pc}, this means {r11, r13, r14, r15}.
Thus if we save the r11 (fp) on pt_regs->r12, we can make this
set on the end of pt_regs.

On the other hand, Clang makes the frame pointer to point the
'fp' address of {fp, lr} on stack. Since the next to the
pt_regs->lr is pt_regs->sp, I reused the pair of pt_regs->fp
and pt_regs->ip.
So this stores the 'lr' on pt_regs->ip and make the fp to point
pt_regs->fp.

For both cases, saves __kretprobe_trampoline address to
pt_regs->lr, so that the stack tracer can identify this frame
pointer has been made by the __kretprobe_trampoline.

Note that if the CONFIG_FRAME_POINTER is not set, this keeps
fp as is.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/probes/kprobes/core.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 95f23b47ba27..7cbd65a22769 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -368,16 +368,35 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
 /*
  * When a retprobed function returns, trampoline_handler() is called,
  * calling the kretprobe's handler. We construct a struct pt_regs to
- * give a view of registers r0-r11 to the user return-handler.  This is
- * not a complete pt_regs structure, but that should be plenty sufficient
- * for kretprobe handlers which should normally be interested in r0 only
- * anyway.
+ * give a view of registers r0-r11, sp, lr, and pc to the user
+ * return-handler. This is not a complete pt_regs structure, but that
+ * should be enough for stacktrace from the return handler with or
+ * without pt_regs.
  */
 void __naked __kprobes __kretprobe_trampoline(void)
 {
 	__asm__ __volatile__ (
-		"sub	sp, sp, #16		\n\t"
+		"ldr	lr, =__kretprobe_trampoline	\n\t"
+		"stmdb	sp!, {sp, lr, pc}	\n\t"
+#ifdef CONFIG_FRAME_POINTER
+	/* __kretprobe_trampoline makes a framepointer on pt_regs. */
+#ifdef CONFIG_CC_IS_CLANG
+		/* In clang case, pt_regs->ip = lr. */
+		"stmdb	sp!, {lr}		\n\t"
 		"stmdb	sp!, {r0 - r11}		\n\t"
+		/* fp points regs->r11 (fp) */
+		"add	fp, sp,	#44		\n\t"
+#else /* !CONFIG_CC_IS_CLANG */
+		/* In gcc case, pt_regs->ip = fp. */
+		"stmdb	sp!, {fp}		\n\t"
+		"stmdb	sp!, {r0 - r11}		\n\t"
+		/* fp points regs->r15 (pc) */
+		"add	fp, sp, #60		\n\t"
+#endif /* CONFIG_CC_IS_CLANG */
+#else /* !CONFIG_FRAME_POINTER */
+		"sub	sp, sp, #4		\n\t"
+		"stmdb	sp!, {r0 - r11}		\n\t"
+#endif /* CONFIG_FRAME_POINTER */
 		"mov	r0, sp			\n\t"
 		"bl	trampoline_handler	\n\t"
 		"mov	lr, r0			\n\t"

[PATCH 8/8] ARM: Recover kretprobe modified return address in stacktrace

[Masami Hiramatsu]

Since the kretprobe replaces the function return address with
the kretprobe_trampoline on the stack, arm unwinder shows it
instead of the correct return address.

This finds the correct return address from the per-task
kretprobe_instances list and verify it is in between the
caller fp and callee fp.

Note that this supports both GCC and clang if CONFIG_FRAME_POINTER=y
and CONFIG_ARM_UNWIND=n. For the ARM unwinder, this is still
not working correctly.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/Kconfig                  |    1 +
 arch/arm/include/asm/stacktrace.h |    5 +++++
 arch/arm/kernel/return_address.c  |    2 ++
 arch/arm/kernel/stacktrace.c      |    8 ++++++++
 4 files changed, 16 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fc196421b2ce..bb4f1872967c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -3,6 +3,7 @@ config ARM
 	bool
 	default y
 	select ARCH_32BIT_OFF_T
+	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE if HAVE_KRETPROBES && FRAME_POINTER && !ARM_UNWIND
 	select ARCH_HAS_BINFMT_FLAT
 	select ARCH_HAS_DEBUG_VIRTUAL if MMU
 	select ARCH_HAS_DMA_WRITE_COMBINE if !ARM_DMA_MEM_BUFFERABLE
diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 2d76a2e29f05..3c23abe935f2 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -3,6 +3,7 @@
 #define __ASM_STACKTRACE_H
 
 #include <asm/ptrace.h>
+#include <linux/llist.h>
 
 struct stackframe {
 	/*
@@ -13,6 +14,8 @@ struct stackframe {
 	unsigned long sp;
 	unsigned long lr;
 	unsigned long pc;
+	struct llist_node *kr_cur;
+	struct task_struct *tsk;
 };
 
 static __always_inline
@@ -22,6 +25,8 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 		frame->sp = regs->ARM_sp;
 		frame->lr = regs->ARM_lr;
 		frame->pc = regs->ARM_pc;
+		frame->kr_cur = NULL;
+		frame->tsk = current;
 }
 
 extern int unwind_frame(struct stackframe *frame);
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 7b42ac010fdf..6fea64d74ebc 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -42,6 +42,8 @@ void *return_address(unsigned int level)
 	frame.sp = current_stack_pointer;
 	frame.lr = (unsigned long)__builtin_return_address(0);
 	frame.pc = (unsigned long)return_address;
+	frame.tsk = current;
+	frame.kr_cur = NULL;
 
 	walk_stackframe(&frame, save_return_addr, &data);
 
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index db798eac7431..7022dda02b93 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 #include <linux/export.h>
+#include <linux/kprobes.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
 #include <linux/stacktrace.h>
@@ -65,6 +66,9 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->sp = *(unsigned long *)(fp - 8);
 	frame->pc = *(unsigned long *)(fp - 4);
 #endif
+	if (is_kretprobe_trampoline(frame->pc))
+		frame->pc = kretprobe_find_ret_addr(frame->tsk,
+					(void *)frame->fp, &frame->kr_cur);
 
 	return 0;
 }
@@ -156,6 +160,8 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
 		frame.lr = (unsigned long)__builtin_return_address(0);
 		frame.pc = (unsigned long)__save_stack_trace;
 	}
+	frame.kr_cur = NULL;
+	frame.tsk = tsk;
 
 	walk_stackframe(&frame, save_trace, &data);
 }
@@ -173,6 +179,8 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 	frame.sp = regs->ARM_sp;
 	frame.lr = regs->ARM_lr;
 	frame.pc = regs->ARM_pc;
+	frame.kr_cur = NULL;
+	frame.tsk = current;
 
 	walk_stackframe(&frame, save_trace, &data);
 }

Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Nick Desaulniers]

On Fri, Oct 8, 2021 at 5:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Currently the stacktrace on clang compiled arm kernel uses the 'lr'
> register to find the first frame address from pt_regs. However, that
> is wrong after calling another function, because the 'lr' register
> is used by 'bl' instruction and never be recovered.
>
> As same as gcc arm kernel, directly use the frame pointer (x11) of
> the pt_regs to find the first frame address.

Hi Masami,
Thanks for the patch. Testing with ARCH=arm defconfig (multi_v7_defconfig)

Before this patch:

$ mount -t proc /proc
$ echo 0 > /proc/sys/kernel/kptr_restrict
$ cat /proc/self/stack
[<0>] proc_single_show+0x4c/0xb8
[<0>] seq_read_iter+0x174/0x4d8
[<0>] seq_read+0x134/0x158
[<0>] vfs_read+0xcc/0x2f8
[<0>] ksys_read+0x74/0xd0
[<0>] __entry_text_start+0x14/0x14
[<0>] 0xbea38cc0

After this patch:
$ mount -t proc /proc
$ echo 0 > /proc/sys/kernel/kptr_restrict
$ cat /proc/self/stack
[<0>] proc_single_show+0x4c/0xb8
[<0>] seq_read_iter+0x174/0x4d8
[<0>] seq_read+0x134/0x158
[<0>] vfs_read+0xcc/0x2f8
[<0>] ksys_read+0x74/0xd0
[<0>] __entry_text_start+0x14/0x14
[<0>] 0xbeb55cc0

Is there a different way to test/verify this patch? (I'm pretty sure
we had verified the WARN_ONCE functionality with this, too.)

If I change from CONFIG_UNWINDER_ARM=y to
CONFIG_UNWINDER_FRAME_POINTER=y, before:

# cat /proc/self/stack
[<0>] stack_trace_save_tsk+0x50/0x6c
[<0>] proc_pid_stack+0xa0/0xf8
[<0>] proc_single_show+0x50/0xbc
[<0>] seq_read_iter+0x178/0x4ec
[<0>] seq_read+0x138/0x15c
[<0>] vfs_read+0xd0/0x304
[<0>] ksys_read+0x78/0xd4
[<0>] sys_read+0xc/0x10

after:
# cat /proc/self/stack
[<0>] proc_pid_stack+0xa0/0xf8
[<0>] proc_single_show+0x50/0xbc
[<0>] seq_read_iter+0x178/0x4ec
[<0>] seq_read+0x138/0x15c
[<0>] vfs_read+0xd0/0x304
[<0>] ksys_read+0x78/0xd4
[<0>] sys_read+0xc/0x10
[<0>] __entry_text_start+0x14/0x14
[<0>] 0xffffffff

So I guess this helps the CONFIG_UNWINDER_FRAME_POINTER=y case? (That
final frame address looks wrong, but is potentially yet another bug;
perhaps for clang we need to manually store the previous frame's pc at
a different offset before jumping to __entry_text_start).

Also, I'm curious about CONFIG_THUMB2_KERNEL (forces CONFIG_UNWINDER_ARM=y).

before:
# cat /proc/self/stack
[<0>] proc_single_show+0x31/0x86
[<0>] seq_read_iter+0xff/0x326
[<0>] seq_read+0xd7/0xf2
[<0>] vfs_read+0x93/0x20e
[<0>] ksys_read+0x53/0x92
[<0>] ret_fast_syscall+0x1/0x52
[<0>] 0xbe9a9cc0

after:
# cat /proc/self/stack
[<0>] proc_single_show+0x31/0x86
[<0>] seq_read_iter+0xff/0x326
[<0>] seq_read+0xd7/0xf2
[<0>] vfs_read+0x93/0x20e
[<0>] ksys_read+0x53/0x92
[<0>] ret_fast_syscall+0x1/0x52
[<0>] 0xbec08cc0

Tested-by: Nick Desaulniers <ndesaulniers@google.com>

so likely this fixes/improves CONFIG_UNWINDER_FRAME_POINTER=y? Is that correct?

>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm/kernel/stacktrace.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 76ea4178a55c..db798eac7431 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -54,8 +54,7 @@ int notrace unwind_frame(struct stackframe *frame)
>
>         frame->sp = frame->fp;
>         frame->fp = *(unsigned long *)(fp);
> -       frame->pc = frame->lr;
> -       frame->lr = *(unsigned long *)(fp + 4);
> +       frame->pc = *(unsigned long *)(fp + 4);
>  #else
>         /* check current frame pointer is within bounds */
>         if (fp < low + 12 || fp > high - 4)
>

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 7/8] ARM: kprobes: Make a frame pointer on __kretprobe_trampoline

[Nick Desaulniers]

On Fri, Oct 8, 2021 at 5:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Currently kretprobe on ARM just fills r0-r11 of pt_regs, but
> that is not enough for the stacktrace. Moreover, from the user
> kretprobe handler, stacktrace needs a frame pointer on the
> __kretprobe_trampoline.
>
> This adds a frame pointer on __kretprobe_trampoline for both gcc
> and clang case. Those have different frame pointer so we need
> different but similar stack on pt_regs.
>
> Gcc makes the frame pointer (fp) to point the 'pc' address of
> the {fp, ip (=sp), lr, pc}, this means {r11, r13, r14, r15}.
> Thus if we save the r11 (fp) on pt_regs->r12, we can make this
> set on the end of pt_regs.
>
> On the other hand, Clang makes the frame pointer to point the
> 'fp' address of {fp, lr} on stack. Since the next to the
> pt_regs->lr is pt_regs->sp, I reused the pair of pt_regs->fp
> and pt_regs->ip.
> So this stores the 'lr' on pt_regs->ip and make the fp to point
> pt_regs->fp.
>
> For both cases, saves __kretprobe_trampoline address to
> pt_regs->lr, so that the stack tracer can identify this frame
> pointer has been made by the __kretprobe_trampoline.
>
> Note that if the CONFIG_FRAME_POINTER is not set, this keeps
> fp as is.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm/probes/kprobes/core.c |   29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index 95f23b47ba27..7cbd65a22769 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -368,16 +368,35 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
>  /*
>   * When a retprobed function returns, trampoline_handler() is called,
>   * calling the kretprobe's handler. We construct a struct pt_regs to
> - * give a view of registers r0-r11 to the user return-handler.  This is
> - * not a complete pt_regs structure, but that should be plenty sufficient
> - * for kretprobe handlers which should normally be interested in r0 only
> - * anyway.
> + * give a view of registers r0-r11, sp, lr, and pc to the user
> + * return-handler. This is not a complete pt_regs structure, but that
> + * should be enough for stacktrace from the return handler with or
> + * without pt_regs.
>   */
>  void __naked __kprobes __kretprobe_trampoline(void)
>  {
>         __asm__ __volatile__ (
> -               "sub    sp, sp, #16             \n\t"
> +               "ldr    lr, =__kretprobe_trampoline     \n\t"
> +               "stmdb  sp!, {sp, lr, pc}       \n\t"
> +#ifdef CONFIG_FRAME_POINTER
> +       /* __kretprobe_trampoline makes a framepointer on pt_regs. */
> +#ifdef CONFIG_CC_IS_CLANG
> +               /* In clang case, pt_regs->ip = lr. */
> +               "stmdb  sp!, {lr}               \n\t"
>                 "stmdb  sp!, {r0 - r11}         \n\t"
> +               /* fp points regs->r11 (fp) */
> +               "add    fp, sp, #44             \n\t"
> +#else /* !CONFIG_CC_IS_CLANG */
> +               /* In gcc case, pt_regs->ip = fp. */
> +               "stmdb  sp!, {fp}               \n\t"
> +               "stmdb  sp!, {r0 - r11}         \n\t"
> +               /* fp points regs->r15 (pc) */
> +               "add    fp, sp, #60             \n\t"

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> +#endif /* CONFIG_CC_IS_CLANG */
> +#else /* !CONFIG_FRAME_POINTER */
> +               "sub    sp, sp, #4              \n\t"
> +               "stmdb  sp!, {r0 - r11}         \n\t"
> +#endif /* CONFIG_FRAME_POINTER */
>                 "mov    r0, sp                  \n\t"
>                 "bl     trampoline_handler      \n\t"
>                 "mov    lr, r0                  \n\t"
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Masami Hiramatsu]

Hi Nick,

On Mon, 11 Oct 2021 11:45:22 -0700
Nick Desaulniers <ndesaulniers@google.com> wrote:

> On Fri, Oct 8, 2021 at 5:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Currently the stacktrace on clang compiled arm kernel uses the 'lr'
> > register to find the first frame address from pt_regs. However, that
> > is wrong after calling another function, because the 'lr' register
> > is used by 'bl' instruction and never be recovered.
> >
> > As same as gcc arm kernel, directly use the frame pointer (x11) of
> > the pt_regs to find the first frame address.
> 
> Hi Masami,
> Thanks for the patch. Testing with ARCH=arm defconfig (multi_v7_defconfig)
> 
> Before this patch:
> 
> $ mount -t proc /proc
> $ echo 0 > /proc/sys/kernel/kptr_restrict
> $ cat /proc/self/stack
> [<0>] proc_single_show+0x4c/0xb8
> [<0>] seq_read_iter+0x174/0x4d8
> [<0>] seq_read+0x134/0x158
> [<0>] vfs_read+0xcc/0x2f8
> [<0>] ksys_read+0x74/0xd0
> [<0>] __entry_text_start+0x14/0x14
> [<0>] 0xbea38cc0
> 
> After this patch:
> $ mount -t proc /proc
> $ echo 0 > /proc/sys/kernel/kptr_restrict
> $ cat /proc/self/stack
> [<0>] proc_single_show+0x4c/0xb8
> [<0>] seq_read_iter+0x174/0x4d8
> [<0>] seq_read+0x134/0x158
> [<0>] vfs_read+0xcc/0x2f8
> [<0>] ksys_read+0x74/0xd0
> [<0>] __entry_text_start+0x14/0x14
> [<0>] 0xbeb55cc0
> 
> Is there a different way to test/verify this patch? (I'm pretty sure
> we had verified the WARN_ONCE functionality with this, too.)

Hmm, I found this bug while testing my kretprobe-stacktrace test
([2/8] in this series), so if you apply this series and revert
this patch and enable CONFIG_KPROBES_SANITY_TEST, you'll see that
the tests failures as below.

[    4.062056]     ok 4 - test_kretprobes
[    4.069944]     # test_stacktrace_on_kretprobe: EXPECTATION FAILED at kernel/test_kprobes.c:235
[    4.069944]     Expected i != ret, but
[    4.069944]         i == 10
[    4.069944]         ret == 10
[    4.072692]     not ok 5 - test_stacktrace_on_kretprobe
[    4.088171]     # test_stacktrace_on_nested_kretprobe: EXPECTATION FAILED at kernel/test_kprobes.c:235
[    4.088171]     Expected i != ret, but
[    4.088171]         i == 10
[    4.088171]         ret == 10
[    4.091265]     not ok 6 - test_stacktrace_on_nested_kretprobe

This means the test failed to find the correct return address from
the stacktrace.
Applying this patch,

[    4.283953]     ok 4 - test_kretprobes
[    4.290206]     ok 5 - test_stacktrace_on_kretprobe
[    4.300429]     ok 6 - test_stacktrace_on_nested_kretprobe
[    4.301743] # kprobes_test: pass:6 fail:0 skip:0 total:6


> 
> If I change from CONFIG_UNWINDER_ARM=y to
> CONFIG_UNWINDER_FRAME_POINTER=y, before:
> 
> # cat /proc/self/stack
> [<0>] stack_trace_save_tsk+0x50/0x6c
> [<0>] proc_pid_stack+0xa0/0xf8
> [<0>] proc_single_show+0x50/0xbc
> [<0>] seq_read_iter+0x178/0x4ec
> [<0>] seq_read+0x138/0x15c
> [<0>] vfs_read+0xd0/0x304
> [<0>] ksys_read+0x78/0xd4
> [<0>] sys_read+0xc/0x10
> 
> after:
> # cat /proc/self/stack
> [<0>] proc_pid_stack+0xa0/0xf8
> [<0>] proc_single_show+0x50/0xbc
> [<0>] seq_read_iter+0x178/0x4ec
> [<0>] seq_read+0x138/0x15c
> [<0>] vfs_read+0xd0/0x304
> [<0>] ksys_read+0x78/0xd4
> [<0>] sys_read+0xc/0x10
> [<0>] __entry_text_start+0x14/0x14
> [<0>] 0xffffffff
> 
> So I guess this helps the CONFIG_UNWINDER_FRAME_POINTER=y case? (That
> final frame address looks wrong, but is potentially yet another bug;
> perhaps for clang we need to manually store the previous frame's pc at
> a different offset before jumping to __entry_text_start).

Ah, yes. I didn't touch the UNWINDER_ARM. As you may know the
unwind_frame()@arch/arm/kernel/stacktrace.c is compiled only
CONFIG_UNWINDER_FRAME_POINTER=y.

> 
> Also, I'm curious about CONFIG_THUMB2_KERNEL (forces CONFIG_UNWINDER_ARM=y).
> 
> before:
> # cat /proc/self/stack
> [<0>] proc_single_show+0x31/0x86
> [<0>] seq_read_iter+0xff/0x326
> [<0>] seq_read+0xd7/0xf2
> [<0>] vfs_read+0x93/0x20e
> [<0>] ksys_read+0x53/0x92
> [<0>] ret_fast_syscall+0x1/0x52
> [<0>] 0xbe9a9cc0
> 
> after:
> # cat /proc/self/stack
> [<0>] proc_single_show+0x31/0x86
> [<0>] seq_read_iter+0xff/0x326
> [<0>] seq_read+0xd7/0xf2
> [<0>] vfs_read+0x93/0x20e
> [<0>] ksys_read+0x53/0x92
> [<0>] ret_fast_syscall+0x1/0x52
> [<0>] 0xbec08cc0
> 
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> so likely this fixes/improves CONFIG_UNWINDER_FRAME_POINTER=y? Is that correct?

Yes, that is correct.

Thank you!

> 
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/arm/kernel/stacktrace.c |    3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> > index 76ea4178a55c..db798eac7431 100644
> > --- a/arch/arm/kernel/stacktrace.c
> > +++ b/arch/arm/kernel/stacktrace.c
> > @@ -54,8 +54,7 @@ int notrace unwind_frame(struct stackframe *frame)
> >
> >         frame->sp = frame->fp;
> >         frame->fp = *(unsigned long *)(fp);
> > -       frame->pc = frame->lr;
> > -       frame->lr = *(unsigned long *)(fp + 4);
> > +       frame->pc = *(unsigned long *)(fp + 4);
> >  #else
> >         /* check current frame pointer is within bounds */
> >         if (fp < low + 12 || fp > high - 4)
> >
> 
> -- 
> Thanks,
> ~Nick Desaulniers


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 5/8] arm64: Recover kretprobe modified return address in stacktrace

[Will Deacon]

On Fri, Oct 08, 2021 at 09:28:58PM +0900, Masami Hiramatsu wrote:
> Since the kretprobe replaces the function return address with
> the kretprobe_trampoline on the stack, stack unwinder shows it
> instead of the correct return address.
> 
> This checks whether the next return address is the
> __kretprobe_trampoline(), and if so, try to find the correct
> return address from the kretprobe instance list.
> 
> With this fix, now arm64 can enable
> CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE, and pass the
> kprobe self tests.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/Kconfig                  |    1 +
>  arch/arm64/include/asm/stacktrace.h |    2 ++
>  arch/arm64/kernel/stacktrace.c      |    3 +++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5c7ae4c3954b..edde5171ffb2 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -11,6 +11,7 @@ config ARM64
>  	select ACPI_PPTT if ACPI
>  	select ARCH_HAS_DEBUG_WX
>  	select ARCH_BINFMT_ELF_STATE
> +	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
>  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
>  	select ARCH_ENABLE_MEMORY_HOTPLUG
>  	select ARCH_ENABLE_MEMORY_HOTREMOVE
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 8aebc00c1718..8f997a602651 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -9,6 +9,7 @@
>  #include <linux/sched.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/types.h>
> +#include <linux/llist.h>
>  
>  #include <asm/memory.h>
>  #include <asm/ptrace.h>
> @@ -59,6 +60,7 @@ struct stackframe {
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	int graph;
>  #endif
> +	struct llist_node *kr_cur;
>  };

Please update the comment above this structure to describe the new member
you're adding. If it's only relevant for kprobes, then let's define it
conditionally too (based on CONFIG_KRETPROBES ?)

>  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 8982a2b78acf..f1eef5745542 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -129,6 +129,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  		frame->pc = ret_stack->ret;
>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +	if (is_kretprobe_trampoline(frame->pc))
> +		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
>  
>  	frame->pc = ptrauth_strip_insn_pac(frame->pc);
>  
> @@ -224,6 +226,7 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  {
>  	struct stackframe frame;
>  
> +	memset(&frame, 0, sizeof(frame));

Why do we need this?

Will

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Will Deacon]

On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> Record the frame pointer instead of stack address with kretprobe
> instance as the identifier on the instance list.
> Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> actual frame pointer (x29).
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/kernel/probes/kprobes.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index e7ad6da980e8..d9dfa82c1f18 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -401,14 +401,14 @@ int __init arch_populate_kprobe_blacklist(void)
>  
>  void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
>  {
> -	return (void *)kretprobe_trampoline_handler(regs, (void *)kernel_stack_pointer(regs));
> +	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
>  }
>  
>  void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  				      struct pt_regs *regs)
>  {
>  	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
> -	ri->fp = (void *)kernel_stack_pointer(regs);
> +	ri->fp = (void *)regs->regs[29];
>  
>  	/* replace return addr (x30) with trampoline */
>  	regs->regs[30] = (long)&__kretprobe_trampoline;

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH 4/8] arm64: kprobes: Make a frame pointer on __kretprobe_trampoline

[Will Deacon]

On Fri, Oct 08, 2021 at 09:28:49PM +0900, Masami Hiramatsu wrote:
> Make a frame pointer (make the x29 register points the
> address of pt_regs->regs[29]) on __kretprobe_trampoline.
> 
> This frame pointer will be used by the stacktracer when it is
> called from the kretprobe handlers. In this case, the stack
> tracer will unwind stack to trampoline_probe_handler() and
> find the next frame pointer in the stack frame of the
> __kretprobe_trampoline().
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/kernel/probes/kprobes_trampoline.S |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes_trampoline.S b/arch/arm64/kernel/probes/kprobes_trampoline.S
> index 520ee8711db1..9a6499bed58b 100644
> --- a/arch/arm64/kernel/probes/kprobes_trampoline.S
> +++ b/arch/arm64/kernel/probes/kprobes_trampoline.S
> @@ -66,6 +66,9 @@ SYM_CODE_START(__kretprobe_trampoline)
>  
>  	save_all_base_regs
>  
> +	/* Setup a frame pointer. */
> +	add x29, sp, #S_FP
> +
>  	mov x0, sp
>  	bl trampoline_probe_handler
>  	/*
> @@ -74,6 +77,7 @@ SYM_CODE_START(__kretprobe_trampoline)
>  	 */
>  	mov lr, x0
>  
> +	/* The frame pointer (x29) is restored with other registers. */
>  	restore_all_base_regs
>  
>  	add sp, sp, #PT_REGS_SIZE

Acked-by: Will Deacon <will@kernel.org>

Will

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Mark Rutland]

On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> Record the frame pointer instead of stack address with kretprobe
> instance as the identifier on the instance list.
> Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> actual frame pointer (x29).

Just to check, why do we need to use the FP rather than SP? It wasn't
clear to me if that's necessary later in the series, or if I'm missing
something here.

FWIW, I plan to rework arm64's ftrace bits to use FP for
HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so I'm happy to do likewise here.

> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Regardless of the above:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/probes/kprobes.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index e7ad6da980e8..d9dfa82c1f18 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -401,14 +401,14 @@ int __init arch_populate_kprobe_blacklist(void)
>  
>  void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
>  {
> -	return (void *)kretprobe_trampoline_handler(regs, (void *)kernel_stack_pointer(regs));
> +	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
>  }
>  
>  void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  				      struct pt_regs *regs)
>  {
>  	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
> -	ri->fp = (void *)kernel_stack_pointer(regs);
> +	ri->fp = (void *)regs->regs[29];
>  
>  	/* replace return addr (x30) with trampoline */
>  	regs->regs[30] = (long)&__kretprobe_trampoline;
> 

Re: [PATCH 5/8] arm64: Recover kretprobe modified return address in stacktrace

[Mark Rutland]

On Fri, Oct 08, 2021 at 09:28:58PM +0900, Masami Hiramatsu wrote:
> Since the kretprobe replaces the function return address with
> the kretprobe_trampoline on the stack, stack unwinder shows it
> instead of the correct return address.
> 
> This checks whether the next return address is the
> __kretprobe_trampoline(), and if so, try to find the correct
> return address from the kretprobe instance list.
> 
> With this fix, now arm64 can enable
> CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE, and pass the
> kprobe self tests.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/Kconfig                  |    1 +
>  arch/arm64/include/asm/stacktrace.h |    2 ++
>  arch/arm64/kernel/stacktrace.c      |    3 +++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5c7ae4c3954b..edde5171ffb2 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -11,6 +11,7 @@ config ARM64
>  	select ACPI_PPTT if ACPI
>  	select ARCH_HAS_DEBUG_WX
>  	select ARCH_BINFMT_ELF_STATE
> +	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
>  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
>  	select ARCH_ENABLE_MEMORY_HOTPLUG
>  	select ARCH_ENABLE_MEMORY_HOTREMOVE
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 8aebc00c1718..8f997a602651 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -9,6 +9,7 @@
>  #include <linux/sched.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/types.h>
> +#include <linux/llist.h>
>  
>  #include <asm/memory.h>
>  #include <asm/ptrace.h>
> @@ -59,6 +60,7 @@ struct stackframe {
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	int graph;
>  #endif
> +	struct llist_node *kr_cur;

As with the fgraph bits above, please make this depedn on the relevant
Kconfig, i.e.

| #ifdef CONFIG_KRETPROBES
| 	struct llist_node *kr_cur;
| #endif

>  };
>  
>  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 8982a2b78acf..f1eef5745542 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -129,6 +129,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  		frame->pc = ret_stack->ret;
>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +	if (is_kretprobe_trampoline(frame->pc))
> +		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);

Please ifdef this, like the CONFIG_FUNCTION_GRAPH_TRACER bits above.
i.e.

| #ifdef CONFIG_KRETPROBES
| 	if (is_kretprobe_trampoline(frame->pc))
| 		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
| #endif

What does kretprobe_find_ret_addr() do when it can't find the original
address? I couldn't spot it in this series or in mainline.

As a future thing, I'd like to be able to have monotonicity and
completeness checks as part of the unwind, i.e. checking that we consume
the kretprobe address *in-order*, and can identify whether we've skipped
any, so that we can identify when unwinding has gone wrong. Does it do
that today?

It'd be nice if it could signal failure reliably (without causing a
BUG() or similar), e.g. by returning an error code.

>  	frame->pc = ptrauth_strip_insn_pac(frame->pc);
>  
> @@ -224,6 +226,7 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  {
>  	struct stackframe frame;
>  
> +	memset(&frame, 0, sizeof(frame));

Please initialise stackframe::kr_cur in start_backtrace, where we
initialize all the other fields in struct stackframe, i.e. just after
the CONFIG_FUNCTION_GRAPH_TRACER bit, have:

| #ifdef CONFIG_KRETPROBES
| 	frame->kr_cur = NULL;
| #endif

Thanks,
Mark.

>  	if (regs)
>  		start_backtrace(&frame, regs->regs[29], regs->pc);
>  	else if (task == current)
> 

Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Nick Desaulniers]

On Tue, Oct 12, 2021 at 7:18 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Nick,
>
> On Mon, 11 Oct 2021 11:45:22 -0700
> Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> >
> > If I change from CONFIG_UNWINDER_ARM=y to
> > CONFIG_UNWINDER_FRAME_POINTER=y, before:
> >
> > # cat /proc/self/stack
> > [<0>] stack_trace_save_tsk+0x50/0x6c
> > [<0>] proc_pid_stack+0xa0/0xf8
> > [<0>] proc_single_show+0x50/0xbc
> > [<0>] seq_read_iter+0x178/0x4ec
> > [<0>] seq_read+0x138/0x15c
> > [<0>] vfs_read+0xd0/0x304
> > [<0>] ksys_read+0x78/0xd4
> > [<0>] sys_read+0xc/0x10
> >
> > after:
> > # cat /proc/self/stack
> > [<0>] proc_pid_stack+0xa0/0xf8
> > [<0>] proc_single_show+0x50/0xbc
> > [<0>] seq_read_iter+0x178/0x4ec
> > [<0>] seq_read+0x138/0x15c
> > [<0>] vfs_read+0xd0/0x304
> > [<0>] ksys_read+0x78/0xd4
> > [<0>] sys_read+0xc/0x10
> > [<0>] __entry_text_start+0x14/0x14
> > [<0>] 0xffffffff
> >
> > So I guess this helps the CONFIG_UNWINDER_FRAME_POINTER=y case? (That
> > final frame address looks wrong, but is potentially yet another bug;
> > perhaps for clang we need to manually store the previous frame's pc at
> > a different offset before jumping to __entry_text_start).
>
> Ah, yes. I didn't touch the UNWINDER_ARM. As you may know the
> unwind_frame()@arch/arm/kernel/stacktrace.c is compiled only
> CONFIG_UNWINDER_FRAME_POINTER=y.

Ah, right, thanks for pointing that out.

> > so likely this fixes/improves CONFIG_UNWINDER_FRAME_POINTER=y? Is that correct?
>
> Yes, that is correct.

In that case, additionally:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Masami Hiramatsu]

On Wed, 13 Oct 2021 11:01:39 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> > Record the frame pointer instead of stack address with kretprobe
> > instance as the identifier on the instance list.
> > Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> > actual frame pointer (x29).
> 
> Just to check, why do we need to use the FP rather than SP? It wasn't
> clear to me if that's necessary later in the series, or if I'm missing
> something here.

Actually, this is for finding correct return address from the per-task
kretprobe instruction list (suppose it as a shadow stack) when it will
be searched in stack-backtracing. At that point, the framepointer will
be a reliable key.

> 
> FWIW, I plan to rework arm64's ftrace bits to use FP for
> HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so I'm happy to do likewise here.

Yes, I think you can use FP for that too.

> 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Regardless of the above:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thank you!

> 
> Mark.
> 
> > ---
> >  arch/arm64/kernel/probes/kprobes.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > index e7ad6da980e8..d9dfa82c1f18 100644
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -401,14 +401,14 @@ int __init arch_populate_kprobe_blacklist(void)
> >  
> >  void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
> >  {
> > -	return (void *)kretprobe_trampoline_handler(regs, (void *)kernel_stack_pointer(regs));
> > +	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
> >  }
> >  
> >  void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> >  				      struct pt_regs *regs)
> >  {
> >  	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
> > -	ri->fp = (void *)kernel_stack_pointer(regs);
> > +	ri->fp = (void *)regs->regs[29];
> >  
> >  	/* replace return addr (x30) with trampoline */
> >  	regs->regs[30] = (long)&__kretprobe_trampoline;
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 5/8] arm64: Recover kretprobe modified return address in stacktrace

[Masami Hiramatsu]

On Wed, 13 Oct 2021 09:14:16 +0100
Will Deacon <will@kernel.org> wrote:

> On Fri, Oct 08, 2021 at 09:28:58PM +0900, Masami Hiramatsu wrote:
> > Since the kretprobe replaces the function return address with
> > the kretprobe_trampoline on the stack, stack unwinder shows it
> > instead of the correct return address.
> > 
> > This checks whether the next return address is the
> > __kretprobe_trampoline(), and if so, try to find the correct
> > return address from the kretprobe instance list.
> > 
> > With this fix, now arm64 can enable
> > CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE, and pass the
> > kprobe self tests.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/arm64/Kconfig                  |    1 +
> >  arch/arm64/include/asm/stacktrace.h |    2 ++
> >  arch/arm64/kernel/stacktrace.c      |    3 +++
> >  3 files changed, 6 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..edde5171ffb2 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -11,6 +11,7 @@ config ARM64
> >  	select ACPI_PPTT if ACPI
> >  	select ARCH_HAS_DEBUG_WX
> >  	select ARCH_BINFMT_ELF_STATE
> > +	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
> >  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
> >  	select ARCH_ENABLE_MEMORY_HOTPLUG
> >  	select ARCH_ENABLE_MEMORY_HOTREMOVE
> > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> > index 8aebc00c1718..8f997a602651 100644
> > --- a/arch/arm64/include/asm/stacktrace.h
> > +++ b/arch/arm64/include/asm/stacktrace.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/sched/task_stack.h>
> >  #include <linux/types.h>
> > +#include <linux/llist.h>
> >  
> >  #include <asm/memory.h>
> >  #include <asm/ptrace.h>
> > @@ -59,6 +60,7 @@ struct stackframe {
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >  	int graph;
> >  #endif
> > +	struct llist_node *kr_cur;
> >  };
> 
> Please update the comment above this structure to describe the new member
> you're adding.

OK, let me update that. 

> If it's only relevant for kprobes, then let's define it
> conditionally too (based on CONFIG_KRETPROBES ?)

Ah, good point! Yes, it must be only valid when CONFIG_KRETPROBES=y.

Thank you,

> 
> >  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index 8982a2b78acf..f1eef5745542 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -129,6 +129,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >  		frame->pc = ret_stack->ret;
> >  	}
> >  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> > +	if (is_kretprobe_trampoline(frame->pc))
> > +		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
> >  
> >  	frame->pc = ptrauth_strip_insn_pac(frame->pc);
> >  
> > @@ -224,6 +226,7 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> >  {
> >  	struct stackframe frame;
> >  
> > +	memset(&frame, 0, sizeof(frame));
> 
> Why do we need this?
> 
> Will


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Mark Rutland]

On Thu, Oct 14, 2021 at 05:04:05PM +0900, Masami Hiramatsu wrote:
> On Wed, 13 Oct 2021 11:01:39 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> > > Record the frame pointer instead of stack address with kretprobe
> > > instance as the identifier on the instance list.
> > > Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> > > actual frame pointer (x29).
> > 
> > Just to check, why do we need to use the FP rather than SP? It wasn't
> > clear to me if that's necessary later in the series, or if I'm missing
> > something here.
> 
> Actually, this is for finding correct return address from the per-task
> kretprobe instruction list (suppose it as a shadow stack) when it will
> be searched in stack-backtracing. At that point, the framepointer will
> be a reliable key.

Sure, my question was more "why isn't the SP a reliable key?", because both
the SP and FP should be balanced at function-entry and function-return
time. I'm asking because I think I'm missing a subtlety.

I'm perfectly happy to use the FP even if they're equivalent; I just
want to make sure there's not some issue I'm unaware of that could
affect unwinding.

Thanks,
Mark.

> > FWIW, I plan to rework arm64's ftrace bits to use FP for
> > HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so I'm happy to do likewise here.
> 
> Yes, I think you can use FP for that too.
> 
> > 
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Regardless of the above:
> > 
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> Thank you!
> 
> > 
> > Mark.
> > 
> > > ---
> > >  arch/arm64/kernel/probes/kprobes.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > > index e7ad6da980e8..d9dfa82c1f18 100644
> > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > @@ -401,14 +401,14 @@ int __init arch_populate_kprobe_blacklist(void)
> > >  
> > >  void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
> > >  {
> > > -	return (void *)kretprobe_trampoline_handler(regs, (void *)kernel_stack_pointer(regs));
> > > +	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
> > >  }
> > >  
> > >  void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> > >  				      struct pt_regs *regs)
> > >  {
> > >  	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
> > > -	ri->fp = (void *)kernel_stack_pointer(regs);
> > > +	ri->fp = (void *)regs->regs[29];
> > >  
> > >  	/* replace return addr (x30) with trampoline */
> > >  	regs->regs[30] = (long)&__kretprobe_trampoline;
> > > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 5/8] arm64: Recover kretprobe modified return address in stacktrace

[Masami Hiramatsu]

On Wed, 13 Oct 2021 11:13:51 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Fri, Oct 08, 2021 at 09:28:58PM +0900, Masami Hiramatsu wrote:
> > Since the kretprobe replaces the function return address with
> > the kretprobe_trampoline on the stack, stack unwinder shows it
> > instead of the correct return address.
> > 
> > This checks whether the next return address is the
> > __kretprobe_trampoline(), and if so, try to find the correct
> > return address from the kretprobe instance list.
> > 
> > With this fix, now arm64 can enable
> > CONFIG_ARCH_CORRECT_STACKTRACE_ON_KRETPROBE, and pass the
> > kprobe self tests.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/arm64/Kconfig                  |    1 +
> >  arch/arm64/include/asm/stacktrace.h |    2 ++
> >  arch/arm64/kernel/stacktrace.c      |    3 +++
> >  3 files changed, 6 insertions(+)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c7ae4c3954b..edde5171ffb2 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -11,6 +11,7 @@ config ARM64
> >  	select ACPI_PPTT if ACPI
> >  	select ARCH_HAS_DEBUG_WX
> >  	select ARCH_BINFMT_ELF_STATE
> > +	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE
> >  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
> >  	select ARCH_ENABLE_MEMORY_HOTPLUG
> >  	select ARCH_ENABLE_MEMORY_HOTREMOVE
> > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> > index 8aebc00c1718..8f997a602651 100644
> > --- a/arch/arm64/include/asm/stacktrace.h
> > +++ b/arch/arm64/include/asm/stacktrace.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/sched/task_stack.h>
> >  #include <linux/types.h>
> > +#include <linux/llist.h>
> >  
> >  #include <asm/memory.h>
> >  #include <asm/ptrace.h>
> > @@ -59,6 +60,7 @@ struct stackframe {
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >  	int graph;
> >  #endif
> > +	struct llist_node *kr_cur;
> 
> As with the fgraph bits above, please make this depedn on the relevant
> Kconfig, i.e.
> 
> | #ifdef CONFIG_KRETPROBES
> | 	struct llist_node *kr_cur;
> | #endif
> 
> >  };

OK.

> >  
> >  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index 8982a2b78acf..f1eef5745542 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -129,6 +129,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >  		frame->pc = ret_stack->ret;
> >  	}
> >  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> > +	if (is_kretprobe_trampoline(frame->pc))
> > +		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
> 
> Please ifdef this, like the CONFIG_FUNCTION_GRAPH_TRACER bits above.
> i.e.
> 
> | #ifdef CONFIG_KRETPROBES
> | 	if (is_kretprobe_trampoline(frame->pc))
> | 		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
> | #endif

OK.

> 
> What does kretprobe_find_ret_addr() do when it can't find the original
> address? I couldn't spot it in this series or in mainline.

If it couldn't find, it returns NULL.
Hmm, should we check it and keep frame->pc if the return value is NULL?
(anyway, it must not happen. If it happens, that task can not continue to run.)

> As a future thing, I'd like to be able to have monotonicity and
> completeness checks as part of the unwind, i.e. checking that we consume
> the kretprobe address *in-order*, and can identify whether we've skipped
> any, so that we can identify when unwinding has gone wrong. Does it do
> that today?

Good question. No today, but is easy to do since we have the loop cursor
(frame->kr_cur).

unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
                                      struct llist_node **cur)
{
...
        do {
                ret = __kretprobe_find_ret_addr(tsk, cur);
                if (!ret)
                        break;
                ri = container_of(*cur, struct kretprobe_instance, llist);
        } while (ri->fp != fp);
...

Change this loop to;

	ri = container_of(*cur, struct kretprobe_instance, llist);
	prev_fp = ri->fp;
	do {
		ret = __kretprobe_find_ret_addr(tsk, cur);
		if (WARN_ON_ONCE(!ret))
			return ERR_PTR(-ENOENT);
		ri = container_of(*cur, struct kretprobe_instance, llist);
	} while (ri->fp == prev_fp);
	if (ri->fp != fp)
		return ERR_PTR(-EILSEQ);

Then, we can detect that wrong sequence from stacktrace side.

> 
> It'd be nice if it could signal failure reliably (without causing a
> BUG() or similar), e.g. by returning an error code.

As above, -EILSEQ is OK?

> 
> >  	frame->pc = ptrauth_strip_insn_pac(frame->pc);
> >  
> > @@ -224,6 +226,7 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> >  {
> >  	struct stackframe frame;
> >  
> > +	memset(&frame, 0, sizeof(frame));
> 
> Please initialise stackframe::kr_cur in start_backtrace, where we
> initialize all the other fields in struct stackframe, i.e. just after
> the CONFIG_FUNCTION_GRAPH_TRACER bit, have:
> 
> | #ifdef CONFIG_KRETPROBES
> | 	frame->kr_cur = NULL;
> | #endif

OK, let me update it.

Thank you!

> 
> Thanks,
> Mark.
> 
> >  	if (regs)
> >  		start_backtrace(&frame, regs->regs[29], regs->pc);
> >  	else if (task == current)
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Masami Hiramatsu]

On Thu, 14 Oct 2021 10:13:32 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Oct 14, 2021 at 05:04:05PM +0900, Masami Hiramatsu wrote:
> > On Wed, 13 Oct 2021 11:01:39 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> > > > Record the frame pointer instead of stack address with kretprobe
> > > > instance as the identifier on the instance list.
> > > > Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> > > > actual frame pointer (x29).
> > > 
> > > Just to check, why do we need to use the FP rather than SP? It wasn't
> > > clear to me if that's necessary later in the series, or if I'm missing
> > > something here.
> > 
> > Actually, this is for finding correct return address from the per-task
> > kretprobe instruction list (suppose it as a shadow stack) when it will
> > be searched in stack-backtracing. At that point, the framepointer will
> > be a reliable key.
> 
> Sure, my question was more "why isn't the SP a reliable key?", because both
> the SP and FP should be balanced at function-entry and function-return
> time. I'm asking because I think I'm missing a subtlety.

Ah, because SP is not used while unwinding frame. For the kretprobe,
either FP or SP is OK. But for the stacktrace.c, I can not use SP
and is easy to change to use FP. :)

So, when we introduce ORC unwinder on arm64, I think I have to reconsider
using SP based on the configuration.

Thank you,

> 
> I'm perfectly happy to use the FP even if they're equivalent; I just
> want to make sure there's not some issue I'm unaware of that could
> affect unwinding.
> 
> Thanks,
> Mark.
> 
> > > FWIW, I plan to rework arm64's ftrace bits to use FP for
> > > HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so I'm happy to do likewise here.
> > 
> > Yes, I think you can use FP for that too.
> > 
> > > 
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > 
> > > Regardless of the above:
> > > 
> > > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > Thank you!
> > 
> > > 
> > > Mark.
> > > 
> > > > ---
> > > >  arch/arm64/kernel/probes/kprobes.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > > > index e7ad6da980e8..d9dfa82c1f18 100644
> > > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > > @@ -401,14 +401,14 @@ int __init arch_populate_kprobe_blacklist(void)
> > > >  
> > > >  void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
> > > >  {
> > > > -	return (void *)kretprobe_trampoline_handler(regs, (void *)kernel_stack_pointer(regs));
> > > > +	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->regs[29]);
> > > >  }
> > > >  
> > > >  void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
> > > >  				      struct pt_regs *regs)
> > > >  {
> > > >  	ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
> > > > -	ri->fp = (void *)kernel_stack_pointer(regs);
> > > > +	ri->fp = (void *)regs->regs[29];
> > > >  
> > > >  	/* replace return addr (x30) with trampoline */
> > > >  	regs->regs[30] = (long)&__kretprobe_trampoline;
> > > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Mark Rutland]

On Thu, Oct 14, 2021 at 07:01:55PM +0900, Masami Hiramatsu wrote:
> On Thu, 14 Oct 2021 10:13:32 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Thu, Oct 14, 2021 at 05:04:05PM +0900, Masami Hiramatsu wrote:
> > > On Wed, 13 Oct 2021 11:01:39 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > > On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> > > > > Record the frame pointer instead of stack address with kretprobe
> > > > > instance as the identifier on the instance list.
> > > > > Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> > > > > actual frame pointer (x29).
> > > > 
> > > > Just to check, why do we need to use the FP rather than SP? It wasn't
> > > > clear to me if that's necessary later in the series, or if I'm missing
> > > > something here.
> > > 
> > > Actually, this is for finding correct return address from the per-task
> > > kretprobe instruction list (suppose it as a shadow stack) when it will
> > > be searched in stack-backtracing. At that point, the framepointer will
> > > be a reliable key.
> > 
> > Sure, my question was more "why isn't the SP a reliable key?", because both
> > the SP and FP should be balanced at function-entry and function-return
> > time. I'm asking because I think I'm missing a subtlety.
> 
> Ah, because SP is not used while unwinding frame. For the kretprobe,
> either FP or SP is OK. But for the stacktrace.c, I can not use SP
> and is easy to change to use FP. :)

Ah, so this is just so that stacktrace can match the address. For
clarity, would you be happy to add a sentence to the commit message like:

| This will allow the stacktrace code to find the original return
| address from the FP alone.

Thanks,
Mark.

Re: [PATCH 3/8] arm64: kprobes: Record frame pointer with kretprobe instance

[Masami Hiramatsu]

On Thu, 14 Oct 2021 11:27:02 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Oct 14, 2021 at 07:01:55PM +0900, Masami Hiramatsu wrote:
> > On Thu, 14 Oct 2021 10:13:32 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > On Thu, Oct 14, 2021 at 05:04:05PM +0900, Masami Hiramatsu wrote:
> > > > On Wed, 13 Oct 2021 11:01:39 +0100
> > > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > > 
> > > > > On Fri, Oct 08, 2021 at 09:28:39PM +0900, Masami Hiramatsu wrote:
> > > > > > Record the frame pointer instead of stack address with kretprobe
> > > > > > instance as the identifier on the instance list.
> > > > > > Since arm64 always enable CONFIG_FRAME_POINTER, we can use the
> > > > > > actual frame pointer (x29).
> > > > > 
> > > > > Just to check, why do we need to use the FP rather than SP? It wasn't
> > > > > clear to me if that's necessary later in the series, or if I'm missing
> > > > > something here.
> > > > 
> > > > Actually, this is for finding correct return address from the per-task
> > > > kretprobe instruction list (suppose it as a shadow stack) when it will
> > > > be searched in stack-backtracing. At that point, the framepointer will
> > > > be a reliable key.
> > > 
> > > Sure, my question was more "why isn't the SP a reliable key?", because both
> > > the SP and FP should be balanced at function-entry and function-return
> > > time. I'm asking because I think I'm missing a subtlety.
> > 
> > Ah, because SP is not used while unwinding frame. For the kretprobe,
> > either FP or SP is OK. But for the stacktrace.c, I can not use SP
> > and is easy to change to use FP. :)
> 
> Ah, so this is just so that stacktrace can match the address. For
> clarity, would you be happy to add a sentence to the commit message like:
> 
> | This will allow the stacktrace code to find the original return
> | address from the FP alone.

Yes, I'm happy to update the changelog :)

Thanks!

> 
> Thanks,
> Mark.


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Russell King (Oracle)]

On Fri, Oct 08, 2021 at 09:29:08PM +0900, Masami Hiramatsu wrote:
> Currently the stacktrace on clang compiled arm kernel uses the 'lr'
> register to find the first frame address from pt_regs. However, that
> is wrong after calling another function, because the 'lr' register
> is used by 'bl' instruction and never be recovered.
> 
> As same as gcc arm kernel, directly use the frame pointer (x11) of
> the pt_regs to find the first frame address.

Can I ask that the subject line is corrected. It's "rely" not "relay".

Also, the frame pointer is called "r11" not "x11" if you want to use
the numerical register reference for 32-bit ARM registers.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Masami Hiramatsu]

On Thu, 14 Oct 2021 17:53:24 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Fri, Oct 08, 2021 at 09:29:08PM +0900, Masami Hiramatsu wrote:
> > Currently the stacktrace on clang compiled arm kernel uses the 'lr'
> > register to find the first frame address from pt_regs. However, that
> > is wrong after calling another function, because the 'lr' register
> > is used by 'bl' instruction and never be recovered.
> > 
> > As same as gcc arm kernel, directly use the frame pointer (x11) of
> > the pt_regs to find the first frame address.
> 
> Can I ask that the subject line is corrected. It's "rely" not "relay".

Oops, yes, that's my typo. Thanks for correcting!

> 
> Also, the frame pointer is called "r11" not "x11" if you want to use
> the numerical register reference for 32-bit ARM registers.

Oh, I mixed up the register name between arm64 and ARM...

Thank you,

> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!


-- 
Masami Hiramatsu <mhiramat@kernel.org>