live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: madvenka@linux.microsoft.com
To: broonie@kernel.org, jpoimboe@redhat.com, mark.rutland@arm.com,
	jthierry@redhat.com, catalin.marinas@arm.com, will@kernel.org,
	jmorris@namei.org, pasha.tatashin@soleen.com,
	linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	madvenka@linux.microsoft.com
Subject: [RFC PATCH v3 4/4] arm64: Handle funtion graph tracer better in the unwinder
Date: Mon,  3 May 2021 12:36:15 -0500	[thread overview]
Message-ID: <20210503173615.21576-5-madvenka@linux.microsoft.com> (raw)
In-Reply-To: <20210503173615.21576-1-madvenka@linux.microsoft.com>

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

The Function Graph Tracer modifies the return address of a traced function
to a return trampoline (return_to_handler()) to gather tracing data on
function return. When the unwinder encounters return_to_handler(), it calls
ftrace_graph_get_ret_stack() to lookup the original return address in the
return address stack.

This lookup will succeed as long as the unwinder is invoked when the traced
function is executing. However, when the traced function returns and control
goes to return_to_handler(), this lookup will not succeed because:

- the return address on the stack would not be return_to_handler. It would
  be return_to_handler+someoffset. To solve this, get the address range for
  return_to_handler() by looking up its symbol table entry and check if
  frame->pc falls in the range. This is also required for the unwinder to
  maintain the index into the return address stack correctly as it unwinds
  through Function Graph trace return trampolines.

- the original return address will be popped off the return address stack
  at some point. From this point till the end of return_to_handler(),
  the lookup will not succeed. The stack trace is unreliable in that
  window.

On arm64, each return address stack entry also stores the FP of the
caller of the traced function. Compare the FP in the current frame
with the entry that is looked up. If the FP matches, then, all is
well. Else, it is in the window. mark the stack trace unreliable.

Although it is possible to close the window mentioned above, it is
not worth it. It is a tiny window.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
 arch/arm64/include/asm/stacktrace.h |  3 ++
 arch/arm64/kernel/stacktrace.c      | 60 ++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index f1eab6b029f7..e70a2a6451db 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -69,6 +69,7 @@ extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 			    bool (*fn)(void *, unsigned long), void *data);
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 			   const char *loglvl);
+extern void init_ranges(void);
 
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
 
@@ -154,6 +155,8 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
 static inline void start_backtrace(struct stackframe *frame,
 				   unsigned long fp, unsigned long pc)
 {
+	init_ranges();
+
 	frame->fp = fp;
 	frame->pc = pc;
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 33e174160f9b..7504aec79faa 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -26,6 +26,9 @@ struct code_range {
 
 struct code_range	sym_code_ranges[] =
 {
+	/* unwindable ranges */
+	{ (unsigned long)return_to_handler, 0 },
+
 	/* non-unwindable ranges */
 	{ (unsigned long)__entry_text_start,
 	  (unsigned long)__entry_text_end },
@@ -48,6 +51,33 @@ struct code_range	sym_code_ranges[] =
 	{ /* sentinel */ }
 };
 
+void init_ranges(void)
+{
+	static char sym[KSYM_NAME_LEN];
+	static bool inited = false;
+	struct code_range *range;
+	unsigned long pc, size, offset;
+
+	if (inited)
+		return;
+
+	for (range = sym_code_ranges; range->start; range++) {
+		if (range->end)
+			continue;
+
+		pc = (unsigned long)range->start;
+		if (kallsyms_lookup(pc, &size, &offset, NULL, sym)) {
+			range->start = pc - offset;
+			range->end = range->start + size;
+		} else {
+			/* Range will only include one instruction */
+			range->start = pc;
+			range->end = pc + 4;
+		}
+	}
+	inited = true;
+}
+
 static struct code_range *lookup_range(unsigned long pc)
 {
 	struct code_range *range;
@@ -149,19 +179,29 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	if (tsk->ret_stack &&
-		frame->pc == (unsigned long)return_to_handler) {
+		range->start == (unsigned long)return_to_handler) {
 		struct ftrace_ret_stack *ret_stack;
 		/*
-		 * This is a case where function graph tracer has
-		 * modified a return address (LR) in a stack frame
-		 * to hook a function return.
-		 * So replace it to an original value.
+		 * Either the function graph tracer has modified a return
+		 * address (LR) in a stack frame to the return trampoline.
+		 * Or, the return trampoline itself is executing upon the
+		 * return of a traced function. Lookup the original return
+		 * address and replace frame->pc with it.
+		 *
+		 * However, the return trampoline pops the original return
+		 * address off the return address stack at some point. So,
+		 * there is a small window towards the end of the return
+		 * trampoline where the lookup will fail. In that case,
+		 * mark the stack trace as unreliable and proceed.
 		 */
-		ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++);
-		if (WARN_ON_ONCE(!ret_stack))
-			return -EINVAL;
-		frame->pc = ret_stack->ret;
-		frame->pc = ptrauth_strip_insn_pac(frame->pc);
+		ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph);
+		if (!ret_stack || frame->fp != ret_stack->fp) {
+			frame->reliable = false;
+		} else {
+			frame->pc = ret_stack->ret;
+			frame->pc = ptrauth_strip_insn_pac(frame->pc);
+			frame->graph++;
+		}
 		return 0;
 	}
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.25.1


  parent reply	other threads:[~2021-05-03 17:36 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <65cf4dfbc439b010b50a0c46ec500432acde86d6>
2021-05-03 17:36 ` [RFC PATCH v3 0/4] arm64: Stack trace reliability checks " madvenka
2021-05-03 17:36   ` [RFC PATCH v3 1/4] arm64: Introduce stack " madvenka
2021-05-04 15:50     ` Mark Brown
2021-05-04 19:14       ` Madhavan T. Venkataraman
2021-05-04 21:52     ` Josh Poimboeuf
2021-05-04 23:13       ` Madhavan T. Venkataraman
2021-05-05  0:07         ` Josh Poimboeuf
2021-05-05  0:21           ` Madhavan T. Venkataraman
2021-05-03 17:36   ` [RFC PATCH v3 2/4] arm64: Check the return PC against unreliable code sections madvenka
2021-05-04 16:05     ` Mark Brown
2021-05-04 19:03       ` Madhavan T. Venkataraman
2021-05-04 19:32         ` Madhavan T. Venkataraman
2021-05-05 16:46           ` Mark Brown
2021-05-05 18:48             ` Madhavan T. Venkataraman
2021-05-05 18:50               ` Madhavan T. Venkataraman
2021-05-06 13:45               ` Mark Brown
2021-05-06 15:21                 ` Madhavan T. Venkataraman
2021-05-05 16:34         ` Mark Brown
2021-05-05 17:51           ` Madhavan T. Venkataraman
2021-05-05 19:30     ` Ard Biesheuvel
2021-05-05 20:00       ` Madhavan T. Venkataraman
2021-05-03 17:36   ` [RFC PATCH v3 3/4] arm64: Handle miscellaneous functions in .text and .init.text madvenka
2021-05-06 14:12     ` Mark Brown
2021-05-06 15:30       ` Madhavan T. Venkataraman
2021-05-06 15:32         ` Madhavan T. Venkataraman
2021-05-06 15:44           ` Mark Brown
2021-05-06 15:56             ` Madhavan T. Venkataraman
2021-05-06 15:37         ` Mark Brown
2021-05-06 15:57           ` Madhavan T. Venkataraman
2021-05-03 17:36   ` madvenka [this message]
2021-05-06 14:43     ` [RFC PATCH v3 4/4] arm64: Handle funtion graph tracer better in the unwinder Mark Brown
2021-05-06 15:20       ` Madhavan T. Venkataraman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210503173615.21576-5-madvenka@linux.microsoft.com \
    --to=madvenka@linux.microsoft.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=jmorris@namei.org \
    --cc=jpoimboe@redhat.com \
    --cc=jthierry@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pasha.tatashin@soleen.com \
    --cc=will@kernel.org \
    --subject='Re: [RFC PATCH v3 4/4] arm64: Handle funtion graph tracer better in the unwinder' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).