[RFC PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[RFC PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[madvenka]

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

I have synced this patch series to v5.19-rc2.
I have also removed the following patch.

	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE

as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
yet. This patch will be added in the future once Objtool is enhanced to
provide stack validation in some form.

Split unwind_init()
===================

Unwind initialization has 3 cases. Accordingly, define 3 separate init
functions as follows:

	- unwind_init_from_regs()
	- unwind_init_from_current()
	- unwind_init_from_task()

This makes it easier to understand and add specialized code to each case
in the future.

Copy task argument
==================

Copy the task argument passed to arch_stack_walk() to unwind_state so that
it can be passed to unwind functions via unwind_state rather than as a
separate argument. The task is a fundamental part of the unwind state.

Redefine the unwinder loop
==========================

Redefine the unwinder loop and make it simple and somewhat similar to other
architectures. Define the following:

	while (unwind_continue(&state, consume_entry, cookie))
		unwind_next(&state);

unwind_continue()
	This new function implements checks to determine whether the
	unwind should continue or terminate.

Reliability checks
==================

There are some kernel features and conditions that make a stack trace
unreliable. Callers may require the unwinder to detect these cases.
E.g., livepatch.

Introduce a new function called unwind_check_reliability() that will detect
these cases and set a boolean "reliable" in the stackframe. Call
unwind_check_reliability() for every frame.

Introduce the first reliability check in unwind_check_reliability() - If
a return PC is not a valid kernel text address, consider the stack
trace unreliable. It could be some generated code.

Other reliability checks will be added in the future.

Make unwind() return a boolean to indicate reliability of the stack trace.

SYM_CODE check
==============

This is the second reliability check implemented.

SYM_CODE functions do not follow normal calling conventions. They cannot
be unwound reliably using the frame pointer. Collect the address ranges
of these functions in a special section called "sym_code_functions".

In unwind_check_reliability(), check the return PC against these ranges. If
a match is found, then mark the stack trace unreliable.

Last stack frame
================

If a SYM_CODE function occurs in the very last frame in the stack trace,
then the stack trace is not considered unreliable. This is because there
is no more unwinding to do. Examples:

	- EL0 exception stack traces end in the top level EL0 exception
	  handlers.

	- All kernel thread stack traces end in ret_from_fork().

arch_stack_walk_reliable()
==========================

Introduce arch_stack_walk_reliable() for ARM64. This works like
arch_stack_walk() except that it returns an error if the stack trace is
found to be unreliable.

Until all of the reliability checks are in place in
unwind_check_reliability(), arch_stack_walk_reliable() may not be used by
livepatch. But it may be used by debug and test code.

HAVE_RELIABLE_STACKTRACE
========================

Select this config for arm64. However, make it conditional on
STACK_VALIDATION. When objtool is enhanced to implement stack
validation for arm64, STACK_VALIDATION will be defined.

---
Changelog:
v15:
	From Mark Brown:

	- Sync this patch series to v5.19-rc2.

	From Madhavan T. Venkataraman:

	- Remove the following patch from the series:

	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE

	  as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is
	  not present yet. This patch will be added in the future once
	  Objtool is enhanced to provide stack validation in some form.

v14:
	From Mark Rutland, Mark Brown:

	- Add requirements for the three helper functions that init a stack
	  trace.

	From Mark Rutland:

	- Change the comment for the task field in struct stackframe.

	- Hard code the task to current in unwind_init_from_regs(). Add a
	  sanity check task == current.

	- Rename unwind_init_from_current() to unwind_init_from_caller().

	- Remove task argument from unwind_init_from_caller().

	From Mark Brown:

	- Reviewed-By: for:

	[PATCH v13 05/11] arm64: Copy the task argument to unwind_state
v13:
	From Mark Brown:

	- Reviewed-by for the following:

	[PATCH v12 03/10] arm64: Rename stackframe to unwind_state
	[PATCH v11 05/10] arm64: Copy unwind arguments to unwind_state
	[PATCH v11 07/10] arm64: Introduce stack trace reliability checks
	                  in the unwinder
	[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
	                return PC against list

	From Mark Rutland:

	- Reviewed-by for the following:

	[PATCH v12 01/10] arm64: Remove NULL task check from unwind_frame()
	[PATCH v12 02/10] arm64: Rename unwinder functions
	[PATCH v12 03/10] arm64: Rename stackframe to unwind_state

	- For each of the 3 cases of unwind initialization, have a separate
	  init function. Call the common init from each of these init
	  functions rather than call it separately.

	- Only copy the task argument to arch_stack_walk() into
	  unwind state. Pass the rest of the arguments as arguments to
	  unwind functions.

v12:
	From Mark Brown:

	- Reviewed-by for the following:

	[PATCH v11 1/5] arm64: Call stack_backtrace() only from within
	                walk_stackframe()
	[PATCH v11 2/5] arm64: Rename unwinder functions
	[PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to
	                other architectures
	[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
	                return PC against list

	- Add an extra patch at the end to select HAVE_RELIABLE_STACKTRACE
	  just as a place holder for the review. I have added it and made
	  it conditional on STACK_VALIDATION which has not yet been
	  implemented.

	- Mark had a concern about the code for the check for the final
	  frame being repeated in two places. I have now added a new
	  field called "final_fp" in struct stackframe which I compute
	  once in stacktrace initialization. I have added an explicit
	  comment that the stacktrace must terminate at the final_fp.

	- Place the implementation of arch_stack_walk_reliable() in a
	  separate patch after all the reliability checks have been
	  implemented.

	From Mark Rutland:

	- Place the removal of the NULL task check in unwind_frame() in
	  a separate patch.

	- Add a task field to struct stackframe so the task pointer can be
	  passed around via the frame instead of as a separate argument. I have
	  taken this a step further by copying all of the arguments to
	  arch_stack_walk() into struct stackframe so that only that
	  struct needs to be passed to unwind functions.

	- Rename start_backtrace() to unwind_init() instead of unwind_start().

	- Acked-by for the following:

	[PATCH v11 2/5] arm64: Rename unwinder functions

	- Rename "struct stackframe" to "struct unwind_state".

	- Define separate inline functions for initializing the starting
	  FP and PC from regs, or caller, or blocked task. Don't merge
	  unwind_init() into unwind().

v11:
	From Mark Rutland:

	- Peter Zijlstra has submitted patches that make ARCH_STACKWALK
	  independent of STACKTRACE. Mark Rutland extracted some of the
	  patches from my v10 series and added his own patches and comments,
	  rebased it on top of Peter's changes and submitted the series.
	  
	  So, I have rebased the rest of the patches from v10 on top of
	  Mark Rutland's changes.

	- Split the renaming of the unwinder functions and annotating them
	  with notrace and NOKPROBE_SYMBOL(). Also, there is currently no
	  need to annotate unwind_start() as its caller is already annotated
	  properly. So, I am removing the annotation patch from the series.
	  This can be done separately later if deemed necessary. Similarly,
	  I have removed the annotations from unwind_check_reliability() and
	  unwind_continue().

	From Nobuta Keiya:

	- unwind_start() should check for final frame and not mark the
	  final frame unreliable.

v9, v10:
	- v9 had a threading problem. So, I resent it as v10.

	From me:

	- Removed the word "RFC" from the subject line as I believe this
	  is mature enough to be a regular patch.

	From Mark Brown, Mark Rutland:

	- Split the patches into smaller, self-contained ones.

	- Always enable STACKTRACE so that arch_stack_walk() is always
	  defined.

	From Mark Rutland:

	- Update callchain_trace() take the return value of
	  perf_callchain_store() into acount.

	- Restore get_wchan() behavior to the original code.

	- Simplify an if statement in dump_backtrace().

	From Mark Brown:

	- Do not abort the stack trace on the first unreliable frame.

	
v8:
	- Synced to v5.14-rc5.

	From Mark Rutland:

	- Make the unwinder loop similar to other architectures.

	- Keep details to within the unwinder functions and return a simple
	  boolean to the caller.

	- Convert some of the current code that contains unwinder logic to
	  simply use arch_stack_walk(). I have converted all of them.

	- Do not copy sym_code_functions[]. Just place it in rodata for now.

	- Have the main loop check for termination conditions rather than
	  having unwind_frame() check for them. In other words, let
	  unwind_frame() assume that the fp is valid.

	- Replace the big comment for SYM_CODE functions with a shorter
	  comment.

		/*
		 * As SYM_CODE functions don't follow the usual calling
		 * conventions, we assume by default that any SYM_CODE function
		 * cannot be unwound reliably.
		 *
		 * Note that this includes:
		 *
		 * - Exception handlers and entry assembly
		 * - Trampoline assembly (e.g., ftrace, kprobes)
		 * - Hypervisor-related assembly
		 * - Hibernation-related assembly
		 * - CPU start-stop, suspend-resume assembly
		 * - Kernel relocation assembly
		 */

v7:
	The Mailer screwed up the threading on this. So, I have resent this
	same series as version 8 with proper threading to avoid confusion.
v6:
	From Mark Rutland:

	- The per-frame reliability concept and flag are acceptable. But more
	  work is needed to make the per-frame checks more accurate and more
	  complete. E.g., some code reorg is being worked on that will help.

	  I have now removed the frame->reliable flag and deleted the whole
	  concept of per-frame status. This is orthogonal to this patch series.
	  Instead, I have improved the unwinder to return proper return codes
	  so a caller can take appropriate action without needing per-frame
	  status.

	- Remove the mention of PLTs and update the comment.

	  I have replaced the comment above the call to __kernel_text_address()
	  with the comment suggested by Mark Rutland.

	Other comments:

	- Other comments on the per-frame stuff are not relevant because
	  that approach is not there anymore.

v5:
	From Keiya Nobuta:
	
	- The term blacklist(ed) is not to be used anymore. I have changed it
	  to unreliable. So, the function unwinder_blacklisted() has been
	  changed to unwinder_is_unreliable().

	From Mark Brown:

	- Add a comment for the "reliable" flag in struct stackframe. The
	  reliability attribute is not complete until all the checks are
	  in place. Added a comment above struct stackframe.

	- Include some of the comments in the cover letter in the actual
	  code so that we can compare it with the reliable stack trace
	  requirements document for completeness. I have added a comment:

	  	- above unwinder_is_unreliable() that lists the requirements
		  that are addressed by the function.

		- above the __kernel_text_address() call about all the cases
		  the call covers.

v4:
	From Mark Brown:

	- I was checking the return PC with __kernel_text_address() before
	  the Function Graph trace handling. Mark Brown felt that all the
	  reliability checks should be performed on the original return PC
	  once that is obtained. So, I have moved all the reliability checks
	  to after the Function Graph Trace handling code in the unwinder.
	  Basically, the unwinder should perform PC translations first (for
	  rhe return trampoline for Function Graph Tracing, Kretprobes, etc).
	  Then, the reliability checks should be applied to the resulting
	  PC.

	- Mark said to improve the naming of the new functions so they don't
	  collide with existing ones. I have used a prefix "unwinder_" for
	  all the new functions.

	From Josh Poimboeuf:

	- In the error scenarios in the unwinder, the reliable flag in the
	  stack frame should be set. Implemented this.

	- Some of the other comments are not relevant to the new code as
	  I have taken a different approach in the new code. That is why
	  I have not made those changes. E.g., Ard wanted me to add the
	  "const" keyword to the global section array. That array does not
	  exist in v4. Similarly, Mark Brown said to use ARRAY_SIZE() for
	  the same array in a for loop.

	Other changes:

	- Add a new definition for SYM_CODE_END() that adds the address
	  range of the function to a special section called
	  "sym_code_functions".

	- Include the new section under initdata in vmlinux.lds.S.

	- Define an early_initcall() to copy the contents of the
	  "sym_code_functions" section to an array by the same name.

	- Define a function unwinder_blacklisted() that compares a return
	  PC against sym_code_sections[]. If there is a match, mark the
	  stack trace unreliable. Call this from unwind_frame().

v3:
	- Implemented a sym_code_ranges[] array to contains sections bounds
	  for text sections that contain SYM_CODE_*() functions. The unwinder
	  checks each return PC against the sections. If it falls in any of
	  the sections, the stack trace is marked unreliable.

	- Moved SYM_CODE functions from .text and .init.text into a new
	  text section called ".code.text". Added this section to
	  vmlinux.lds.S and sym_code_ranges[].

	- Fixed the logic in the unwinder that handles Function Graph
	  Tracer return trampoline.

	- Removed all the previous code that handles:
		- ftrace entry code for traced function
		- special_functions[] array that lists individual functions
		- kretprobe_trampoline() special case

v2
	- Removed the terminating entry { 0, 0 } in special_functions[]
	  and replaced it with the idiom { /* sentinel */ }.

	- Change the ftrace trampoline entry ftrace_graph_call in
	  special_functions[] to ftrace_call + 4 and added explanatory
	  comments.

	- Unnested #ifdefs in special_functions[] for FTRACE.

v1
	- Define a bool field in struct stackframe. This will indicate if
	  a stack trace is reliable.

	- Implement a special_functions[] array that will be populated
	  with special functions in which the stack trace is considered
	  unreliable.
	
	- Using kallsyms_lookup(), get the address ranges for the special
	  functions and record them.

	- Implement an is_reliable_function(pc). This function will check
	  if a given return PC falls in any of the special functions. If
	  it does, the stack trace is unreliable.

	- Implement check_reliability() function that will check if a
	  stack frame is reliable. Call is_reliable_function() from
	  check_reliability().

	- Before a return PC is checked against special_funtions[], it
	  must be validates as a proper kernel text address. Call
	  __kernel_text_address() from check_reliability().

	- Finally, call check_reliability() from unwind_frame() for
	  each stack frame.

	- Add EL1 exception handlers to special_functions[].

		el1_sync();
		el1_irq();
		el1_error();
		el1_sync_invalid();
		el1_irq_invalid();
		el1_fiq_invalid();
		el1_error_invalid();

	- The above functions are currently defined as LOCAL symbols.
	  Make them global so that they can be referenced from the
	  unwinder code.

	- Add FTRACE trampolines to special_functions[]:

		ftrace_graph_call()
		ftrace_graph_caller()
		return_to_handler()

	- Add the kretprobe trampoline to special functions[]:

		kretprobe_trampoline()

Previous versions and discussion
================================

v14: https://lore.kernel.org/linux-arm-kernel/20220413140528.3815-1-madvenka@linux.microsoft.com/T/#t
v13: https://lore.kernel.org/linux-arm-kernel/20220117145608.6781-1-madvenka@linux.microsoft.com/T/#t
v12: https://lore.kernel.org/linux-arm-kernel/20220103165212.9303-1-madvenka@linux.microsoft.com/T/#m21e86eecb9b8f0831196568f0bf62c3b56f65bf0
v11: https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@linux.microsoft.com/T/#t
v10: https://lore.kernel.org/linux-arm-kernel/4b3d5552-590c-e6a0-866b-9bc51da7bebf@linux.microsoft.com/T/#t
v9: Mailer screwed up the threading. Sent the same as v10 with proper threading.
v8: https://lore.kernel.org/linux-arm-kernel/20210812190603.25326-1-madvenka@linux.microsoft.com/
v7: Mailer screwed up the threading. Sent the same as v8 with proper threading.
v6: https://lore.kernel.org/linux-arm-kernel/20210630223356.58714-1-madvenka@linux.microsoft.com/
v5: https://lore.kernel.org/linux-arm-kernel/20210526214917.20099-1-madvenka@linux.microsoft.com/
v4: https://lore.kernel.org/linux-arm-kernel/20210516040018.128105-1-madvenka@linux.microsoft.com/
v3: https://lore.kernel.org/linux-arm-kernel/20210503173615.21576-1-madvenka@linux.microsoft.com/
v2: https://lore.kernel.org/linux-arm-kernel/20210405204313.21346-1-madvenka@linux.microsoft.com/
v1: https://lore.kernel.org/linux-arm-kernel/20210330190955.13707-1-madvenka@linux.microsoft.com/

Madhavan T. Venkataraman (6):
  arm64: Split unwind_init()
  arm64: Copy the task argument to unwind_state
  arm64: Make the unwind loop in unwind() similar to other architectures
  arm64: Introduce stack trace reliability checks in the unwinder
  arm64: Create a list of SYM_CODE functions, check return PC against
    list
  arm64: Introduce arch_stack_walk_reliable()

 arch/arm64/include/asm/linkage.h  |  11 ++
 arch/arm64/include/asm/sections.h |   1 +
 arch/arm64/kernel/stacktrace.c    | 266 +++++++++++++++++++++++++-----
 arch/arm64/kernel/vmlinux.lds.S   |  10 ++
 4 files changed, 246 insertions(+), 42 deletions(-)


base-commit: b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v15 1/6] arm64: Split unwind_init()

[madvenka]

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

unwind_init() is currently a single function that initializes all of the
unwind state. Split it into the following functions and call them
appropriately:

	- unwind_init_from_regs() - initialize from regs passed by caller.

	- unwind_init_from_caller() - initialize for the current task
	  from the caller of arch_stack_walk().

	- unwind_init_from_task() - initialize from the saved state of a
	  task other than the current task. In this case, the other
	  task must not be running.

This is done for two reasons:

	- the different ways of initializing are clear

	- specialized code can be added to each initializer in the future.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 66 ++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 0467cb79f080..e44f93ff25f0 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -50,11 +50,8 @@ struct unwind_state {
 #endif
 };
 
-static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
-				unsigned long pc)
+static void unwind_init_common(struct unwind_state *state)
 {
-	state->fp = fp;
-	state->pc = pc;
 #ifdef CONFIG_KRETPROBES
 	state->kr_cur = NULL;
 #endif
@@ -72,7 +69,57 @@ static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
 	state->prev_fp = 0;
 	state->prev_type = STACK_TYPE_UNKNOWN;
 }
-NOKPROBE_SYMBOL(unwind_init);
+
+/*
+ * Start an unwind from a pt_regs.
+ *
+ * The unwind will begin at the PC within the regs.
+ *
+ * The regs must be on a stack currently owned by the calling task.
+ */
+static inline void unwind_init_from_regs(struct unwind_state *state,
+					 struct pt_regs *regs)
+{
+	unwind_init_common(state);
+
+	state->fp = regs->regs[29];
+	state->pc = regs->pc;
+}
+
+/*
+ * Start an unwind from a caller.
+ *
+ * The unwind will begin at the caller of whichever function this is inlined
+ * into.
+ *
+ * The function which invokes this must be noinline.
+ */
+static __always_inline void unwind_init_from_caller(struct unwind_state *state)
+{
+	unwind_init_common(state);
+
+	state->fp = (unsigned long)__builtin_frame_address(1);
+	state->pc = (unsigned long)__builtin_return_address(0);
+}
+
+/*
+ * Start an unwind from a blocked task.
+ *
+ * The unwind will begin at the blocked tasks saved PC (i.e. the caller of
+ * cpu_switch_to()).
+ *
+ * The caller should ensure the task is blocked in cpu_switch_to() for the
+ * duration of the unwind, or the unwind will be bogus. It is never valid to
+ * call this for the current task.
+ */
+static inline void unwind_init_from_task(struct unwind_state *state,
+					 struct task_struct *task)
+{
+	unwind_init_common(state);
+
+	state->fp = thread_saved_fp(task);
+	state->pc = thread_saved_pc(task);
+}
 
 /*
  * Unwind from one frame record (A) to the next frame record (B).
@@ -213,14 +260,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 	struct unwind_state state;
 
 	if (regs)
-		unwind_init(&state, regs->regs[29], regs->pc);
+		unwind_init_from_regs(&state, regs);
 	else if (task == current)
-		unwind_init(&state,
-				(unsigned long)__builtin_frame_address(1),
-				(unsigned long)__builtin_return_address(0));
+		unwind_init_from_caller(&state);
 	else
-		unwind_init(&state, thread_saved_fp(task),
-				thread_saved_pc(task));
+		unwind_init_from_task(&state, task);
 
 	unwind(task, &state, consume_entry, cookie);
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v15 2/6] arm64: Copy the task argument to unwind_state

[madvenka]

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

Copy the task argument passed to arch_stack_walk() to unwind_state so that
it can be passed to unwind functions via unwind_state rather than as a
separate argument. The task is a fundamental part of the unwind state.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e44f93ff25f0..8e43444d50e2 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -38,6 +38,8 @@
  * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
  *               associated with the most recently encountered replacement lr
  *               value.
+ *
+ * @task:        The task being unwound.
  */
 struct unwind_state {
 	unsigned long fp;
@@ -48,10 +50,13 @@ struct unwind_state {
 #ifdef CONFIG_KRETPROBES
 	struct llist_node *kr_cur;
 #endif
+	struct task_struct *task;
 };
 
-static void unwind_init_common(struct unwind_state *state)
+static void unwind_init_common(struct unwind_state *state,
+			       struct task_struct *task)
 {
+	state->task = task;
 #ifdef CONFIG_KRETPROBES
 	state->kr_cur = NULL;
 #endif
@@ -80,7 +85,7 @@ static void unwind_init_common(struct unwind_state *state)
 static inline void unwind_init_from_regs(struct unwind_state *state,
 					 struct pt_regs *regs)
 {
-	unwind_init_common(state);
+	unwind_init_common(state, current);
 
 	state->fp = regs->regs[29];
 	state->pc = regs->pc;
@@ -96,7 +101,7 @@ static inline void unwind_init_from_regs(struct unwind_state *state,
  */
 static __always_inline void unwind_init_from_caller(struct unwind_state *state)
 {
-	unwind_init_common(state);
+	unwind_init_common(state, current);
 
 	state->fp = (unsigned long)__builtin_frame_address(1);
 	state->pc = (unsigned long)__builtin_return_address(0);
@@ -115,7 +120,7 @@ static __always_inline void unwind_init_from_caller(struct unwind_state *state)
 static inline void unwind_init_from_task(struct unwind_state *state,
 					 struct task_struct *task)
 {
-	unwind_init_common(state);
+	unwind_init_common(state, task);
 
 	state->fp = thread_saved_fp(task);
 	state->pc = thread_saved_pc(task);
@@ -128,9 +133,9 @@ static inline void unwind_init_from_task(struct unwind_state *state,
  * records (e.g. a cycle), determined based on the location and fp value of A
  * and the location (but not the fp value) of B.
  */
-static int notrace unwind_next(struct task_struct *tsk,
-			       struct unwind_state *state)
+static int notrace unwind_next(struct unwind_state *state)
 {
+	struct task_struct *tsk = state->task;
 	unsigned long fp = state->fp;
 	struct stack_info info;
 
@@ -204,8 +209,7 @@ static int notrace unwind_next(struct task_struct *tsk,
 }
 NOKPROBE_SYMBOL(unwind_next);
 
-static void notrace unwind(struct task_struct *tsk,
-			   struct unwind_state *state,
+static void notrace unwind(struct unwind_state *state,
 			   stack_trace_consume_fn consume_entry, void *cookie)
 {
 	while (1) {
@@ -213,7 +217,7 @@ static void notrace unwind(struct task_struct *tsk,
 
 		if (!consume_entry(cookie, state->pc))
 			break;
-		ret = unwind_next(tsk, state);
+		ret = unwind_next(state);
 		if (ret < 0)
 			break;
 	}
@@ -259,12 +263,15 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 {
 	struct unwind_state state;
 
-	if (regs)
+	if (regs) {
+		if (task != current)
+			return;
 		unwind_init_from_regs(&state, regs);
-	else if (task == current)
+	} else if (task == current) {
 		unwind_init_from_caller(&state);
-	else
+	} else {
 		unwind_init_from_task(&state, task);
+	}
 
-	unwind(task, &state, consume_entry, cookie);
+	unwind(&state, consume_entry, cookie);
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar to other architectures

[madvenka]

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

Change the loop in unwind()
===========================

Change the unwind loop in unwind() to:

	while (unwind_continue(state, consume_entry, cookie))
		unwind_next(state);

This is easy to understand and maintain.

New function unwind_continue()
==============================

Define a new function unwind_continue() that is used in the unwind loop
to check for conditions that terminate a stack trace.

The conditions checked are:

	- If the bottom of the stack (final frame) has been reached,
	  terminate.

	- If the consume_entry() function returns false, the caller of
	  unwind has asked to terminate the stack trace. So, terminate.

	- If unwind_next() failed for some reason (like stack corruption),
	  terminate.

Do not return an error value from unwind_next()
===============================================

We want to check for terminating conditions only in unwind_continue() from
the unwinder loop. So, do not return an error value from unwind_next().
Simply set a flag in unwind_state and check the flag in unwind_continue().

Final FP
========

Introduce a new field "final_fp" in "struct unwind_state". Initialize this
to the final frame of the stack trace:

	task_pt_regs(task)->stackframe

This is where the stacktrace must terminate if it is successful. Add an
explicit comment to that effect.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 78 ++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 8e43444d50e2..c749129aba5a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -40,6 +40,10 @@
  *               value.
  *
  * @task:        The task being unwound.
+ *
+ * @final_fp:	 Pointer to the final frame.
+ *
+ * @failed:      Unwind failed.
  */
 struct unwind_state {
 	unsigned long fp;
@@ -51,6 +55,8 @@ struct unwind_state {
 	struct llist_node *kr_cur;
 #endif
 	struct task_struct *task;
+	unsigned long final_fp;
+	bool failed;
 };
 
 static void unwind_init_common(struct unwind_state *state,
@@ -73,6 +79,10 @@ static void unwind_init_common(struct unwind_state *state,
 	bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
 	state->prev_fp = 0;
 	state->prev_type = STACK_TYPE_UNKNOWN;
+	state->failed = false;
+
+	/* Stack trace terminates here. */
+	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
 }
 
 /*
@@ -126,6 +136,25 @@ static inline void unwind_init_from_task(struct unwind_state *state,
 	state->pc = thread_saved_pc(task);
 }
 
+static bool notrace unwind_continue(struct unwind_state *state,
+				    stack_trace_consume_fn consume_entry,
+				    void *cookie)
+{
+	if (state->failed) {
+		/* PC is suspect. Cannot consume it. */
+		return false;
+	}
+
+	if (!consume_entry(cookie, state->pc)) {
+		/* Caller terminated the unwind. */
+		state->failed = true;
+		return false;
+	}
+
+	return state->fp != state->final_fp;
+}
+NOKPROBE_SYMBOL(unwind_continue);
+
 /*
  * Unwind from one frame record (A) to the next frame record (B).
  *
@@ -133,24 +162,26 @@ static inline void unwind_init_from_task(struct unwind_state *state,
  * records (e.g. a cycle), determined based on the location and fp value of A
  * and the location (but not the fp value) of B.
  */
-static int notrace unwind_next(struct unwind_state *state)
+static void notrace unwind_next(struct unwind_state *state)
 {
 	struct task_struct *tsk = state->task;
 	unsigned long fp = state->fp;
 	struct stack_info info;
 
-	/* Final frame; nothing to unwind */
-	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
-		return -ENOENT;
-
-	if (fp & 0x7)
-		return -EINVAL;
+	if (fp & 0x7) {
+		state->failed = true;
+		return;
+	}
 
-	if (!on_accessible_stack(tsk, fp, 16, &info))
-		return -EINVAL;
+	if (!on_accessible_stack(tsk, fp, 16, &info)) {
+		state->failed = true;
+		return;
+	}
 
-	if (test_bit(info.type, state->stacks_done))
-		return -EINVAL;
+	if (test_bit(info.type, state->stacks_done)) {
+		state->failed = true;
+		return;
+	}
 
 	/*
 	 * As stacks grow downward, any valid record on the same stack must be
@@ -166,8 +197,10 @@ static int notrace unwind_next(struct unwind_state *state)
 	 * stack.
 	 */
 	if (info.type == state->prev_type) {
-		if (fp <= state->prev_fp)
-			return -EINVAL;
+		if (fp <= state->prev_fp) {
+			state->failed = true;
+			return;
+		}
 	} else {
 		set_bit(state->prev_type, state->stacks_done);
 	}
@@ -195,8 +228,10 @@ static int notrace unwind_next(struct unwind_state *state)
 		 */
 		orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
 						(void *)state->fp);
-		if (WARN_ON_ONCE(state->pc == orig_pc))
-			return -EINVAL;
+		if (WARN_ON_ONCE(state->pc == orig_pc)) {
+			state->failed = true;
+			return;
+		}
 		state->pc = orig_pc;
 	}
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
@@ -204,23 +239,14 @@ static int notrace unwind_next(struct unwind_state *state)
 	if (is_kretprobe_trampoline(state->pc))
 		state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
 #endif
-
-	return 0;
 }
 NOKPROBE_SYMBOL(unwind_next);
 
 static void notrace unwind(struct unwind_state *state,
 			   stack_trace_consume_fn consume_entry, void *cookie)
 {
-	while (1) {
-		int ret;
-
-		if (!consume_entry(cookie, state->pc))
-			break;
-		ret = unwind_next(state);
-		if (ret < 0)
-			break;
-	}
+	while (unwind_continue(state, consume_entry, cookie))
+		unwind_next(state);
 }
 NOKPROBE_SYMBOL(unwind);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v15 4/6] arm64: Introduce stack trace reliability checks in the unwinder

[madvenka]

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

There are some kernel features and conditions that make a stack trace
unreliable. Callers may require the unwinder to detect these cases.
E.g., livepatch.

Introduce a new function called unwind_check_reliability() that will
detect these cases and set a flag in the stack frame. Call
unwind_check_reliability() for every frame in unwind().

Introduce the first reliability check in unwind_check_reliability() - If
a return PC is not a valid kernel text address, consider the stack
trace unreliable. It could be some generated code. Other reliability checks
will be added in the future.

Let unwind() return a boolean to indicate if the stack trace is
reliable.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index c749129aba5a..5ef2ce217324 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -44,6 +44,8 @@
  * @final_fp:	 Pointer to the final frame.
  *
  * @failed:      Unwind failed.
+ *
+ * @reliable:    Stack trace is reliable.
  */
 struct unwind_state {
 	unsigned long fp;
@@ -57,6 +59,7 @@ struct unwind_state {
 	struct task_struct *task;
 	unsigned long final_fp;
 	bool failed;
+	bool reliable;
 };
 
 static void unwind_init_common(struct unwind_state *state,
@@ -80,6 +83,7 @@ static void unwind_init_common(struct unwind_state *state,
 	state->prev_fp = 0;
 	state->prev_type = STACK_TYPE_UNKNOWN;
 	state->failed = false;
+	state->reliable = true;
 
 	/* Stack trace terminates here. */
 	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
@@ -242,11 +246,34 @@ static void notrace unwind_next(struct unwind_state *state)
 }
 NOKPROBE_SYMBOL(unwind_next);
 
-static void notrace unwind(struct unwind_state *state,
+/*
+ * Check the stack frame for conditions that make further unwinding unreliable.
+ */
+static void unwind_check_reliability(struct unwind_state *state)
+{
+	if (state->fp == state->final_fp) {
+		/* Final frame; no more unwind, no need to check reliability */
+		return;
+	}
+
+	/*
+	 * If the PC is not a known kernel text address, then we cannot
+	 * be sure that a subsequent unwind will be reliable, as we
+	 * don't know that the code follows our unwind requirements.
+	 */
+	if (!__kernel_text_address(state->pc))
+		state->reliable = false;
+}
+
+static bool notrace unwind(struct unwind_state *state,
 			   stack_trace_consume_fn consume_entry, void *cookie)
 {
-	while (unwind_continue(state, consume_entry, cookie))
+	unwind_check_reliability(state);
+	while (unwind_continue(state, consume_entry, cookie)) {
 		unwind_next(state);
+		unwind_check_reliability(state);
+	}
+	return !state->failed && state->reliable;
 }
 NOKPROBE_SYMBOL(unwind);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v15 5/6] arm64: Create a list of SYM_CODE functions, check return PC against list

[madvenka]

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

SYM_CODE functions don't follow the usual calling conventions. Check if the
return PC in a stack frame falls in any of these. If it does, consider the
stack trace unreliable.

Define a special section for unreliable functions
=================================================

Define a SYM_CODE_END() macro for arm64 that adds the function address
range to a new section called "sym_code_functions".

Linker file
===========

Include the "sym_code_functions" section under read-only data in
vmlinux.lds.S.

Initialization
==============

Define an early_initcall() to create a sym_code_functions[] array from
the linker data.

Unwinder check
==============

Add a reliability check in unwind_check_reliability() that compares a
return PC with sym_code_functions[]. If there is a match, then return
failure.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/linkage.h  | 11 +++++++
 arch/arm64/include/asm/sections.h |  1 +
 arch/arm64/kernel/stacktrace.c    | 55 +++++++++++++++++++++++++++++++
 arch/arm64/kernel/vmlinux.lds.S   | 10 ++++++
 4 files changed, 77 insertions(+)

diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index 43f8c25b3fda..d4058de4af78 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -39,4 +39,15 @@
 	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
 	bti c ;
 
+/*
+ * Record the address range of each SYM_CODE function in a struct code_range
+ * in a special section.
+ */
+#define SYM_CODE_END(name)				\
+	SYM_END(name, SYM_T_NONE)			;\
+99:	.pushsection "sym_code_functions", "aw"		;\
+	.quad	name					;\
+	.quad	99b					;\
+	.popsection
+
 #endif
diff --git a/arch/arm64/include/asm/sections.h b/arch/arm64/include/asm/sections.h
index 40971ac1303f..50cfd1083563 100644
--- a/arch/arm64/include/asm/sections.h
+++ b/arch/arm64/include/asm/sections.h
@@ -22,6 +22,7 @@ extern char __irqentry_text_start[], __irqentry_text_end[];
 extern char __mmuoff_data_start[], __mmuoff_data_end[];
 extern char __entry_tramp_text_start[], __entry_tramp_text_end[];
 extern char __relocate_new_kernel_start[], __relocate_new_kernel_end[];
+extern char __sym_code_functions_start[], __sym_code_functions_end[];
 
 static inline size_t entry_tramp_text_size(void)
 {
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 5ef2ce217324..eda8581f7dbe 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -62,6 +62,31 @@ struct unwind_state {
 	bool reliable;
 };
 
+struct code_range {
+	unsigned long	start;
+	unsigned long	end;
+};
+
+static struct code_range	*sym_code_functions;
+static int			num_sym_code_functions;
+
+int __init init_sym_code_functions(void)
+{
+	size_t size = (unsigned long)__sym_code_functions_end -
+		      (unsigned long)__sym_code_functions_start;
+
+	sym_code_functions = (struct code_range *)__sym_code_functions_start;
+	/*
+	 * Order it so that sym_code_functions is not visible before
+	 * num_sym_code_functions.
+	 */
+	smp_mb();
+	num_sym_code_functions = size / sizeof(struct code_range);
+
+	return 0;
+}
+early_initcall(init_sym_code_functions);
+
 static void unwind_init_common(struct unwind_state *state,
 			       struct task_struct *task)
 {
@@ -251,6 +276,10 @@ NOKPROBE_SYMBOL(unwind_next);
  */
 static void unwind_check_reliability(struct unwind_state *state)
 {
+	const struct code_range *range;
+	unsigned long pc;
+	int i;
+
 	if (state->fp == state->final_fp) {
 		/* Final frame; no more unwind, no need to check reliability */
 		return;
@@ -263,6 +292,32 @@ static void unwind_check_reliability(struct unwind_state *state)
 	 */
 	if (!__kernel_text_address(state->pc))
 		state->reliable = false;
+
+	/*
+	 * Check the return PC against sym_code_functions[]. If there is a
+	 * match, then the consider the stack frame unreliable.
+	 *
+	 * As SYM_CODE functions don't follow the usual calling conventions,
+	 * we assume by default that any SYM_CODE function cannot be unwound
+	 * reliably.
+	 *
+	 * Note that this includes:
+	 *
+	 * - Exception handlers and entry assembly
+	 * - Trampoline assembly (e.g., ftrace, kprobes)
+	 * - Hypervisor-related assembly
+	 * - Hibernation-related assembly
+	 * - CPU start-stop, suspend-resume assembly
+	 * - Kernel relocation assembly
+	 */
+	pc = state->pc;
+	for (i = 0; i < num_sym_code_functions; i++) {
+		range = &sym_code_functions[i];
+		if (pc >= range->start && pc < range->end) {
+			state->reliable = false;
+			return;
+		}
+	}
 }
 
 static bool notrace unwind(struct unwind_state *state,
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 2d4a8f995175..414dbc82d0a6 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -120,6 +120,14 @@ jiffies = jiffies_64;
 #define TRAMP_TEXT
 #endif
 
+#define SYM_CODE_FUNCTIONS				\
+	. = ALIGN(16);					\
+	.symcode : AT(ADDR(.symcode) - LOAD_OFFSET) {	\
+		__sym_code_functions_start = .;		\
+		KEEP(*(sym_code_functions))		\
+		__sym_code_functions_end = .;		\
+	}
+
 /*
  * The size of the PE/COFF section that covers the kernel image, which
  * runs from _stext to _edata, must be a round multiple of the PE/COFF
@@ -212,6 +220,8 @@ SECTIONS
 	swapper_pg_dir = .;
 	. += PAGE_SIZE;
 
+	SYM_CODE_FUNCTIONS
+
 	. = ALIGN(SEGMENT_ALIGN);
 	__init_begin = .;
 	__inittext_begin = .;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v15 6/6] arm64: Introduce arch_stack_walk_reliable()

[madvenka]

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

Introduce arch_stack_walk_reliable() for ARM64. This works like
arch_stack_walk() except that it returns -EINVAL if the stack trace is not
reliable.

Until all the reliability checks are in place, arch_stack_walk_reliable()
may not be used by livepatch. But it may be used by debug and test code.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index eda8581f7dbe..8016ba0e2c96 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -383,3 +383,26 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 
 	unwind(&state, consume_entry, cookie);
 }
+
+/*
+ * arch_stack_walk_reliable() may not be used for livepatch until all of
+ * the reliability checks are in place in unwind_consume(). However,
+ * debug and test code can choose to use it even if all the checks are not
+ * in place.
+ */
+noinline int notrace arch_stack_walk_reliable(
+					stack_trace_consume_fn consume_entry,
+					void *cookie,
+					struct task_struct *task)
+{
+	struct unwind_state state;
+	bool reliable;
+
+	if (task == current)
+		unwind_init_from_caller(&state);
+	else
+		unwind_init_from_task(&state, task);
+
+	reliable = unwind(&state, consume_entry, cookie);
+	return reliable ? 0 : -EINVAL;
+}
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

Sorry for the word "RFC" in the Subject line. It is my silly script.
Please ignore the word. This is not an RFC.

I will resend with the word deleted from the subject line.

Madhavan

On 6/17/22 13:02, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> I have synced this patch series to v5.19-rc2.
> I have also removed the following patch.
> 
> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> 
> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
> yet. This patch will be added in the future once Objtool is enhanced to
> provide stack validation in some form.
> 
> Split unwind_init()
> ===================
> 
> Unwind initialization has 3 cases. Accordingly, define 3 separate init
> functions as follows:
> 
> 	- unwind_init_from_regs()
> 	- unwind_init_from_current()
> 	- unwind_init_from_task()
> 
> This makes it easier to understand and add specialized code to each case
> in the future.
> 
> Copy task argument
> ==================
> 
> Copy the task argument passed to arch_stack_walk() to unwind_state so that
> it can be passed to unwind functions via unwind_state rather than as a
> separate argument. The task is a fundamental part of the unwind state.
> 
> Redefine the unwinder loop
> ==========================
> 
> Redefine the unwinder loop and make it simple and somewhat similar to other
> architectures. Define the following:
> 
> 	while (unwind_continue(&state, consume_entry, cookie))
> 		unwind_next(&state);
> 
> unwind_continue()
> 	This new function implements checks to determine whether the
> 	unwind should continue or terminate.
> 
> Reliability checks
> ==================
> 
> There are some kernel features and conditions that make a stack trace
> unreliable. Callers may require the unwinder to detect these cases.
> E.g., livepatch.
> 
> Introduce a new function called unwind_check_reliability() that will detect
> these cases and set a boolean "reliable" in the stackframe. Call
> unwind_check_reliability() for every frame.
> 
> Introduce the first reliability check in unwind_check_reliability() - If
> a return PC is not a valid kernel text address, consider the stack
> trace unreliable. It could be some generated code.
> 
> Other reliability checks will be added in the future.
> 
> Make unwind() return a boolean to indicate reliability of the stack trace.
> 
> SYM_CODE check
> ==============
> 
> This is the second reliability check implemented.
> 
> SYM_CODE functions do not follow normal calling conventions. They cannot
> be unwound reliably using the frame pointer. Collect the address ranges
> of these functions in a special section called "sym_code_functions".
> 
> In unwind_check_reliability(), check the return PC against these ranges. If
> a match is found, then mark the stack trace unreliable.
> 
> Last stack frame
> ================
> 
> If a SYM_CODE function occurs in the very last frame in the stack trace,
> then the stack trace is not considered unreliable. This is because there
> is no more unwinding to do. Examples:
> 
> 	- EL0 exception stack traces end in the top level EL0 exception
> 	  handlers.
> 
> 	- All kernel thread stack traces end in ret_from_fork().
> 
> arch_stack_walk_reliable()
> ==========================
> 
> Introduce arch_stack_walk_reliable() for ARM64. This works like
> arch_stack_walk() except that it returns an error if the stack trace is
> found to be unreliable.
> 
> Until all of the reliability checks are in place in
> unwind_check_reliability(), arch_stack_walk_reliable() may not be used by
> livepatch. But it may be used by debug and test code.
> 
> HAVE_RELIABLE_STACKTRACE
> ========================
> 
> Select this config for arm64. However, make it conditional on
> STACK_VALIDATION. When objtool is enhanced to implement stack
> validation for arm64, STACK_VALIDATION will be defined.
> 
> ---
> Changelog:
> v15:
> 	From Mark Brown:
> 
> 	- Sync this patch series to v5.19-rc2.
> 
> 	From Madhavan T. Venkataraman:
> 
> 	- Remove the following patch from the series:
> 
> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> 
> 	  as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is
> 	  not present yet. This patch will be added in the future once
> 	  Objtool is enhanced to provide stack validation in some form.
> 
> v14:
> 	From Mark Rutland, Mark Brown:
> 
> 	- Add requirements for the three helper functions that init a stack
> 	  trace.
> 
> 	From Mark Rutland:
> 
> 	- Change the comment for the task field in struct stackframe.
> 
> 	- Hard code the task to current in unwind_init_from_regs(). Add a
> 	  sanity check task == current.
> 
> 	- Rename unwind_init_from_current() to unwind_init_from_caller().
> 
> 	- Remove task argument from unwind_init_from_caller().
> 
> 	From Mark Brown:
> 
> 	- Reviewed-By: for:
> 
> 	[PATCH v13 05/11] arm64: Copy the task argument to unwind_state
> v13:
> 	From Mark Brown:
> 
> 	- Reviewed-by for the following:
> 
> 	[PATCH v12 03/10] arm64: Rename stackframe to unwind_state
> 	[PATCH v11 05/10] arm64: Copy unwind arguments to unwind_state
> 	[PATCH v11 07/10] arm64: Introduce stack trace reliability checks
> 	                  in the unwinder
> 	[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
> 	                return PC against list
> 
> 	From Mark Rutland:
> 
> 	- Reviewed-by for the following:
> 
> 	[PATCH v12 01/10] arm64: Remove NULL task check from unwind_frame()
> 	[PATCH v12 02/10] arm64: Rename unwinder functions
> 	[PATCH v12 03/10] arm64: Rename stackframe to unwind_state
> 
> 	- For each of the 3 cases of unwind initialization, have a separate
> 	  init function. Call the common init from each of these init
> 	  functions rather than call it separately.
> 
> 	- Only copy the task argument to arch_stack_walk() into
> 	  unwind state. Pass the rest of the arguments as arguments to
> 	  unwind functions.
> 
> v12:
> 	From Mark Brown:
> 
> 	- Reviewed-by for the following:
> 
> 	[PATCH v11 1/5] arm64: Call stack_backtrace() only from within
> 	                walk_stackframe()
> 	[PATCH v11 2/5] arm64: Rename unwinder functions
> 	[PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to
> 	                other architectures
> 	[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
> 	                return PC against list
> 
> 	- Add an extra patch at the end to select HAVE_RELIABLE_STACKTRACE
> 	  just as a place holder for the review. I have added it and made
> 	  it conditional on STACK_VALIDATION which has not yet been
> 	  implemented.
> 
> 	- Mark had a concern about the code for the check for the final
> 	  frame being repeated in two places. I have now added a new
> 	  field called "final_fp" in struct stackframe which I compute
> 	  once in stacktrace initialization. I have added an explicit
> 	  comment that the stacktrace must terminate at the final_fp.
> 
> 	- Place the implementation of arch_stack_walk_reliable() in a
> 	  separate patch after all the reliability checks have been
> 	  implemented.
> 
> 	From Mark Rutland:
> 
> 	- Place the removal of the NULL task check in unwind_frame() in
> 	  a separate patch.
> 
> 	- Add a task field to struct stackframe so the task pointer can be
> 	  passed around via the frame instead of as a separate argument. I have
> 	  taken this a step further by copying all of the arguments to
> 	  arch_stack_walk() into struct stackframe so that only that
> 	  struct needs to be passed to unwind functions.
> 
> 	- Rename start_backtrace() to unwind_init() instead of unwind_start().
> 
> 	- Acked-by for the following:
> 
> 	[PATCH v11 2/5] arm64: Rename unwinder functions
> 
> 	- Rename "struct stackframe" to "struct unwind_state".
> 
> 	- Define separate inline functions for initializing the starting
> 	  FP and PC from regs, or caller, or blocked task. Don't merge
> 	  unwind_init() into unwind().
> 
> v11:
> 	From Mark Rutland:
> 
> 	- Peter Zijlstra has submitted patches that make ARCH_STACKWALK
> 	  independent of STACKTRACE. Mark Rutland extracted some of the
> 	  patches from my v10 series and added his own patches and comments,
> 	  rebased it on top of Peter's changes and submitted the series.
> 	  
> 	  So, I have rebased the rest of the patches from v10 on top of
> 	  Mark Rutland's changes.
> 
> 	- Split the renaming of the unwinder functions and annotating them
> 	  with notrace and NOKPROBE_SYMBOL(). Also, there is currently no
> 	  need to annotate unwind_start() as its caller is already annotated
> 	  properly. So, I am removing the annotation patch from the series.
> 	  This can be done separately later if deemed necessary. Similarly,
> 	  I have removed the annotations from unwind_check_reliability() and
> 	  unwind_continue().
> 
> 	From Nobuta Keiya:
> 
> 	- unwind_start() should check for final frame and not mark the
> 	  final frame unreliable.
> 
> v9, v10:
> 	- v9 had a threading problem. So, I resent it as v10.
> 
> 	From me:
> 
> 	- Removed the word "RFC" from the subject line as I believe this
> 	  is mature enough to be a regular patch.
> 
> 	From Mark Brown, Mark Rutland:
> 
> 	- Split the patches into smaller, self-contained ones.
> 
> 	- Always enable STACKTRACE so that arch_stack_walk() is always
> 	  defined.
> 
> 	From Mark Rutland:
> 
> 	- Update callchain_trace() take the return value of
> 	  perf_callchain_store() into acount.
> 
> 	- Restore get_wchan() behavior to the original code.
> 
> 	- Simplify an if statement in dump_backtrace().
> 
> 	From Mark Brown:
> 
> 	- Do not abort the stack trace on the first unreliable frame.
> 
> 	
> v8:
> 	- Synced to v5.14-rc5.
> 
> 	From Mark Rutland:
> 
> 	- Make the unwinder loop similar to other architectures.
> 
> 	- Keep details to within the unwinder functions and return a simple
> 	  boolean to the caller.
> 
> 	- Convert some of the current code that contains unwinder logic to
> 	  simply use arch_stack_walk(). I have converted all of them.
> 
> 	- Do not copy sym_code_functions[]. Just place it in rodata for now.
> 
> 	- Have the main loop check for termination conditions rather than
> 	  having unwind_frame() check for them. In other words, let
> 	  unwind_frame() assume that the fp is valid.
> 
> 	- Replace the big comment for SYM_CODE functions with a shorter
> 	  comment.
> 
> 		/*
> 		 * As SYM_CODE functions don't follow the usual calling
> 		 * conventions, we assume by default that any SYM_CODE function
> 		 * cannot be unwound reliably.
> 		 *
> 		 * Note that this includes:
> 		 *
> 		 * - Exception handlers and entry assembly
> 		 * - Trampoline assembly (e.g., ftrace, kprobes)
> 		 * - Hypervisor-related assembly
> 		 * - Hibernation-related assembly
> 		 * - CPU start-stop, suspend-resume assembly
> 		 * - Kernel relocation assembly
> 		 */
> 
> v7:
> 	The Mailer screwed up the threading on this. So, I have resent this
> 	same series as version 8 with proper threading to avoid confusion.
> v6:
> 	From Mark Rutland:
> 
> 	- The per-frame reliability concept and flag are acceptable. But more
> 	  work is needed to make the per-frame checks more accurate and more
> 	  complete. E.g., some code reorg is being worked on that will help.
> 
> 	  I have now removed the frame->reliable flag and deleted the whole
> 	  concept of per-frame status. This is orthogonal to this patch series.
> 	  Instead, I have improved the unwinder to return proper return codes
> 	  so a caller can take appropriate action without needing per-frame
> 	  status.
> 
> 	- Remove the mention of PLTs and update the comment.
> 
> 	  I have replaced the comment above the call to __kernel_text_address()
> 	  with the comment suggested by Mark Rutland.
> 
> 	Other comments:
> 
> 	- Other comments on the per-frame stuff are not relevant because
> 	  that approach is not there anymore.
> 
> v5:
> 	From Keiya Nobuta:
> 	
> 	- The term blacklist(ed) is not to be used anymore. I have changed it
> 	  to unreliable. So, the function unwinder_blacklisted() has been
> 	  changed to unwinder_is_unreliable().
> 
> 	From Mark Brown:
> 
> 	- Add a comment for the "reliable" flag in struct stackframe. The
> 	  reliability attribute is not complete until all the checks are
> 	  in place. Added a comment above struct stackframe.
> 
> 	- Include some of the comments in the cover letter in the actual
> 	  code so that we can compare it with the reliable stack trace
> 	  requirements document for completeness. I have added a comment:
> 
> 	  	- above unwinder_is_unreliable() that lists the requirements
> 		  that are addressed by the function.
> 
> 		- above the __kernel_text_address() call about all the cases
> 		  the call covers.
> 
> v4:
> 	From Mark Brown:
> 
> 	- I was checking the return PC with __kernel_text_address() before
> 	  the Function Graph trace handling. Mark Brown felt that all the
> 	  reliability checks should be performed on the original return PC
> 	  once that is obtained. So, I have moved all the reliability checks
> 	  to after the Function Graph Trace handling code in the unwinder.
> 	  Basically, the unwinder should perform PC translations first (for
> 	  rhe return trampoline for Function Graph Tracing, Kretprobes, etc).
> 	  Then, the reliability checks should be applied to the resulting
> 	  PC.
> 
> 	- Mark said to improve the naming of the new functions so they don't
> 	  collide with existing ones. I have used a prefix "unwinder_" for
> 	  all the new functions.
> 
> 	From Josh Poimboeuf:
> 
> 	- In the error scenarios in the unwinder, the reliable flag in the
> 	  stack frame should be set. Implemented this.
> 
> 	- Some of the other comments are not relevant to the new code as
> 	  I have taken a different approach in the new code. That is why
> 	  I have not made those changes. E.g., Ard wanted me to add the
> 	  "const" keyword to the global section array. That array does not
> 	  exist in v4. Similarly, Mark Brown said to use ARRAY_SIZE() for
> 	  the same array in a for loop.
> 
> 	Other changes:
> 
> 	- Add a new definition for SYM_CODE_END() that adds the address
> 	  range of the function to a special section called
> 	  "sym_code_functions".
> 
> 	- Include the new section under initdata in vmlinux.lds.S.
> 
> 	- Define an early_initcall() to copy the contents of the
> 	  "sym_code_functions" section to an array by the same name.
> 
> 	- Define a function unwinder_blacklisted() that compares a return
> 	  PC against sym_code_sections[]. If there is a match, mark the
> 	  stack trace unreliable. Call this from unwind_frame().
> 
> v3:
> 	- Implemented a sym_code_ranges[] array to contains sections bounds
> 	  for text sections that contain SYM_CODE_*() functions. The unwinder
> 	  checks each return PC against the sections. If it falls in any of
> 	  the sections, the stack trace is marked unreliable.
> 
> 	- Moved SYM_CODE functions from .text and .init.text into a new
> 	  text section called ".code.text". Added this section to
> 	  vmlinux.lds.S and sym_code_ranges[].
> 
> 	- Fixed the logic in the unwinder that handles Function Graph
> 	  Tracer return trampoline.
> 
> 	- Removed all the previous code that handles:
> 		- ftrace entry code for traced function
> 		- special_functions[] array that lists individual functions
> 		- kretprobe_trampoline() special case
> 
> v2
> 	- Removed the terminating entry { 0, 0 } in special_functions[]
> 	  and replaced it with the idiom { /* sentinel */ }.
> 
> 	- Change the ftrace trampoline entry ftrace_graph_call in
> 	  special_functions[] to ftrace_call + 4 and added explanatory
> 	  comments.
> 
> 	- Unnested #ifdefs in special_functions[] for FTRACE.
> 
> v1
> 	- Define a bool field in struct stackframe. This will indicate if
> 	  a stack trace is reliable.
> 
> 	- Implement a special_functions[] array that will be populated
> 	  with special functions in which the stack trace is considered
> 	  unreliable.
> 	
> 	- Using kallsyms_lookup(), get the address ranges for the special
> 	  functions and record them.
> 
> 	- Implement an is_reliable_function(pc). This function will check
> 	  if a given return PC falls in any of the special functions. If
> 	  it does, the stack trace is unreliable.
> 
> 	- Implement check_reliability() function that will check if a
> 	  stack frame is reliable. Call is_reliable_function() from
> 	  check_reliability().
> 
> 	- Before a return PC is checked against special_funtions[], it
> 	  must be validates as a proper kernel text address. Call
> 	  __kernel_text_address() from check_reliability().
> 
> 	- Finally, call check_reliability() from unwind_frame() for
> 	  each stack frame.
> 
> 	- Add EL1 exception handlers to special_functions[].
> 
> 		el1_sync();
> 		el1_irq();
> 		el1_error();
> 		el1_sync_invalid();
> 		el1_irq_invalid();
> 		el1_fiq_invalid();
> 		el1_error_invalid();
> 
> 	- The above functions are currently defined as LOCAL symbols.
> 	  Make them global so that they can be referenced from the
> 	  unwinder code.
> 
> 	- Add FTRACE trampolines to special_functions[]:
> 
> 		ftrace_graph_call()
> 		ftrace_graph_caller()
> 		return_to_handler()
> 
> 	- Add the kretprobe trampoline to special functions[]:
> 
> 		kretprobe_trampoline()
> 
> Previous versions and discussion
> ================================
> 
> v14: https://lore.kernel.org/linux-arm-kernel/20220413140528.3815-1-madvenka@linux.microsoft.com/T/#t
> v13: https://lore.kernel.org/linux-arm-kernel/20220117145608.6781-1-madvenka@linux.microsoft.com/T/#t
> v12: https://lore.kernel.org/linux-arm-kernel/20220103165212.9303-1-madvenka@linux.microsoft.com/T/#m21e86eecb9b8f0831196568f0bf62c3b56f65bf0
> v11: https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@linux.microsoft.com/T/#t
> v10: https://lore.kernel.org/linux-arm-kernel/4b3d5552-590c-e6a0-866b-9bc51da7bebf@linux.microsoft.com/T/#t
> v9: Mailer screwed up the threading. Sent the same as v10 with proper threading.
> v8: https://lore.kernel.org/linux-arm-kernel/20210812190603.25326-1-madvenka@linux.microsoft.com/
> v7: Mailer screwed up the threading. Sent the same as v8 with proper threading.
> v6: https://lore.kernel.org/linux-arm-kernel/20210630223356.58714-1-madvenka@linux.microsoft.com/
> v5: https://lore.kernel.org/linux-arm-kernel/20210526214917.20099-1-madvenka@linux.microsoft.com/
> v4: https://lore.kernel.org/linux-arm-kernel/20210516040018.128105-1-madvenka@linux.microsoft.com/
> v3: https://lore.kernel.org/linux-arm-kernel/20210503173615.21576-1-madvenka@linux.microsoft.com/
> v2: https://lore.kernel.org/linux-arm-kernel/20210405204313.21346-1-madvenka@linux.microsoft.com/
> v1: https://lore.kernel.org/linux-arm-kernel/20210330190955.13707-1-madvenka@linux.microsoft.com/
> 
> Madhavan T. Venkataraman (6):
>   arm64: Split unwind_init()
>   arm64: Copy the task argument to unwind_state
>   arm64: Make the unwind loop in unwind() similar to other architectures
>   arm64: Introduce stack trace reliability checks in the unwinder
>   arm64: Create a list of SYM_CODE functions, check return PC against
>     list
>   arm64: Introduce arch_stack_walk_reliable()
> 
>  arch/arm64/include/asm/linkage.h  |  11 ++
>  arch/arm64/include/asm/sections.h |   1 +
>  arch/arm64/kernel/stacktrace.c    | 266 +++++++++++++++++++++++++-----
>  arch/arm64/kernel/vmlinux.lds.S   |  10 ++
>  4 files changed, 246 insertions(+), 42 deletions(-)
> 
> 
> base-commit: b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Will Deacon]

On Fri, 17 Jun 2022 13:02:13 -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> I have synced this patch series to v5.19-rc2.
> I have also removed the following patch.
> 
> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> 
> [...]

Applied first two patches to arm64 (for-next/stacktrace), thanks!

[1/6] arm64: Split unwind_init()
      https://git.kernel.org/arm64/c/a019d8a2cc82
[2/6] arm64: Copy the task argument to unwind_state
      https://git.kernel.org/arm64/c/82a592c13b0a

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/27/22 08:00, Will Deacon wrote:
> On Fri, 17 Jun 2022 13:02:13 -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> I have synced this patch series to v5.19-rc2.
>> I have also removed the following patch.
>>
>> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
>>
>> [...]
> 
> Applied first two patches to arm64 (for-next/stacktrace), thanks!
> 
> [1/6] arm64: Split unwind_init()
>       https://git.kernel.org/arm64/c/a019d8a2cc82
> [2/6] arm64: Copy the task argument to unwind_state
>       https://git.kernel.org/arm64/c/82a592c13b0a
> 
> Cheers,

Great!

Thanks!

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[madvenka]

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

I have synced this patch series to v5.19-rc2.
I have also removed the following patch.

	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE

as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
yet. This patch will be added in the future once Objtool is enhanced to
provide stack validation in some form.

Split unwind_init()
===================

Unwind initialization has 3 cases. Accordingly, define 3 separate init
functions as follows:

	- unwind_init_from_regs()
	- unwind_init_from_current()
	- unwind_init_from_task()

This makes it easier to understand and add specialized code to each case
in the future.

Copy task argument
==================

Copy the task argument passed to arch_stack_walk() to unwind_state so that
it can be passed to unwind functions via unwind_state rather than as a
separate argument. The task is a fundamental part of the unwind state.

Redefine the unwinder loop
==========================

Redefine the unwinder loop and make it simple and somewhat similar to other
architectures. Define the following:

	while (unwind_continue(&state, consume_entry, cookie))
		unwind_next(&state);

unwind_continue()
	This new function implements checks to determine whether the
	unwind should continue or terminate.

Reliability checks
==================

There are some kernel features and conditions that make a stack trace
unreliable. Callers may require the unwinder to detect these cases.
E.g., livepatch.

Introduce a new function called unwind_check_reliability() that will detect
these cases and set a boolean "reliable" in the stackframe. Call
unwind_check_reliability() for every frame.

Introduce the first reliability check in unwind_check_reliability() - If
a return PC is not a valid kernel text address, consider the stack
trace unreliable. It could be some generated code.

Other reliability checks will be added in the future.

Make unwind() return a boolean to indicate reliability of the stack trace.

SYM_CODE check
==============

This is the second reliability check implemented.

SYM_CODE functions do not follow normal calling conventions. They cannot
be unwound reliably using the frame pointer. Collect the address ranges
of these functions in a special section called "sym_code_functions".

In unwind_check_reliability(), check the return PC against these ranges. If
a match is found, then mark the stack trace unreliable.

Last stack frame
================

If a SYM_CODE function occurs in the very last frame in the stack trace,
then the stack trace is not considered unreliable. This is because there
is no more unwinding to do. Examples:

	- EL0 exception stack traces end in the top level EL0 exception
	  handlers.

	- All kernel thread stack traces end in ret_from_fork().

arch_stack_walk_reliable()
==========================

Introduce arch_stack_walk_reliable() for ARM64. This works like
arch_stack_walk() except that it returns an error if the stack trace is
found to be unreliable.

Until all of the reliability checks are in place in
unwind_check_reliability(), arch_stack_walk_reliable() may not be used by
livepatch. But it may be used by debug and test code.

HAVE_RELIABLE_STACKTRACE
========================

Select this config for arm64. However, make it conditional on
STACK_VALIDATION. When objtool is enhanced to implement stack
validation for arm64, STACK_VALIDATION will be defined.

---
Changelog:
v15:
	From Mark Brown:

	- Sync this patch series to v5.19-rc2.

	- Reviewed-By: for:

	[PATCH v14 1/7] arm64: Split unwind_init()
	[PATCH v14 2/7] arm64: Copy the task argument to unwind_state

	From Madhavan T. Venkataraman:

	- Remove the following patch from the series:

	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE

	  as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is
	  not present yet. This patch will be added in the future once
	  Objtool is enhanced to provide stack validation in some form.

v14:
	From Mark Rutland, Mark Brown:

	- Add requirements for the three helper functions that init a stack
	  trace.

	From Mark Rutland:

	- Change the comment for the task field in struct stackframe.

	- Hard code the task to current in unwind_init_from_regs(). Add a
	  sanity check task == current.

	- Rename unwind_init_from_current() to unwind_init_from_caller().

	- Remove task argument from unwind_init_from_caller().

	From Mark Brown:

	- Reviewed-By: for:

	[PATCH v13 05/11] arm64: Copy the task argument to unwind_state
v13:
	From Mark Brown:

	- Reviewed-by for the following:

	[PATCH v12 03/10] arm64: Rename stackframe to unwind_state
	[PATCH v11 05/10] arm64: Copy unwind arguments to unwind_state
	[PATCH v11 07/10] arm64: Introduce stack trace reliability checks
	                  in the unwinder
	[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
	                return PC against list

	From Mark Rutland:

	- Reviewed-by for the following:

	[PATCH v12 01/10] arm64: Remove NULL task check from unwind_frame()
	[PATCH v12 02/10] arm64: Rename unwinder functions
	[PATCH v12 03/10] arm64: Rename stackframe to unwind_state

	- For each of the 3 cases of unwind initialization, have a separate
	  init function. Call the common init from each of these init
	  functions rather than call it separately.

	- Only copy the task argument to arch_stack_walk() into
	  unwind state. Pass the rest of the arguments as arguments to
	  unwind functions.

v12:
	From Mark Brown:

	- Reviewed-by for the following:

	[PATCH v11 1/5] arm64: Call stack_backtrace() only from within
	                walk_stackframe()
	[PATCH v11 2/5] arm64: Rename unwinder functions
	[PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to
	                other architectures
	[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
	                return PC against list

	- Add an extra patch at the end to select HAVE_RELIABLE_STACKTRACE
	  just as a place holder for the review. I have added it and made
	  it conditional on STACK_VALIDATION which has not yet been
	  implemented.

	- Mark had a concern about the code for the check for the final
	  frame being repeated in two places. I have now added a new
	  field called "final_fp" in struct stackframe which I compute
	  once in stacktrace initialization. I have added an explicit
	  comment that the stacktrace must terminate at the final_fp.

	- Place the implementation of arch_stack_walk_reliable() in a
	  separate patch after all the reliability checks have been
	  implemented.

	From Mark Rutland:

	- Place the removal of the NULL task check in unwind_frame() in
	  a separate patch.

	- Add a task field to struct stackframe so the task pointer can be
	  passed around via the frame instead of as a separate argument. I have
	  taken this a step further by copying all of the arguments to
	  arch_stack_walk() into struct stackframe so that only that
	  struct needs to be passed to unwind functions.

	- Rename start_backtrace() to unwind_init() instead of unwind_start().

	- Acked-by for the following:

	[PATCH v11 2/5] arm64: Rename unwinder functions

	- Rename "struct stackframe" to "struct unwind_state".

	- Define separate inline functions for initializing the starting
	  FP and PC from regs, or caller, or blocked task. Don't merge
	  unwind_init() into unwind().

v11:
	From Mark Rutland:

	- Peter Zijlstra has submitted patches that make ARCH_STACKWALK
	  independent of STACKTRACE. Mark Rutland extracted some of the
	  patches from my v10 series and added his own patches and comments,
	  rebased it on top of Peter's changes and submitted the series.
	  
	  So, I have rebased the rest of the patches from v10 on top of
	  Mark Rutland's changes.

	- Split the renaming of the unwinder functions and annotating them
	  with notrace and NOKPROBE_SYMBOL(). Also, there is currently no
	  need to annotate unwind_start() as its caller is already annotated
	  properly. So, I am removing the annotation patch from the series.
	  This can be done separately later if deemed necessary. Similarly,
	  I have removed the annotations from unwind_check_reliability() and
	  unwind_continue().

	From Nobuta Keiya:

	- unwind_start() should check for final frame and not mark the
	  final frame unreliable.

v9, v10:
	- v9 had a threading problem. So, I resent it as v10.

	From me:

	- Removed the word "RFC" from the subject line as I believe this
	  is mature enough to be a regular patch.

	From Mark Brown, Mark Rutland:

	- Split the patches into smaller, self-contained ones.

	- Always enable STACKTRACE so that arch_stack_walk() is always
	  defined.

	From Mark Rutland:

	- Update callchain_trace() take the return value of
	  perf_callchain_store() into acount.

	- Restore get_wchan() behavior to the original code.

	- Simplify an if statement in dump_backtrace().

	From Mark Brown:

	- Do not abort the stack trace on the first unreliable frame.

	
v8:
	- Synced to v5.14-rc5.

	From Mark Rutland:

	- Make the unwinder loop similar to other architectures.

	- Keep details to within the unwinder functions and return a simple
	  boolean to the caller.

	- Convert some of the current code that contains unwinder logic to
	  simply use arch_stack_walk(). I have converted all of them.

	- Do not copy sym_code_functions[]. Just place it in rodata for now.

	- Have the main loop check for termination conditions rather than
	  having unwind_frame() check for them. In other words, let
	  unwind_frame() assume that the fp is valid.

	- Replace the big comment for SYM_CODE functions with a shorter
	  comment.

		/*
		 * As SYM_CODE functions don't follow the usual calling
		 * conventions, we assume by default that any SYM_CODE function
		 * cannot be unwound reliably.
		 *
		 * Note that this includes:
		 *
		 * - Exception handlers and entry assembly
		 * - Trampoline assembly (e.g., ftrace, kprobes)
		 * - Hypervisor-related assembly
		 * - Hibernation-related assembly
		 * - CPU start-stop, suspend-resume assembly
		 * - Kernel relocation assembly
		 */

v7:
	The Mailer screwed up the threading on this. So, I have resent this
	same series as version 8 with proper threading to avoid confusion.
v6:
	From Mark Rutland:

	- The per-frame reliability concept and flag are acceptable. But more
	  work is needed to make the per-frame checks more accurate and more
	  complete. E.g., some code reorg is being worked on that will help.

	  I have now removed the frame->reliable flag and deleted the whole
	  concept of per-frame status. This is orthogonal to this patch series.
	  Instead, I have improved the unwinder to return proper return codes
	  so a caller can take appropriate action without needing per-frame
	  status.

	- Remove the mention of PLTs and update the comment.

	  I have replaced the comment above the call to __kernel_text_address()
	  with the comment suggested by Mark Rutland.

	Other comments:

	- Other comments on the per-frame stuff are not relevant because
	  that approach is not there anymore.

v5:
	From Keiya Nobuta:
	
	- The term blacklist(ed) is not to be used anymore. I have changed it
	  to unreliable. So, the function unwinder_blacklisted() has been
	  changed to unwinder_is_unreliable().

	From Mark Brown:

	- Add a comment for the "reliable" flag in struct stackframe. The
	  reliability attribute is not complete until all the checks are
	  in place. Added a comment above struct stackframe.

	- Include some of the comments in the cover letter in the actual
	  code so that we can compare it with the reliable stack trace
	  requirements document for completeness. I have added a comment:

	  	- above unwinder_is_unreliable() that lists the requirements
		  that are addressed by the function.

		- above the __kernel_text_address() call about all the cases
		  the call covers.

v4:
	From Mark Brown:

	- I was checking the return PC with __kernel_text_address() before
	  the Function Graph trace handling. Mark Brown felt that all the
	  reliability checks should be performed on the original return PC
	  once that is obtained. So, I have moved all the reliability checks
	  to after the Function Graph Trace handling code in the unwinder.
	  Basically, the unwinder should perform PC translations first (for
	  rhe return trampoline for Function Graph Tracing, Kretprobes, etc).
	  Then, the reliability checks should be applied to the resulting
	  PC.

	- Mark said to improve the naming of the new functions so they don't
	  collide with existing ones. I have used a prefix "unwinder_" for
	  all the new functions.

	From Josh Poimboeuf:

	- In the error scenarios in the unwinder, the reliable flag in the
	  stack frame should be set. Implemented this.

	- Some of the other comments are not relevant to the new code as
	  I have taken a different approach in the new code. That is why
	  I have not made those changes. E.g., Ard wanted me to add the
	  "const" keyword to the global section array. That array does not
	  exist in v4. Similarly, Mark Brown said to use ARRAY_SIZE() for
	  the same array in a for loop.

	Other changes:

	- Add a new definition for SYM_CODE_END() that adds the address
	  range of the function to a special section called
	  "sym_code_functions".

	- Include the new section under initdata in vmlinux.lds.S.

	- Define an early_initcall() to copy the contents of the
	  "sym_code_functions" section to an array by the same name.

	- Define a function unwinder_blacklisted() that compares a return
	  PC against sym_code_sections[]. If there is a match, mark the
	  stack trace unreliable. Call this from unwind_frame().

v3:
	- Implemented a sym_code_ranges[] array to contains sections bounds
	  for text sections that contain SYM_CODE_*() functions. The unwinder
	  checks each return PC against the sections. If it falls in any of
	  the sections, the stack trace is marked unreliable.

	- Moved SYM_CODE functions from .text and .init.text into a new
	  text section called ".code.text". Added this section to
	  vmlinux.lds.S and sym_code_ranges[].

	- Fixed the logic in the unwinder that handles Function Graph
	  Tracer return trampoline.

	- Removed all the previous code that handles:
		- ftrace entry code for traced function
		- special_functions[] array that lists individual functions
		- kretprobe_trampoline() special case

v2
	- Removed the terminating entry { 0, 0 } in special_functions[]
	  and replaced it with the idiom { /* sentinel */ }.

	- Change the ftrace trampoline entry ftrace_graph_call in
	  special_functions[] to ftrace_call + 4 and added explanatory
	  comments.

	- Unnested #ifdefs in special_functions[] for FTRACE.

v1
	- Define a bool field in struct stackframe. This will indicate if
	  a stack trace is reliable.

	- Implement a special_functions[] array that will be populated
	  with special functions in which the stack trace is considered
	  unreliable.
	
	- Using kallsyms_lookup(), get the address ranges for the special
	  functions and record them.

	- Implement an is_reliable_function(pc). This function will check
	  if a given return PC falls in any of the special functions. If
	  it does, the stack trace is unreliable.

	- Implement check_reliability() function that will check if a
	  stack frame is reliable. Call is_reliable_function() from
	  check_reliability().

	- Before a return PC is checked against special_funtions[], it
	  must be validates as a proper kernel text address. Call
	  __kernel_text_address() from check_reliability().

	- Finally, call check_reliability() from unwind_frame() for
	  each stack frame.

	- Add EL1 exception handlers to special_functions[].

		el1_sync();
		el1_irq();
		el1_error();
		el1_sync_invalid();
		el1_irq_invalid();
		el1_fiq_invalid();
		el1_error_invalid();

	- The above functions are currently defined as LOCAL symbols.
	  Make them global so that they can be referenced from the
	  unwinder code.

	- Add FTRACE trampolines to special_functions[]:

		ftrace_graph_call()
		ftrace_graph_caller()
		return_to_handler()

	- Add the kretprobe trampoline to special functions[]:

		kretprobe_trampoline()

Previous versions and discussion
================================

v14: https://lore.kernel.org/linux-arm-kernel/20220413140528.3815-1-madvenka@linux.microsoft.com/T/#t
v13: https://lore.kernel.org/linux-arm-kernel/20220117145608.6781-1-madvenka@linux.microsoft.com/T/#t
v12: https://lore.kernel.org/linux-arm-kernel/20220103165212.9303-1-madvenka@linux.microsoft.com/T/#m21e86eecb9b8f0831196568f0bf62c3b56f65bf0
v11: https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@linux.microsoft.com/T/#t
v10: https://lore.kernel.org/linux-arm-kernel/4b3d5552-590c-e6a0-866b-9bc51da7bebf@linux.microsoft.com/T/#t
v9: Mailer screwed up the threading. Sent the same as v10 with proper threading.
v8: https://lore.kernel.org/linux-arm-kernel/20210812190603.25326-1-madvenka@linux.microsoft.com/
v7: Mailer screwed up the threading. Sent the same as v8 with proper threading.
v6: https://lore.kernel.org/linux-arm-kernel/20210630223356.58714-1-madvenka@linux.microsoft.com/
v5: https://lore.kernel.org/linux-arm-kernel/20210526214917.20099-1-madvenka@linux.microsoft.com/
v4: https://lore.kernel.org/linux-arm-kernel/20210516040018.128105-1-madvenka@linux.microsoft.com/
v3: https://lore.kernel.org/linux-arm-kernel/20210503173615.21576-1-madvenka@linux.microsoft.com/
v2: https://lore.kernel.org/linux-arm-kernel/20210405204313.21346-1-madvenka@linux.microsoft.com/
v1: https://lore.kernel.org/linux-arm-kernel/20210330190955.13707-1-madvenka@linux.microsoft.com/

Madhavan T. Venkataraman (6):
  arm64: Split unwind_init()
  arm64: Copy the task argument to unwind_state
  arm64: Make the unwind loop in unwind() similar to other architectures
  arm64: Introduce stack trace reliability checks in the unwinder
  arm64: Create a list of SYM_CODE functions, check return PC against
    list
  arm64: Introduce arch_stack_walk_reliable()

 arch/arm64/include/asm/linkage.h  |  11 ++
 arch/arm64/include/asm/sections.h |   1 +
 arch/arm64/kernel/stacktrace.c    | 266 +++++++++++++++++++++++++-----
 arch/arm64/kernel/vmlinux.lds.S   |  10 ++
 4 files changed, 246 insertions(+), 42 deletions(-)


base-commit: b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 1/6] arm64: Split unwind_init()

[madvenka]

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

unwind_init() is currently a single function that initializes all of the
unwind state. Split it into the following functions and call them
appropriately:

	- unwind_init_from_regs() - initialize from regs passed by caller.

	- unwind_init_from_caller() - initialize for the current task
	  from the caller of arch_stack_walk().

	- unwind_init_from_task() - initialize from the saved state of a
	  task other than the current task. In this case, the other
	  task must not be running.

This is done for two reasons:

	- the different ways of initializing are clear

	- specialized code can be added to each initializer in the future.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 66 ++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 0467cb79f080..e44f93ff25f0 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -50,11 +50,8 @@ struct unwind_state {
 #endif
 };
 
-static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
-				unsigned long pc)
+static void unwind_init_common(struct unwind_state *state)
 {
-	state->fp = fp;
-	state->pc = pc;
 #ifdef CONFIG_KRETPROBES
 	state->kr_cur = NULL;
 #endif
@@ -72,7 +69,57 @@ static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
 	state->prev_fp = 0;
 	state->prev_type = STACK_TYPE_UNKNOWN;
 }
-NOKPROBE_SYMBOL(unwind_init);
+
+/*
+ * Start an unwind from a pt_regs.
+ *
+ * The unwind will begin at the PC within the regs.
+ *
+ * The regs must be on a stack currently owned by the calling task.
+ */
+static inline void unwind_init_from_regs(struct unwind_state *state,
+					 struct pt_regs *regs)
+{
+	unwind_init_common(state);
+
+	state->fp = regs->regs[29];
+	state->pc = regs->pc;
+}
+
+/*
+ * Start an unwind from a caller.
+ *
+ * The unwind will begin at the caller of whichever function this is inlined
+ * into.
+ *
+ * The function which invokes this must be noinline.
+ */
+static __always_inline void unwind_init_from_caller(struct unwind_state *state)
+{
+	unwind_init_common(state);
+
+	state->fp = (unsigned long)__builtin_frame_address(1);
+	state->pc = (unsigned long)__builtin_return_address(0);
+}
+
+/*
+ * Start an unwind from a blocked task.
+ *
+ * The unwind will begin at the blocked tasks saved PC (i.e. the caller of
+ * cpu_switch_to()).
+ *
+ * The caller should ensure the task is blocked in cpu_switch_to() for the
+ * duration of the unwind, or the unwind will be bogus. It is never valid to
+ * call this for the current task.
+ */
+static inline void unwind_init_from_task(struct unwind_state *state,
+					 struct task_struct *task)
+{
+	unwind_init_common(state);
+
+	state->fp = thread_saved_fp(task);
+	state->pc = thread_saved_pc(task);
+}
 
 /*
  * Unwind from one frame record (A) to the next frame record (B).
@@ -213,14 +260,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 	struct unwind_state state;
 
 	if (regs)
-		unwind_init(&state, regs->regs[29], regs->pc);
+		unwind_init_from_regs(&state, regs);
 	else if (task == current)
-		unwind_init(&state,
-				(unsigned long)__builtin_frame_address(1),
-				(unsigned long)__builtin_return_address(0));
+		unwind_init_from_caller(&state);
 	else
-		unwind_init(&state, thread_saved_fp(task),
-				thread_saved_pc(task));
+		unwind_init_from_task(&state, task);
 
 	unwind(task, &state, consume_entry, cookie);
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 1/6] arm64: Split unwind_init()

[Mark Rutland]

On Fri, Jun 17, 2022 at 04:07:12PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> unwind_init() is currently a single function that initializes all of the
> unwind state. Split it into the following functions and call them
> appropriately:
> 
> 	- unwind_init_from_regs() - initialize from regs passed by caller.
> 
> 	- unwind_init_from_caller() - initialize for the current task
> 	  from the caller of arch_stack_walk().
> 
> 	- unwind_init_from_task() - initialize from the saved state of a
> 	  task other than the current task. In this case, the other
> 	  task must not be running.
> 
> This is done for two reasons:
> 
> 	- the different ways of initializing are clear
> 
> 	- specialized code can be added to each initializer in the future.
> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>

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

Mark.

> ---
>  arch/arm64/kernel/stacktrace.c | 66 ++++++++++++++++++++++++++++------
>  1 file changed, 55 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 0467cb79f080..e44f93ff25f0 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -50,11 +50,8 @@ struct unwind_state {
>  #endif
>  };
>  
> -static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
> -				unsigned long pc)
> +static void unwind_init_common(struct unwind_state *state)
>  {
> -	state->fp = fp;
> -	state->pc = pc;
>  #ifdef CONFIG_KRETPROBES
>  	state->kr_cur = NULL;
>  #endif
> @@ -72,7 +69,57 @@ static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
>  	state->prev_fp = 0;
>  	state->prev_type = STACK_TYPE_UNKNOWN;
>  }
> -NOKPROBE_SYMBOL(unwind_init);
> +
> +/*
> + * Start an unwind from a pt_regs.
> + *
> + * The unwind will begin at the PC within the regs.
> + *
> + * The regs must be on a stack currently owned by the calling task.
> + */
> +static inline void unwind_init_from_regs(struct unwind_state *state,
> +					 struct pt_regs *regs)
> +{
> +	unwind_init_common(state);
> +
> +	state->fp = regs->regs[29];
> +	state->pc = regs->pc;
> +}
> +
> +/*
> + * Start an unwind from a caller.
> + *
> + * The unwind will begin at the caller of whichever function this is inlined
> + * into.
> + *
> + * The function which invokes this must be noinline.
> + */
> +static __always_inline void unwind_init_from_caller(struct unwind_state *state)
> +{
> +	unwind_init_common(state);
> +
> +	state->fp = (unsigned long)__builtin_frame_address(1);
> +	state->pc = (unsigned long)__builtin_return_address(0);
> +}
> +
> +/*
> + * Start an unwind from a blocked task.
> + *
> + * The unwind will begin at the blocked tasks saved PC (i.e. the caller of
> + * cpu_switch_to()).
> + *
> + * The caller should ensure the task is blocked in cpu_switch_to() for the
> + * duration of the unwind, or the unwind will be bogus. It is never valid to
> + * call this for the current task.
> + */
> +static inline void unwind_init_from_task(struct unwind_state *state,
> +					 struct task_struct *task)
> +{
> +	unwind_init_common(state);
> +
> +	state->fp = thread_saved_fp(task);
> +	state->pc = thread_saved_pc(task);
> +}
>  
>  /*
>   * Unwind from one frame record (A) to the next frame record (B).
> @@ -213,14 +260,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  	struct unwind_state state;
>  
>  	if (regs)
> -		unwind_init(&state, regs->regs[29], regs->pc);
> +		unwind_init_from_regs(&state, regs);
>  	else if (task == current)
> -		unwind_init(&state,
> -				(unsigned long)__builtin_frame_address(1),
> -				(unsigned long)__builtin_return_address(0));
> +		unwind_init_from_caller(&state);
>  	else
> -		unwind_init(&state, thread_saved_fp(task),
> -				thread_saved_pc(task));
> +		unwind_init_from_task(&state, task);
>  
>  	unwind(task, &state, consume_entry, cookie);
>  }
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 2/6] arm64: Copy the task argument to unwind_state

[madvenka]

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

Copy the task argument passed to arch_stack_walk() to unwind_state so that
it can be passed to unwind functions via unwind_state rather than as a
separate argument. The task is a fundamental part of the unwind state.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e44f93ff25f0..8e43444d50e2 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -38,6 +38,8 @@
  * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
  *               associated with the most recently encountered replacement lr
  *               value.
+ *
+ * @task:        The task being unwound.
  */
 struct unwind_state {
 	unsigned long fp;
@@ -48,10 +50,13 @@ struct unwind_state {
 #ifdef CONFIG_KRETPROBES
 	struct llist_node *kr_cur;
 #endif
+	struct task_struct *task;
 };
 
-static void unwind_init_common(struct unwind_state *state)
+static void unwind_init_common(struct unwind_state *state,
+			       struct task_struct *task)
 {
+	state->task = task;
 #ifdef CONFIG_KRETPROBES
 	state->kr_cur = NULL;
 #endif
@@ -80,7 +85,7 @@ static void unwind_init_common(struct unwind_state *state)
 static inline void unwind_init_from_regs(struct unwind_state *state,
 					 struct pt_regs *regs)
 {
-	unwind_init_common(state);
+	unwind_init_common(state, current);
 
 	state->fp = regs->regs[29];
 	state->pc = regs->pc;
@@ -96,7 +101,7 @@ static inline void unwind_init_from_regs(struct unwind_state *state,
  */
 static __always_inline void unwind_init_from_caller(struct unwind_state *state)
 {
-	unwind_init_common(state);
+	unwind_init_common(state, current);
 
 	state->fp = (unsigned long)__builtin_frame_address(1);
 	state->pc = (unsigned long)__builtin_return_address(0);
@@ -115,7 +120,7 @@ static __always_inline void unwind_init_from_caller(struct unwind_state *state)
 static inline void unwind_init_from_task(struct unwind_state *state,
 					 struct task_struct *task)
 {
-	unwind_init_common(state);
+	unwind_init_common(state, task);
 
 	state->fp = thread_saved_fp(task);
 	state->pc = thread_saved_pc(task);
@@ -128,9 +133,9 @@ static inline void unwind_init_from_task(struct unwind_state *state,
  * records (e.g. a cycle), determined based on the location and fp value of A
  * and the location (but not the fp value) of B.
  */
-static int notrace unwind_next(struct task_struct *tsk,
-			       struct unwind_state *state)
+static int notrace unwind_next(struct unwind_state *state)
 {
+	struct task_struct *tsk = state->task;
 	unsigned long fp = state->fp;
 	struct stack_info info;
 
@@ -204,8 +209,7 @@ static int notrace unwind_next(struct task_struct *tsk,
 }
 NOKPROBE_SYMBOL(unwind_next);
 
-static void notrace unwind(struct task_struct *tsk,
-			   struct unwind_state *state,
+static void notrace unwind(struct unwind_state *state,
 			   stack_trace_consume_fn consume_entry, void *cookie)
 {
 	while (1) {
@@ -213,7 +217,7 @@ static void notrace unwind(struct task_struct *tsk,
 
 		if (!consume_entry(cookie, state->pc))
 			break;
-		ret = unwind_next(tsk, state);
+		ret = unwind_next(state);
 		if (ret < 0)
 			break;
 	}
@@ -259,12 +263,15 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 {
 	struct unwind_state state;
 
-	if (regs)
+	if (regs) {
+		if (task != current)
+			return;
 		unwind_init_from_regs(&state, regs);
-	else if (task == current)
+	} else if (task == current) {
 		unwind_init_from_caller(&state);
-	else
+	} else {
 		unwind_init_from_task(&state, task);
+	}
 
-	unwind(task, &state, consume_entry, cookie);
+	unwind(&state, consume_entry, cookie);
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 2/6] arm64: Copy the task argument to unwind_state

[Mark Rutland]

On Fri, Jun 17, 2022 at 04:07:13PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> Copy the task argument passed to arch_stack_walk() to unwind_state so that
> it can be passed to unwind functions via unwind_state rather than as a
> separate argument. The task is a fundamental part of the unwind state.
> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>

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

Mark.

> ---
>  arch/arm64/kernel/stacktrace.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index e44f93ff25f0..8e43444d50e2 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -38,6 +38,8 @@
>   * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
>   *               associated with the most recently encountered replacement lr
>   *               value.
> + *
> + * @task:        The task being unwound.
>   */
>  struct unwind_state {
>  	unsigned long fp;
> @@ -48,10 +50,13 @@ struct unwind_state {
>  #ifdef CONFIG_KRETPROBES
>  	struct llist_node *kr_cur;
>  #endif
> +	struct task_struct *task;
>  };
>  
> -static void unwind_init_common(struct unwind_state *state)
> +static void unwind_init_common(struct unwind_state *state,
> +			       struct task_struct *task)
>  {
> +	state->task = task;
>  #ifdef CONFIG_KRETPROBES
>  	state->kr_cur = NULL;
>  #endif
> @@ -80,7 +85,7 @@ static void unwind_init_common(struct unwind_state *state)
>  static inline void unwind_init_from_regs(struct unwind_state *state,
>  					 struct pt_regs *regs)
>  {
> -	unwind_init_common(state);
> +	unwind_init_common(state, current);
>  
>  	state->fp = regs->regs[29];
>  	state->pc = regs->pc;
> @@ -96,7 +101,7 @@ static inline void unwind_init_from_regs(struct unwind_state *state,
>   */
>  static __always_inline void unwind_init_from_caller(struct unwind_state *state)
>  {
> -	unwind_init_common(state);
> +	unwind_init_common(state, current);
>  
>  	state->fp = (unsigned long)__builtin_frame_address(1);
>  	state->pc = (unsigned long)__builtin_return_address(0);
> @@ -115,7 +120,7 @@ static __always_inline void unwind_init_from_caller(struct unwind_state *state)
>  static inline void unwind_init_from_task(struct unwind_state *state,
>  					 struct task_struct *task)
>  {
> -	unwind_init_common(state);
> +	unwind_init_common(state, task);
>  
>  	state->fp = thread_saved_fp(task);
>  	state->pc = thread_saved_pc(task);
> @@ -128,9 +133,9 @@ static inline void unwind_init_from_task(struct unwind_state *state,
>   * records (e.g. a cycle), determined based on the location and fp value of A
>   * and the location (but not the fp value) of B.
>   */
> -static int notrace unwind_next(struct task_struct *tsk,
> -			       struct unwind_state *state)
> +static int notrace unwind_next(struct unwind_state *state)
>  {
> +	struct task_struct *tsk = state->task;
>  	unsigned long fp = state->fp;
>  	struct stack_info info;
>  
> @@ -204,8 +209,7 @@ static int notrace unwind_next(struct task_struct *tsk,
>  }
>  NOKPROBE_SYMBOL(unwind_next);
>  
> -static void notrace unwind(struct task_struct *tsk,
> -			   struct unwind_state *state,
> +static void notrace unwind(struct unwind_state *state,
>  			   stack_trace_consume_fn consume_entry, void *cookie)
>  {
>  	while (1) {
> @@ -213,7 +217,7 @@ static void notrace unwind(struct task_struct *tsk,
>  
>  		if (!consume_entry(cookie, state->pc))
>  			break;
> -		ret = unwind_next(tsk, state);
> +		ret = unwind_next(state);
>  		if (ret < 0)
>  			break;
>  	}
> @@ -259,12 +263,15 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  {
>  	struct unwind_state state;
>  
> -	if (regs)
> +	if (regs) {
> +		if (task != current)
> +			return;
>  		unwind_init_from_regs(&state, regs);
> -	else if (task == current)
> +	} else if (task == current) {
>  		unwind_init_from_caller(&state);
> -	else
> +	} else {
>  		unwind_init_from_task(&state, task);
> +	}
>  
> -	unwind(task, &state, consume_entry, cookie);
> +	unwind(&state, consume_entry, cookie);
>  }
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar to other architectures

[madvenka]

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

Change the loop in unwind()
===========================

Change the unwind loop in unwind() to:

	while (unwind_continue(state, consume_entry, cookie))
		unwind_next(state);

This is easy to understand and maintain.

New function unwind_continue()
==============================

Define a new function unwind_continue() that is used in the unwind loop
to check for conditions that terminate a stack trace.

The conditions checked are:

	- If the bottom of the stack (final frame) has been reached,
	  terminate.

	- If the consume_entry() function returns false, the caller of
	  unwind has asked to terminate the stack trace. So, terminate.

	- If unwind_next() failed for some reason (like stack corruption),
	  terminate.

Do not return an error value from unwind_next()
===============================================

We want to check for terminating conditions only in unwind_continue() from
the unwinder loop. So, do not return an error value from unwind_next().
Simply set a flag in unwind_state and check the flag in unwind_continue().

Final FP
========

Introduce a new field "final_fp" in "struct unwind_state". Initialize this
to the final frame of the stack trace:

	task_pt_regs(task)->stackframe

This is where the stacktrace must terminate if it is successful. Add an
explicit comment to that effect.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 78 ++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 8e43444d50e2..c749129aba5a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -40,6 +40,10 @@
  *               value.
  *
  * @task:        The task being unwound.
+ *
+ * @final_fp:	 Pointer to the final frame.
+ *
+ * @failed:      Unwind failed.
  */
 struct unwind_state {
 	unsigned long fp;
@@ -51,6 +55,8 @@ struct unwind_state {
 	struct llist_node *kr_cur;
 #endif
 	struct task_struct *task;
+	unsigned long final_fp;
+	bool failed;
 };
 
 static void unwind_init_common(struct unwind_state *state,
@@ -73,6 +79,10 @@ static void unwind_init_common(struct unwind_state *state,
 	bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
 	state->prev_fp = 0;
 	state->prev_type = STACK_TYPE_UNKNOWN;
+	state->failed = false;
+
+	/* Stack trace terminates here. */
+	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
 }
 
 /*
@@ -126,6 +136,25 @@ static inline void unwind_init_from_task(struct unwind_state *state,
 	state->pc = thread_saved_pc(task);
 }
 
+static bool notrace unwind_continue(struct unwind_state *state,
+				    stack_trace_consume_fn consume_entry,
+				    void *cookie)
+{
+	if (state->failed) {
+		/* PC is suspect. Cannot consume it. */
+		return false;
+	}
+
+	if (!consume_entry(cookie, state->pc)) {
+		/* Caller terminated the unwind. */
+		state->failed = true;
+		return false;
+	}
+
+	return state->fp != state->final_fp;
+}
+NOKPROBE_SYMBOL(unwind_continue);
+
 /*
  * Unwind from one frame record (A) to the next frame record (B).
  *
@@ -133,24 +162,26 @@ static inline void unwind_init_from_task(struct unwind_state *state,
  * records (e.g. a cycle), determined based on the location and fp value of A
  * and the location (but not the fp value) of B.
  */
-static int notrace unwind_next(struct unwind_state *state)
+static void notrace unwind_next(struct unwind_state *state)
 {
 	struct task_struct *tsk = state->task;
 	unsigned long fp = state->fp;
 	struct stack_info info;
 
-	/* Final frame; nothing to unwind */
-	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
-		return -ENOENT;
-
-	if (fp & 0x7)
-		return -EINVAL;
+	if (fp & 0x7) {
+		state->failed = true;
+		return;
+	}
 
-	if (!on_accessible_stack(tsk, fp, 16, &info))
-		return -EINVAL;
+	if (!on_accessible_stack(tsk, fp, 16, &info)) {
+		state->failed = true;
+		return;
+	}
 
-	if (test_bit(info.type, state->stacks_done))
-		return -EINVAL;
+	if (test_bit(info.type, state->stacks_done)) {
+		state->failed = true;
+		return;
+	}
 
 	/*
 	 * As stacks grow downward, any valid record on the same stack must be
@@ -166,8 +197,10 @@ static int notrace unwind_next(struct unwind_state *state)
 	 * stack.
 	 */
 	if (info.type == state->prev_type) {
-		if (fp <= state->prev_fp)
-			return -EINVAL;
+		if (fp <= state->prev_fp) {
+			state->failed = true;
+			return;
+		}
 	} else {
 		set_bit(state->prev_type, state->stacks_done);
 	}
@@ -195,8 +228,10 @@ static int notrace unwind_next(struct unwind_state *state)
 		 */
 		orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
 						(void *)state->fp);
-		if (WARN_ON_ONCE(state->pc == orig_pc))
-			return -EINVAL;
+		if (WARN_ON_ONCE(state->pc == orig_pc)) {
+			state->failed = true;
+			return;
+		}
 		state->pc = orig_pc;
 	}
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
@@ -204,23 +239,14 @@ static int notrace unwind_next(struct unwind_state *state)
 	if (is_kretprobe_trampoline(state->pc))
 		state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
 #endif
-
-	return 0;
 }
 NOKPROBE_SYMBOL(unwind_next);
 
 static void notrace unwind(struct unwind_state *state,
 			   stack_trace_consume_fn consume_entry, void *cookie)
 {
-	while (1) {
-		int ret;
-
-		if (!consume_entry(cookie, state->pc))
-			break;
-		ret = unwind_next(state);
-		if (ret < 0)
-			break;
-	}
+	while (unwind_continue(state, consume_entry, cookie))
+		unwind_next(state);
 }
 NOKPROBE_SYMBOL(unwind);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar to other architectures

[Mark Rutland]

On Fri, Jun 17, 2022 at 04:07:14PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> Change the loop in unwind()
> ===========================
> 
> Change the unwind loop in unwind() to:
> 
> 	while (unwind_continue(state, consume_entry, cookie))
> 		unwind_next(state);
> 
> This is easy to understand and maintain.
> New function unwind_continue()
> ==============================
> 
> Define a new function unwind_continue() that is used in the unwind loop
> to check for conditions that terminate a stack trace.
> 
> The conditions checked are:
> 
> 	- If the bottom of the stack (final frame) has been reached,
> 	  terminate.
> 
> 	- If the consume_entry() function returns false, the caller of
> 	  unwind has asked to terminate the stack trace. So, terminate.
> 
> 	- If unwind_next() failed for some reason (like stack corruption),
> 	  terminate.

I'm a bit confused as to why this structure, since AFAICT this doesn't match
other architectures (looking at x86, powerpc, and s390). I note that x86 has:

* In arch_stack_walk():

        for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
             unwind_next_frame(&state)) {
		...
		if (!consume_entry(...))
			break;
		...
	}

* In arch_stack_walk_reliable():

        for (unwind_start(&state, task, NULL, NULL);
             !unwind_done(&state) && !unwind_error(&state);
             unwind_next_frame(&state)) {
		...
		if (!consume_entry(...)
			return -EINVAL;
	}

... and back in v6 I suggeted exactly that shape:

  https://lore.kernel.org/linux-arm-kernel/20210728165635.GA47345@C02TD0UTHF1T.local/

> 
> Do not return an error value from unwind_next()
> ===============================================
> 
> We want to check for terminating conditions only in unwind_continue() from
> the unwinder loop. So, do not return an error value from unwind_next().
> Simply set a flag in unwind_state and check the flag in unwind_continue().

I'm fine with the concept of moving ghe return value out of unwind_next() (e.g.
if we go with an x86-like structure), but I don't think that we should
centralize the other checks *and* the consumption within unwind_continue(), as
I think those are two separate things.

> 
> Final FP
> ========
> 
> Introduce a new field "final_fp" in "struct unwind_state". Initialize this
> to the final frame of the stack trace:
> 
> 	task_pt_regs(task)->stackframe
> 
> This is where the stacktrace must terminate if it is successful. Add an
> explicit comment to that effect.

Can we please make this change as a preparatory step, as with the 'task' field?

We can wrap this in a helper like:

static bool is_final_frame(struct unwind state *state)
{
	return state->fp == state->final_fp;
}

... and use that in the main loop.

Thanks,
Mark.

> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 78 ++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 8e43444d50e2..c749129aba5a 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -40,6 +40,10 @@
>   *               value.
>   *
>   * @task:        The task being unwound.
> + *
> + * @final_fp:	 Pointer to the final frame.
> + *
> + * @failed:      Unwind failed.
>   */
>  struct unwind_state {
>  	unsigned long fp;
> @@ -51,6 +55,8 @@ struct unwind_state {
>  	struct llist_node *kr_cur;
>  #endif
>  	struct task_struct *task;
> +	unsigned long final_fp;
> +	bool failed;
>  };
>  
>  static void unwind_init_common(struct unwind_state *state,
> @@ -73,6 +79,10 @@ static void unwind_init_common(struct unwind_state *state,
>  	bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
>  	state->prev_fp = 0;
>  	state->prev_type = STACK_TYPE_UNKNOWN;
> +	state->failed = false;
> +
> +	/* Stack trace terminates here. */
> +	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
>  }
>  
>  /*
> @@ -126,6 +136,25 @@ static inline void unwind_init_from_task(struct unwind_state *state,
>  	state->pc = thread_saved_pc(task);
>  }
>  
> +static bool notrace unwind_continue(struct unwind_state *state,
> +				    stack_trace_consume_fn consume_entry,
> +				    void *cookie)
> +{
> +	if (state->failed) {
> +		/* PC is suspect. Cannot consume it. */
> +		return false;
> +	}
> +
> +	if (!consume_entry(cookie, state->pc)) {
> +		/* Caller terminated the unwind. */
> +		state->failed = true;
> +		return false;
> +	}
> +
> +	return state->fp != state->final_fp;
> +}
> +NOKPROBE_SYMBOL(unwind_continue);
> +
>  /*
>   * Unwind from one frame record (A) to the next frame record (B).
>   *
> @@ -133,24 +162,26 @@ static inline void unwind_init_from_task(struct unwind_state *state,
>   * records (e.g. a cycle), determined based on the location and fp value of A
>   * and the location (but not the fp value) of B.
>   */
> -static int notrace unwind_next(struct unwind_state *state)
> +static void notrace unwind_next(struct unwind_state *state)
>  {
>  	struct task_struct *tsk = state->task;
>  	unsigned long fp = state->fp;
>  	struct stack_info info;
>  
> -	/* Final frame; nothing to unwind */
> -	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
> -		return -ENOENT;
> -
> -	if (fp & 0x7)
> -		return -EINVAL;
> +	if (fp & 0x7) {
> +		state->failed = true;
> +		return;
> +	}
>  
> -	if (!on_accessible_stack(tsk, fp, 16, &info))
> -		return -EINVAL;
> +	if (!on_accessible_stack(tsk, fp, 16, &info)) {
> +		state->failed = true;
> +		return;
> +	}

>  
> -	if (test_bit(info.type, state->stacks_done))
> -		return -EINVAL;
> +	if (test_bit(info.type, state->stacks_done)) {
> +		state->failed = true;
> +		return;
> +	}
>  
>  	/*
>  	 * As stacks grow downward, any valid record on the same stack must be
> @@ -166,8 +197,10 @@ static int notrace unwind_next(struct unwind_state *state)
>  	 * stack.
>  	 */
>  	if (info.type == state->prev_type) {
> -		if (fp <= state->prev_fp)
> -			return -EINVAL;
> +		if (fp <= state->prev_fp) {
> +			state->failed = true;
> +			return;
> +		}
>  	} else {
>  		set_bit(state->prev_type, state->stacks_done);
>  	}
> @@ -195,8 +228,10 @@ static int notrace unwind_next(struct unwind_state *state)
>  		 */
>  		orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
>  						(void *)state->fp);
> -		if (WARN_ON_ONCE(state->pc == orig_pc))
> -			return -EINVAL;
> +		if (WARN_ON_ONCE(state->pc == orig_pc)) {
> +			state->failed = true;
> +			return;
> +		}
>  		state->pc = orig_pc;
>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> @@ -204,23 +239,14 @@ static int notrace unwind_next(struct unwind_state *state)
>  	if (is_kretprobe_trampoline(state->pc))
>  		state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
>  #endif
> -
> -	return 0;
>  }
>  NOKPROBE_SYMBOL(unwind_next);
>  
>  static void notrace unwind(struct unwind_state *state,
>  			   stack_trace_consume_fn consume_entry, void *cookie)
>  {
> -	while (1) {
> -		int ret;
> -
> -		if (!consume_entry(cookie, state->pc))
> -			break;
> -		ret = unwind_next(state);
> -		if (ret < 0)
> -			break;
> -	}
> +	while (unwind_continue(state, consume_entry, cookie))
> +		unwind_next(state);
>  }
>  NOKPROBE_SYMBOL(unwind);
>  
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar to other architectures

[Madhavan T. Venkataraman]

On 6/26/22 03:21, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:14PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Change the loop in unwind()
>> ===========================
>>
>> Change the unwind loop in unwind() to:
>>
>> 	while (unwind_continue(state, consume_entry, cookie))
>> 		unwind_next(state);
>>
>> This is easy to understand and maintain.
>> New function unwind_continue()
>> ==============================
>>
>> Define a new function unwind_continue() that is used in the unwind loop
>> to check for conditions that terminate a stack trace.
>>
>> The conditions checked are:
>>
>> 	- If the bottom of the stack (final frame) has been reached,
>> 	  terminate.
>>
>> 	- If the consume_entry() function returns false, the caller of
>> 	  unwind has asked to terminate the stack trace. So, terminate.
>>
>> 	- If unwind_next() failed for some reason (like stack corruption),
>> 	  terminate.
> 
> I'm a bit confused as to why this structure, since AFAICT this doesn't match
> other architectures (looking at x86, powerpc, and s390). I note that x86 has:
> 
> * In arch_stack_walk():
> 
>         for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...))
> 			break;
> 		...
> 	}
> 
> * In arch_stack_walk_reliable():
> 
>         for (unwind_start(&state, task, NULL, NULL);
>              !unwind_done(&state) && !unwind_error(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...)
> 			return -EINVAL;
> 	}
> 
> ... and back in v6 I suggeted exactly that shape:
> 
>   https://lore.kernel.org/linux-arm-kernel/20210728165635.GA47345@C02TD0UTHF1T.local/
> 

OK. I will take a look at your suggestion and resend this patch.

>>
>> Do not return an error value from unwind_next()
>> ===============================================
>>
>> We want to check for terminating conditions only in unwind_continue() from
>> the unwinder loop. So, do not return an error value from unwind_next().
>> Simply set a flag in unwind_state and check the flag in unwind_continue().
> 
> I'm fine with the concept of moving ghe return value out of unwind_next() (e.g.
> if we go with an x86-like structure), but I don't think that we should
> centralize the other checks *and* the consumption within unwind_continue(), as
> I think those are two separate things.
> 

OK. I will address this in the next version.

>>
>> Final FP
>> ========
>>
>> Introduce a new field "final_fp" in "struct unwind_state". Initialize this
>> to the final frame of the stack trace:
>>
>> 	task_pt_regs(task)->stackframe
>>
>> This is where the stacktrace must terminate if it is successful. Add an
>> explicit comment to that effect.
> 
> Can we please make this change as a preparatory step, as with the 'task' field?
> 
> We can wrap this in a helper like:
> 
> static bool is_final_frame(struct unwind state *state)
> {
> 	return state->fp == state->final_fp;
> }
> 
> ... and use that in the main loop.
> 

OK. I will make these changes.

Thanks.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 4/6] arm64: Introduce stack trace reliability checks in the unwinder

[madvenka]

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

There are some kernel features and conditions that make a stack trace
unreliable. Callers may require the unwinder to detect these cases.
E.g., livepatch.

Introduce a new function called unwind_check_reliability() that will
detect these cases and set a flag in the stack frame. Call
unwind_check_reliability() for every frame in unwind().

Introduce the first reliability check in unwind_check_reliability() - If
a return PC is not a valid kernel text address, consider the stack
trace unreliable. It could be some generated code. Other reliability checks
will be added in the future.

Let unwind() return a boolean to indicate if the stack trace is
reliable.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index c749129aba5a..5ef2ce217324 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -44,6 +44,8 @@
  * @final_fp:	 Pointer to the final frame.
  *
  * @failed:      Unwind failed.
+ *
+ * @reliable:    Stack trace is reliable.
  */
 struct unwind_state {
 	unsigned long fp;
@@ -57,6 +59,7 @@ struct unwind_state {
 	struct task_struct *task;
 	unsigned long final_fp;
 	bool failed;
+	bool reliable;
 };
 
 static void unwind_init_common(struct unwind_state *state,
@@ -80,6 +83,7 @@ static void unwind_init_common(struct unwind_state *state,
 	state->prev_fp = 0;
 	state->prev_type = STACK_TYPE_UNKNOWN;
 	state->failed = false;
+	state->reliable = true;
 
 	/* Stack trace terminates here. */
 	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
@@ -242,11 +246,34 @@ static void notrace unwind_next(struct unwind_state *state)
 }
 NOKPROBE_SYMBOL(unwind_next);
 
-static void notrace unwind(struct unwind_state *state,
+/*
+ * Check the stack frame for conditions that make further unwinding unreliable.
+ */
+static void unwind_check_reliability(struct unwind_state *state)
+{
+	if (state->fp == state->final_fp) {
+		/* Final frame; no more unwind, no need to check reliability */
+		return;
+	}
+
+	/*
+	 * If the PC is not a known kernel text address, then we cannot
+	 * be sure that a subsequent unwind will be reliable, as we
+	 * don't know that the code follows our unwind requirements.
+	 */
+	if (!__kernel_text_address(state->pc))
+		state->reliable = false;
+}
+
+static bool notrace unwind(struct unwind_state *state,
 			   stack_trace_consume_fn consume_entry, void *cookie)
 {
-	while (unwind_continue(state, consume_entry, cookie))
+	unwind_check_reliability(state);
+	while (unwind_continue(state, consume_entry, cookie)) {
 		unwind_next(state);
+		unwind_check_reliability(state);
+	}
+	return !state->failed && state->reliable;
 }
 NOKPROBE_SYMBOL(unwind);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 4/6] arm64: Introduce stack trace reliability checks in the unwinder

[Mark Rutland]

On Fri, Jun 17, 2022 at 04:07:15PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> There are some kernel features and conditions that make a stack trace
> unreliable. Callers may require the unwinder to detect these cases.
> E.g., livepatch.
> 
> Introduce a new function called unwind_check_reliability() that will
> detect these cases and set a flag in the stack frame. Call
> unwind_check_reliability() for every frame in unwind().
> 
> Introduce the first reliability check in unwind_check_reliability() - If
> a return PC is not a valid kernel text address, consider the stack
> trace unreliable. It could be some generated code. Other reliability checks
> will be added in the future.
> 
> Let unwind() return a boolean to indicate if the stack trace is
> reliable.
> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index c749129aba5a..5ef2ce217324 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -44,6 +44,8 @@
>   * @final_fp:	 Pointer to the final frame.
>   *
>   * @failed:      Unwind failed.
> + *
> + * @reliable:    Stack trace is reliable.
>   */

I would strongly prefer if we could have something like an
unwind_state_is_reliable() helper, and just use that directly, rather than
storing that into the state.

That way, we can opt-into any expensive checks in the reliable unwinder (e.g.
__kernel_text_address), and can use them elsewhere for informative purposes
(e.g. when dumping a stacktrace out to the console).

>  struct unwind_state {
>  	unsigned long fp;
> @@ -57,6 +59,7 @@ struct unwind_state {
>  	struct task_struct *task;
>  	unsigned long final_fp;
>  	bool failed;
> +	bool reliable;
>  };
>  
>  static void unwind_init_common(struct unwind_state *state,
> @@ -80,6 +83,7 @@ static void unwind_init_common(struct unwind_state *state,
>  	state->prev_fp = 0;
>  	state->prev_type = STACK_TYPE_UNKNOWN;
>  	state->failed = false;
> +	state->reliable = true;
>  
>  	/* Stack trace terminates here. */
>  	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
> @@ -242,11 +246,34 @@ static void notrace unwind_next(struct unwind_state *state)
>  }
>  NOKPROBE_SYMBOL(unwind_next);
>  
> -static void notrace unwind(struct unwind_state *state,
> +/*
> + * Check the stack frame for conditions that make further unwinding unreliable.
> + */
> +static void unwind_check_reliability(struct unwind_state *state)
> +{
> +	if (state->fp == state->final_fp) {
> +		/* Final frame; no more unwind, no need to check reliability */
> +		return;
> +	}
> +
> +	/*
> +	 * If the PC is not a known kernel text address, then we cannot
> +	 * be sure that a subsequent unwind will be reliable, as we
> +	 * don't know that the code follows our unwind requirements.
> +	 */
> +	if (!__kernel_text_address(state->pc))
> +		state->reliable = false;
> +}

I'd strongly prefer that we split this into two helpers, e.g.

static inline bool unwind_state_is_final(struct unwind_state *state)
{
	return state->fp == state->final_fp;
}

static inline bool unwind_state_is_reliable(struct unwind_state *state)
{
	return __kernel_text_address(state->pc);
}

> +
> +static bool notrace unwind(struct unwind_state *state,
>  			   stack_trace_consume_fn consume_entry, void *cookie)
>  {
> -	while (unwind_continue(state, consume_entry, cookie))
> +	unwind_check_reliability(state);
> +	while (unwind_continue(state, consume_entry, cookie)) {
>  		unwind_next(state);
> +		unwind_check_reliability(state);

This is going to slow down regular unwinds even when the reliablity value is
not consumed (e.g. for KASAN traces on alloc and free), so I don't think this
should live here, and should be intreoduced with arch_stack_walk_reliable().

Thanks,
Mark.

> +	}
> +	return !state->failed && state->reliable;
>  }
>  NOKPROBE_SYMBOL(unwind);
>  
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 4/6] arm64: Introduce stack trace reliability checks in the unwinder

[Madhavan T. Venkataraman]

On 6/26/22 03:32, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:15PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> There are some kernel features and conditions that make a stack trace
>> unreliable. Callers may require the unwinder to detect these cases.
>> E.g., livepatch.
>>
>> Introduce a new function called unwind_check_reliability() that will
>> detect these cases and set a flag in the stack frame. Call
>> unwind_check_reliability() for every frame in unwind().
>>
>> Introduce the first reliability check in unwind_check_reliability() - If
>> a return PC is not a valid kernel text address, consider the stack
>> trace unreliable. It could be some generated code. Other reliability checks
>> will be added in the future.
>>
>> Let unwind() return a boolean to indicate if the stack trace is
>> reliable.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
>> Reviewed-by: Mark Brown <broonie@kernel.org>
>> ---
>>  arch/arm64/kernel/stacktrace.c | 31 +++++++++++++++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index c749129aba5a..5ef2ce217324 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -44,6 +44,8 @@
>>   * @final_fp:	 Pointer to the final frame.
>>   *
>>   * @failed:      Unwind failed.
>> + *
>> + * @reliable:    Stack trace is reliable.
>>   */
> 
> I would strongly prefer if we could have something like an
> unwind_state_is_reliable() helper, and just use that directly, rather than
> storing that into the state.
> 
> That way, we can opt-into any expensive checks in the reliable unwinder (e.g.
> __kernel_text_address), and can use them elsewhere for informative purposes
> (e.g. when dumping a stacktrace out to the console).
> 
>>  struct unwind_state {
>>  	unsigned long fp;
>> @@ -57,6 +59,7 @@ struct unwind_state {
>>  	struct task_struct *task;
>>  	unsigned long final_fp;
>>  	bool failed;
>> +	bool reliable;
>>  };
>>  
>>  static void unwind_init_common(struct unwind_state *state,
>> @@ -80,6 +83,7 @@ static void unwind_init_common(struct unwind_state *state,
>>  	state->prev_fp = 0;
>>  	state->prev_type = STACK_TYPE_UNKNOWN;
>>  	state->failed = false;
>> +	state->reliable = true;
>>  
>>  	/* Stack trace terminates here. */
>>  	state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
>> @@ -242,11 +246,34 @@ static void notrace unwind_next(struct unwind_state *state)
>>  }
>>  NOKPROBE_SYMBOL(unwind_next);
>>  
>> -static void notrace unwind(struct unwind_state *state,
>> +/*
>> + * Check the stack frame for conditions that make further unwinding unreliable.
>> + */
>> +static void unwind_check_reliability(struct unwind_state *state)
>> +{
>> +	if (state->fp == state->final_fp) {
>> +		/* Final frame; no more unwind, no need to check reliability */
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * If the PC is not a known kernel text address, then we cannot
>> +	 * be sure that a subsequent unwind will be reliable, as we
>> +	 * don't know that the code follows our unwind requirements.
>> +	 */
>> +	if (!__kernel_text_address(state->pc))
>> +		state->reliable = false;
>> +}
> 
> I'd strongly prefer that we split this into two helpers, e.g.
> 
> static inline bool unwind_state_is_final(struct unwind_state *state)
> {
> 	return state->fp == state->final_fp;
> }
> 
> static inline bool unwind_state_is_reliable(struct unwind_state *state)
> {
> 	return __kernel_text_address(state->pc);
> }
> 
>> +
>> +static bool notrace unwind(struct unwind_state *state,
>>  			   stack_trace_consume_fn consume_entry, void *cookie)
>>  {
>> -	while (unwind_continue(state, consume_entry, cookie))
>> +	unwind_check_reliability(state);
>> +	while (unwind_continue(state, consume_entry, cookie)) {
>>  		unwind_next(state);
>> +		unwind_check_reliability(state);
> 
> This is going to slow down regular unwinds even when the reliablity value is
> not consumed (e.g. for KASAN traces on alloc and free), so I don't think this
> should live here, and should be intreoduced with arch_stack_walk_reliable().
> 

So, I have been thinking about this whole reliability check thing. Instead of
checking many different things for reliability, I believe that a single frame
pointer validation check is sufficient. I am attempting to do that in my
other frame pointer validation patch series. Hopefully, in that patch series,
I can prove that that one check is sufficient. We will continue this discussion
there.

So, for now, I am dropping the reliability checks patches from the series.
I will just send the unwind loop reorg in v16 and focus on getting that
upstreamed.

Thanks.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 5/6] arm64: Create a list of SYM_CODE functions, check return PC against list

[madvenka]

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

SYM_CODE functions don't follow the usual calling conventions. Check if the
return PC in a stack frame falls in any of these. If it does, consider the
stack trace unreliable.

Define a special section for unreliable functions
=================================================

Define a SYM_CODE_END() macro for arm64 that adds the function address
range to a new section called "sym_code_functions".

Linker file
===========

Include the "sym_code_functions" section under read-only data in
vmlinux.lds.S.

Initialization
==============

Define an early_initcall() to create a sym_code_functions[] array from
the linker data.

Unwinder check
==============

Add a reliability check in unwind_check_reliability() that compares a
return PC with sym_code_functions[]. If there is a match, then return
failure.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/linkage.h  | 11 +++++++
 arch/arm64/include/asm/sections.h |  1 +
 arch/arm64/kernel/stacktrace.c    | 55 +++++++++++++++++++++++++++++++
 arch/arm64/kernel/vmlinux.lds.S   | 10 ++++++
 4 files changed, 77 insertions(+)

diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index 43f8c25b3fda..d4058de4af78 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -39,4 +39,15 @@
 	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
 	bti c ;
 
+/*
+ * Record the address range of each SYM_CODE function in a struct code_range
+ * in a special section.
+ */
+#define SYM_CODE_END(name)				\
+	SYM_END(name, SYM_T_NONE)			;\
+99:	.pushsection "sym_code_functions", "aw"		;\
+	.quad	name					;\
+	.quad	99b					;\
+	.popsection
+
 #endif
diff --git a/arch/arm64/include/asm/sections.h b/arch/arm64/include/asm/sections.h
index 40971ac1303f..50cfd1083563 100644
--- a/arch/arm64/include/asm/sections.h
+++ b/arch/arm64/include/asm/sections.h
@@ -22,6 +22,7 @@ extern char __irqentry_text_start[], __irqentry_text_end[];
 extern char __mmuoff_data_start[], __mmuoff_data_end[];
 extern char __entry_tramp_text_start[], __entry_tramp_text_end[];
 extern char __relocate_new_kernel_start[], __relocate_new_kernel_end[];
+extern char __sym_code_functions_start[], __sym_code_functions_end[];
 
 static inline size_t entry_tramp_text_size(void)
 {
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 5ef2ce217324..eda8581f7dbe 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -62,6 +62,31 @@ struct unwind_state {
 	bool reliable;
 };
 
+struct code_range {
+	unsigned long	start;
+	unsigned long	end;
+};
+
+static struct code_range	*sym_code_functions;
+static int			num_sym_code_functions;
+
+int __init init_sym_code_functions(void)
+{
+	size_t size = (unsigned long)__sym_code_functions_end -
+		      (unsigned long)__sym_code_functions_start;
+
+	sym_code_functions = (struct code_range *)__sym_code_functions_start;
+	/*
+	 * Order it so that sym_code_functions is not visible before
+	 * num_sym_code_functions.
+	 */
+	smp_mb();
+	num_sym_code_functions = size / sizeof(struct code_range);
+
+	return 0;
+}
+early_initcall(init_sym_code_functions);
+
 static void unwind_init_common(struct unwind_state *state,
 			       struct task_struct *task)
 {
@@ -251,6 +276,10 @@ NOKPROBE_SYMBOL(unwind_next);
  */
 static void unwind_check_reliability(struct unwind_state *state)
 {
+	const struct code_range *range;
+	unsigned long pc;
+	int i;
+
 	if (state->fp == state->final_fp) {
 		/* Final frame; no more unwind, no need to check reliability */
 		return;
@@ -263,6 +292,32 @@ static void unwind_check_reliability(struct unwind_state *state)
 	 */
 	if (!__kernel_text_address(state->pc))
 		state->reliable = false;
+
+	/*
+	 * Check the return PC against sym_code_functions[]. If there is a
+	 * match, then the consider the stack frame unreliable.
+	 *
+	 * As SYM_CODE functions don't follow the usual calling conventions,
+	 * we assume by default that any SYM_CODE function cannot be unwound
+	 * reliably.
+	 *
+	 * Note that this includes:
+	 *
+	 * - Exception handlers and entry assembly
+	 * - Trampoline assembly (e.g., ftrace, kprobes)
+	 * - Hypervisor-related assembly
+	 * - Hibernation-related assembly
+	 * - CPU start-stop, suspend-resume assembly
+	 * - Kernel relocation assembly
+	 */
+	pc = state->pc;
+	for (i = 0; i < num_sym_code_functions; i++) {
+		range = &sym_code_functions[i];
+		if (pc >= range->start && pc < range->end) {
+			state->reliable = false;
+			return;
+		}
+	}
 }
 
 static bool notrace unwind(struct unwind_state *state,
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 2d4a8f995175..414dbc82d0a6 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -120,6 +120,14 @@ jiffies = jiffies_64;
 #define TRAMP_TEXT
 #endif
 
+#define SYM_CODE_FUNCTIONS				\
+	. = ALIGN(16);					\
+	.symcode : AT(ADDR(.symcode) - LOAD_OFFSET) {	\
+		__sym_code_functions_start = .;		\
+		KEEP(*(sym_code_functions))		\
+		__sym_code_functions_end = .;		\
+	}
+
 /*
  * The size of the PE/COFF section that covers the kernel image, which
  * runs from _stext to _edata, must be a round multiple of the PE/COFF
@@ -212,6 +220,8 @@ SECTIONS
 	swapper_pg_dir = .;
 	. += PAGE_SIZE;
 
+	SYM_CODE_FUNCTIONS
+
 	. = ALIGN(SEGMENT_ALIGN);
 	__init_begin = .;
 	__inittext_begin = .;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 5/6] arm64: Create a list of SYM_CODE functions, check return PC against list

[Mark Rutland]

On Fri, Jun 17, 2022 at 04:07:16PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> SYM_CODE functions don't follow the usual calling conventions. Check if the
> return PC in a stack frame falls in any of these. If it does, consider the
> stack trace unreliable.
> 
> Define a special section for unreliable functions
> =================================================
> 
> Define a SYM_CODE_END() macro for arm64 that adds the function address
> range to a new section called "sym_code_functions".
> 
> Linker file
> ===========
> 
> Include the "sym_code_functions" section under read-only data in
> vmlinux.lds.S.
> 
> Initialization
> ==============
> 
> Define an early_initcall() to create a sym_code_functions[] array from
> the linker data.
> 
> Unwinder check
> ==============
> 
> Add a reliability check in unwind_check_reliability() that compares a
> return PC with sym_code_functions[]. If there is a match, then return
> failure.
> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/linkage.h  | 11 +++++++
>  arch/arm64/include/asm/sections.h |  1 +
>  arch/arm64/kernel/stacktrace.c    | 55 +++++++++++++++++++++++++++++++
>  arch/arm64/kernel/vmlinux.lds.S   | 10 ++++++
>  4 files changed, 77 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
> index 43f8c25b3fda..d4058de4af78 100644
> --- a/arch/arm64/include/asm/linkage.h
> +++ b/arch/arm64/include/asm/linkage.h
> @@ -39,4 +39,15 @@
>  	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
>  	bti c ;
>  
> +/*
> + * Record the address range of each SYM_CODE function in a struct code_range
> + * in a special section.
> + */
> +#define SYM_CODE_END(name)				\
> +	SYM_END(name, SYM_T_NONE)			;\
> +99:	.pushsection "sym_code_functions", "aw"		;\
> +	.quad	name					;\
> +	.quad	99b					;\
> +	.popsection
> +
>  #endif
> diff --git a/arch/arm64/include/asm/sections.h b/arch/arm64/include/asm/sections.h
> index 40971ac1303f..50cfd1083563 100644
> --- a/arch/arm64/include/asm/sections.h
> +++ b/arch/arm64/include/asm/sections.h
> @@ -22,6 +22,7 @@ extern char __irqentry_text_start[], __irqentry_text_end[];
>  extern char __mmuoff_data_start[], __mmuoff_data_end[];
>  extern char __entry_tramp_text_start[], __entry_tramp_text_end[];
>  extern char __relocate_new_kernel_start[], __relocate_new_kernel_end[];
> +extern char __sym_code_functions_start[], __sym_code_functions_end[];
>  
>  static inline size_t entry_tramp_text_size(void)
>  {
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 5ef2ce217324..eda8581f7dbe 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -62,6 +62,31 @@ struct unwind_state {
>  	bool reliable;
>  };
>  
> +struct code_range {
> +	unsigned long	start;
> +	unsigned long	end;
> +};
> +
> +static struct code_range	*sym_code_functions;
> +static int			num_sym_code_functions;
> +
> +int __init init_sym_code_functions(void)
> +{
> +	size_t size = (unsigned long)__sym_code_functions_end -
> +		      (unsigned long)__sym_code_functions_start;
> +
> +	sym_code_functions = (struct code_range *)__sym_code_functions_start;
> +	/*
> +	 * Order it so that sym_code_functions is not visible before
> +	 * num_sym_code_functions.
> +	 */
> +	smp_mb();
> +	num_sym_code_functions = size / sizeof(struct code_range);
> +
> +	return 0;
> +}
> +early_initcall(init_sym_code_functions);

There's no reason to need an initcall for this; we can iterate over this
directly using __sym_code_functions_start and __sym_code_functions_end, like we
do for exception tables today.

For example:

static inline bool pc_is_sym_code(unsigned long pc)
{
	extern struct code_range *__sym_code_functions_start;
	extern struct code_range *__sym_code_functions_end;

	struct code_range *r;

	for (r = __sym_code_functions_start; r < __sym_code_functions_end; r++) {
		if (pc >= r->start && pc < r->end)
			return true;
	}

	return false;
}

Thanks,
Mark.

> +
>  static void unwind_init_common(struct unwind_state *state,
>  			       struct task_struct *task)
>  {
> @@ -251,6 +276,10 @@ NOKPROBE_SYMBOL(unwind_next);
>   */
>  static void unwind_check_reliability(struct unwind_state *state)
>  {
> +	const struct code_range *range;
> +	unsigned long pc;
> +	int i;
> +
>  	if (state->fp == state->final_fp) {
>  		/* Final frame; no more unwind, no need to check reliability */
>  		return;
> @@ -263,6 +292,32 @@ static void unwind_check_reliability(struct unwind_state *state)
>  	 */
>  	if (!__kernel_text_address(state->pc))
>  		state->reliable = false;
> +
> +	/*
> +	 * Check the return PC against sym_code_functions[]. If there is a
> +	 * match, then the consider the stack frame unreliable.
> +	 *
> +	 * As SYM_CODE functions don't follow the usual calling conventions,
> +	 * we assume by default that any SYM_CODE function cannot be unwound
> +	 * reliably.
> +	 *
> +	 * Note that this includes:
> +	 *
> +	 * - Exception handlers and entry assembly
> +	 * - Trampoline assembly (e.g., ftrace, kprobes)
> +	 * - Hypervisor-related assembly
> +	 * - Hibernation-related assembly
> +	 * - CPU start-stop, suspend-resume assembly
> +	 * - Kernel relocation assembly
> +	 */
> +	pc = state->pc;
> +	for (i = 0; i < num_sym_code_functions; i++) {
> +		range = &sym_code_functions[i];
> +		if (pc >= range->start && pc < range->end) {
> +			state->reliable = false;
> +			return;
> +		}
> +	}
>  }
>  
>  static bool notrace unwind(struct unwind_state *state,
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 2d4a8f995175..414dbc82d0a6 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -120,6 +120,14 @@ jiffies = jiffies_64;
>  #define TRAMP_TEXT
>  #endif
>  
> +#define SYM_CODE_FUNCTIONS				\
> +	. = ALIGN(16);					\
> +	.symcode : AT(ADDR(.symcode) - LOAD_OFFSET) {	\
> +		__sym_code_functions_start = .;		\
> +		KEEP(*(sym_code_functions))		\
> +		__sym_code_functions_end = .;		\
> +	}
> +
>  /*
>   * The size of the PE/COFF section that covers the kernel image, which
>   * runs from _stext to _edata, must be a round multiple of the PE/COFF
> @@ -212,6 +220,8 @@ SECTIONS
>  	swapper_pg_dir = .;
>  	. += PAGE_SIZE;
>  
> +	SYM_CODE_FUNCTIONS
> +
>  	. = ALIGN(SEGMENT_ALIGN);
>  	__init_begin = .;
>  	__inittext_begin = .;
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 5/6] arm64: Create a list of SYM_CODE functions, check return PC against list

[Madhavan T. Venkataraman]

On 6/26/22 03:46, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:16PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> SYM_CODE functions don't follow the usual calling conventions. Check if the
>> return PC in a stack frame falls in any of these. If it does, consider the
>> stack trace unreliable.
>>
>> Define a special section for unreliable functions
>> =================================================
>>
>> Define a SYM_CODE_END() macro for arm64 that adds the function address
>> range to a new section called "sym_code_functions".
>>
>> Linker file
>> ===========
>>
>> Include the "sym_code_functions" section under read-only data in
>> vmlinux.lds.S.
>>
>> Initialization
>> ==============
>>
>> Define an early_initcall() to create a sym_code_functions[] array from
>> the linker data.
>>
>> Unwinder check
>> ==============
>>
>> Add a reliability check in unwind_check_reliability() that compares a
>> return PC with sym_code_functions[]. If there is a match, then return
>> failure.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
>> Reviewed-by: Mark Brown <broonie@kernel.org>
>> ---
>>  arch/arm64/include/asm/linkage.h  | 11 +++++++
>>  arch/arm64/include/asm/sections.h |  1 +
>>  arch/arm64/kernel/stacktrace.c    | 55 +++++++++++++++++++++++++++++++
>>  arch/arm64/kernel/vmlinux.lds.S   | 10 ++++++
>>  4 files changed, 77 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
>> index 43f8c25b3fda..d4058de4af78 100644
>> --- a/arch/arm64/include/asm/linkage.h
>> +++ b/arch/arm64/include/asm/linkage.h
>> @@ -39,4 +39,15 @@
>>  	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
>>  	bti c ;
>>  
>> +/*
>> + * Record the address range of each SYM_CODE function in a struct code_range
>> + * in a special section.
>> + */
>> +#define SYM_CODE_END(name)				\
>> +	SYM_END(name, SYM_T_NONE)			;\
>> +99:	.pushsection "sym_code_functions", "aw"		;\
>> +	.quad	name					;\
>> +	.quad	99b					;\
>> +	.popsection
>> +
>>  #endif
>> diff --git a/arch/arm64/include/asm/sections.h b/arch/arm64/include/asm/sections.h
>> index 40971ac1303f..50cfd1083563 100644
>> --- a/arch/arm64/include/asm/sections.h
>> +++ b/arch/arm64/include/asm/sections.h
>> @@ -22,6 +22,7 @@ extern char __irqentry_text_start[], __irqentry_text_end[];
>>  extern char __mmuoff_data_start[], __mmuoff_data_end[];
>>  extern char __entry_tramp_text_start[], __entry_tramp_text_end[];
>>  extern char __relocate_new_kernel_start[], __relocate_new_kernel_end[];
>> +extern char __sym_code_functions_start[], __sym_code_functions_end[];
>>  
>>  static inline size_t entry_tramp_text_size(void)
>>  {
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 5ef2ce217324..eda8581f7dbe 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -62,6 +62,31 @@ struct unwind_state {
>>  	bool reliable;
>>  };
>>  
>> +struct code_range {
>> +	unsigned long	start;
>> +	unsigned long	end;
>> +};
>> +
>> +static struct code_range	*sym_code_functions;
>> +static int			num_sym_code_functions;
>> +
>> +int __init init_sym_code_functions(void)
>> +{
>> +	size_t size = (unsigned long)__sym_code_functions_end -
>> +		      (unsigned long)__sym_code_functions_start;
>> +
>> +	sym_code_functions = (struct code_range *)__sym_code_functions_start;
>> +	/*
>> +	 * Order it so that sym_code_functions is not visible before
>> +	 * num_sym_code_functions.
>> +	 */
>> +	smp_mb();
>> +	num_sym_code_functions = size / sizeof(struct code_range);
>> +
>> +	return 0;
>> +}
>> +early_initcall(init_sym_code_functions);
> 
> There's no reason to need an initcall for this; we can iterate over this
> directly using __sym_code_functions_start and __sym_code_functions_end, like we
> do for exception tables today.
> 
> For example:
> 
> static inline bool pc_is_sym_code(unsigned long pc)
> {
> 	extern struct code_range *__sym_code_functions_start;
> 	extern struct code_range *__sym_code_functions_end;
> 
> 	struct code_range *r;
> 
> 	for (r = __sym_code_functions_start; r < __sym_code_functions_end; r++) {
> 		if (pc >= r->start && pc < r->end)
> 			return true;
> 	}
> 
> 	return false;
> }
> 

OK.

However, I have decided to hold off on the reliability checks until we have the right
structure in the unwind code. I am also trying to address the question of reliability
with a single FP check in my FP validation series.

So, for now, I will remove the reliability checks part of the patch series.

Thanks for the review though. It will be useful when I revisit this in the future and
resend.

Thanks.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v15 6/6] arm64: Introduce arch_stack_walk_reliable()

[madvenka]

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

Introduce arch_stack_walk_reliable() for ARM64. This works like
arch_stack_walk() except that it returns -EINVAL if the stack trace is not
reliable.

Until all the reliability checks are in place, arch_stack_walk_reliable()
may not be used by livepatch. But it may be used by debug and test code.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index eda8581f7dbe..8016ba0e2c96 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -383,3 +383,26 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 
 	unwind(&state, consume_entry, cookie);
 }
+
+/*
+ * arch_stack_walk_reliable() may not be used for livepatch until all of
+ * the reliability checks are in place in unwind_consume(). However,
+ * debug and test code can choose to use it even if all the checks are not
+ * in place.
+ */
+noinline int notrace arch_stack_walk_reliable(
+					stack_trace_consume_fn consume_entry,
+					void *cookie,
+					struct task_struct *task)
+{
+	struct unwind_state state;
+	bool reliable;
+
+	if (task == current)
+		unwind_init_from_caller(&state);
+	else
+		unwind_init_from_task(&state, task);
+
+	reliable = unwind(&state, consume_entry, cookie);
+	return reliable ? 0 : -EINVAL;
+}
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 6/6] arm64: Introduce arch_stack_walk_reliable()

[Mark Rutland]

On Fri, Jun 17, 2022 at 04:07:17PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> Introduce arch_stack_walk_reliable() for ARM64. This works like
> arch_stack_walk() except that it returns -EINVAL if the stack trace is not
> reliable.
> 
> Until all the reliability checks are in place, arch_stack_walk_reliable()
> may not be used by livepatch. But it may be used by debug and test code.

For the moment I would strongly perfer *not* to add this until we have the
missing bits and pieces sorted out.

Until then, I'd like to ensure that any infrastructure we add is immediately
useful and tested. One way to do that would be to enhance the stack dumping
code (i.e. dump_backtrace()) to log some metadata.

As an end-goal, I'd like to get to a point where we can do:

* Explicit logging when trace terminate at the final frame, e.g.

  stacktrace:
    function_c+offset/total
    function_b+offset/total
    function_a+offset/total
    <unwind successful>

* Explicit logging of early termination, e.g.

  stacktrace:
    function_c+offset/total
    <unwind terminated early (bad FP)>

* Unreliability on individual elements, e.g.

  stacktrace:
    function_c+offset/total
    function_b+offset/total (?)
    function_a+offset/total

* Annotations for special unwinding, e.g.

  stacktrace:
    function_c+offset/total (K) // kretprobes trampoline
    function_b+offset/total (F) // ftrace trampoline
    function_a+offset/total (FK) // ftrace and kretprobes
    other_function+offset/total (P) // from pt_regs::pc
    another_function+offset/total (L?) // from pt_regs::lr, unreliable
    something_else+offset/total

  Note: the comments here are just to explain the idea, I don't expect those in
  the actual output.

That'll justify some of the infrastructure we need for reliable unwinding, and
ensure that it is tested, well before we actually enable reliable stacktracing.

Thanks,
Mark.

> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index eda8581f7dbe..8016ba0e2c96 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -383,3 +383,26 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  
>  	unwind(&state, consume_entry, cookie);
>  }
> +
> +/*
> + * arch_stack_walk_reliable() may not be used for livepatch until all of
> + * the reliability checks are in place in unwind_consume(). However,
> + * debug and test code can choose to use it even if all the checks are not
> + * in place.
> + */
> +noinline int notrace arch_stack_walk_reliable(
> +					stack_trace_consume_fn consume_entry,
> +					void *cookie,
> +					struct task_struct *task)
> +{
> +	struct unwind_state state;
> +	bool reliable;
> +
> +	if (task == current)
> +		unwind_init_from_caller(&state);
> +	else
> +		unwind_init_from_task(&state, task);
> +
> +	reliable = unwind(&state, consume_entry, cookie);
> +	return reliable ? 0 : -EINVAL;
> +}
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 6/6] arm64: Introduce arch_stack_walk_reliable()

[Madhavan T. Venkataraman]

On 6/26/22 03:57, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:17PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Introduce arch_stack_walk_reliable() for ARM64. This works like
>> arch_stack_walk() except that it returns -EINVAL if the stack trace is not
>> reliable.
>>
>> Until all the reliability checks are in place, arch_stack_walk_reliable()
>> may not be used by livepatch. But it may be used by debug and test code.
> 
> For the moment I would strongly perfer *not* to add this until we have the
> missing bits and pieces sorted out.
> 

Yes. I am removing this from the patch series.

> Until then, I'd like to ensure that any infrastructure we add is immediately
> useful and tested. One way to do that would be to enhance the stack dumping
> code (i.e. dump_backtrace()) to log some metadata.
> 
> As an end-goal, I'd like to get to a point where we can do:
> 
> * Explicit logging when trace terminate at the final frame, e.g.
> 
>   stacktrace:
>     function_c+offset/total
>     function_b+offset/total
>     function_a+offset/total
>     <unwind successful>
> 
> * Explicit logging of early termination, e.g.
> 
>   stacktrace:
>     function_c+offset/total
>     <unwind terminated early (bad FP)>
> 
> * Unreliability on individual elements, e.g.
> 
>   stacktrace:
>     function_c+offset/total
>     function_b+offset/total (?)
>     function_a+offset/total
> 
> * Annotations for special unwinding, e.g.
> 
>   stacktrace:
>     function_c+offset/total (K) // kretprobes trampoline
>     function_b+offset/total (F) // ftrace trampoline
>     function_a+offset/total (FK) // ftrace and kretprobes
>     other_function+offset/total (P) // from pt_regs::pc
>     another_function+offset/total (L?) // from pt_regs::lr, unreliable
>     something_else+offset/total
> 
>   Note: the comments here are just to explain the idea, I don't expect those in
>   the actual output.
> 
> That'll justify some of the infrastructure we need for reliable unwinding, and
> ensure that it is tested, well before we actually enable reliable stacktracing.
> 

In the current code structure, the annotations are a problem.

The printing of the entry along with the annotations and metadata cannot be done in
the unwind functions themselves as the caller may not even want anything printed.
The printing has to be done in consume_entry() if the caller wants to do it. But
consume_entry() only gets the PC as the argument (apart from the cookie passed by
the caller). It currently has no way of figuring out where the PC was obtained from
(ftrace, kretprobe, pt_regs, etc) or if the PC is reliable.

We need to replace the PC argument with a pointer to a structure that contains the
PC as well as other information about the PC. unwind_init() and unwind_next() need
to update that for each frame.

If this approach is acceptable, I will submit a patch series for that. Please let
me know.

Thanks.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Will Deacon]

On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> I have synced this patch series to v5.19-rc2.
> I have also removed the following patch.
> 
> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> 
> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
> yet. This patch will be added in the future once Objtool is enhanced to
> provide stack validation in some form.

Given that it's not at all obvious that we're going to end up using objtool
for arm64, does this patch series gain us anything in isolation?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/23/22 12:32, Will Deacon wrote:
> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> I have synced this patch series to v5.19-rc2.
>> I have also removed the following patch.
>>
>> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
>>
>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
>> yet. This patch will be added in the future once Objtool is enhanced to
>> provide stack validation in some form.
> 
> Given that it's not at all obvious that we're going to end up using objtool
> for arm64, does this patch series gain us anything in isolation?
> 

BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.

So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
consider reliable stacktrace. These patches reorganize the unwinder code based on
comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
If Mark Rutland OKes them, we should upstream them.

I can drop patches 4 thru 6. Actually, the objtool patch series that I have
sent separately for supporting livepatch already addresses reliability. So, if that
gets reviewed and accepted, we don't even need patches 4 thru 6.

If you are OK with that, I can resend v16 with just patches 1 thru 3. Let me know.

Madhavan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/24/22 00:19, Madhavan T. Venkataraman wrote:
> 
> 
> On 6/23/22 12:32, Will Deacon wrote:
>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>
>>> I have synced this patch series to v5.19-rc2.
>>> I have also removed the following patch.
>>>
>>> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
>>>
>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
>>> yet. This patch will be added in the future once Objtool is enhanced to
>>> provide stack validation in some form.
>>
>> Given that it's not at all obvious that we're going to end up using objtool
>> for arm64, does this patch series gain us anything in isolation?
>>
> 
> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.

Sorry. What I wanted to say was that in v15 I have removed the patch titled:

	arm64: Select HAVE_RELIABLE_STACKTRACE

since objtool changes are not in place.

Apologies.

Madhavan

> 
> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
> consider reliable stacktrace. These patches reorganize the unwinder code based on
> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
> If Mark Rutland OKes them, we should upstream them.
> 
> I can drop patches 4 thru 6. Actually, the objtool patch series that I have
> sent separately for supporting livepatch already addresses reliability. So, if that
> gets reviewed and accepted, we don't even need patches 4 thru 6.
> 
> If you are OK with that, I can resend v16 with just patches 1 thru 3. Let me know.
> 
> Madhavan
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Mark Rutland]

On Fri, Jun 24, 2022 at 12:19:01AM -0500, Madhavan T. Venkataraman wrote:
> 
> 
> On 6/23/22 12:32, Will Deacon wrote:
> > On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
> >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> >>
> >> I have synced this patch series to v5.19-rc2.
> >> I have also removed the following patch.
> >>
> >> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> >>
> >> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
> >> yet. This patch will be added in the future once Objtool is enhanced to
> >> provide stack validation in some form.
> > 
> > Given that it's not at all obvious that we're going to end up using objtool
> > for arm64, does this patch series gain us anything in isolation?
> > 
> 
> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.
> 
> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
> consider reliable stacktrace. These patches reorganize the unwinder code based on
> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
> If Mark Rutland OKes them, we should upstream them.

Sorry for the delay; I have been rather swamped recently and haven't had the
time to give this the time it needs.

I'm happy with patches 1 and 2, and I've acked those in case Will wants to pick
them.

Kalesh (cc'd) is working to share the unwinder code with hyp, and I think that
we need to take a step back and consider how we can make the design work
cleanly with that. I'd had a go at prototyping making the unwinder more data
driven, but I haven't come up with something satisfactory so far.

It would be good if you could look at / comment on each others series.

Thanks,
Mark.

> 
> I can drop patches 4 thru 6. Actually, the objtool patch series that I have
> sent separately for supporting livepatch already addresses reliability. So, if that
> gets reviewed and accepted, we don't even need patches 4 thru 6.
> 
> If you are OK with that, I can resend v16 with just patches 1 thru 3. Let me know.
> 
> Madhavan
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/26/22 04:18, Mark Rutland wrote:
> On Fri, Jun 24, 2022 at 12:19:01AM -0500, Madhavan T. Venkataraman wrote:
>>
>>
>> On 6/23/22 12:32, Will Deacon wrote:
>>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>>
>>>> I have synced this patch series to v5.19-rc2.
>>>> I have also removed the following patch.
>>>>
>>>> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
>>>>
>>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
>>>> yet. This patch will be added in the future once Objtool is enhanced to
>>>> provide stack validation in some form.
>>>
>>> Given that it's not at all obvious that we're going to end up using objtool
>>> for arm64, does this patch series gain us anything in isolation?
>>>
>>
>> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.
>>
>> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
>> consider reliable stacktrace. These patches reorganize the unwinder code based on
>> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
>> If Mark Rutland OKes them, we should upstream them.
> 
> Sorry for the delay; I have been rather swamped recently and haven't had the
> time to give this the time it needs.
> 
> I'm happy with patches 1 and 2, and I've acked those in case Will wants to pick
> them.
> 
> Kalesh (cc'd) is working to share the unwinder code with hyp, and I think that
> we need to take a step back and consider how we can make the design work
> cleanly with that. I'd had a go at prototyping making the unwinder more data
> driven, but I haven't come up with something satisfactory so far.
> 
> It would be good if you could look at / comment on each others series.
> 

I will review Kalesh's unwinder changes.

Thanks.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Kalesh Singh]

On Sun, Jun 26, 2022 at 9:33 PM Madhavan T. Venkataraman
<madvenka@linux.microsoft.com> wrote:
>
>
>
> On 6/26/22 04:18, Mark Rutland wrote:
> > On Fri, Jun 24, 2022 at 12:19:01AM -0500, Madhavan T. Venkataraman wrote:
> >>
> >>
> >> On 6/23/22 12:32, Will Deacon wrote:
> >>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
> >>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> >>>>
> >>>> I have synced this patch series to v5.19-rc2.
> >>>> I have also removed the following patch.
> >>>>
> >>>>    [PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> >>>>
> >>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
> >>>> yet. This patch will be added in the future once Objtool is enhanced to
> >>>> provide stack validation in some form.
> >>>
> >>> Given that it's not at all obvious that we're going to end up using objtool
> >>> for arm64, does this patch series gain us anything in isolation?
> >>>
> >>
> >> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.
> >>
> >> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
> >> consider reliable stacktrace. These patches reorganize the unwinder code based on
> >> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
> >> If Mark Rutland OKes them, we should upstream them.
> >
> > Sorry for the delay; I have been rather swamped recently and haven't had the
> > time to give this the time it needs.
> >
> > I'm happy with patches 1 and 2, and I've acked those in case Will wants to pick
> > them.
> >
> > Kalesh (cc'd) is working to share the unwinder code with hyp, and I think that
> > we need to take a step back and consider how we can make the design work
> > cleanly with that. I'd had a go at prototyping making the unwinder more data
> > driven, but I haven't come up with something satisfactory so far.
> >
> > It would be good if you could look at / comment on each others series.
> >
>
> I will review Kalesh's unwinder changes.

Thanks Mark, I'll take a look.

Madhavan, I'm in the process of preparing a new version. Let me rebase
on your first 2 patches and resend, so you can look at that version
instead.

Thanks,
Kalesh

>
> Thanks.
>
> Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/27/22 11:32, Kalesh Singh wrote:
> On Sun, Jun 26, 2022 at 9:33 PM Madhavan T. Venkataraman
> <madvenka@linux.microsoft.com> wrote:
>>
>>
>>
>> On 6/26/22 04:18, Mark Rutland wrote:
>>> On Fri, Jun 24, 2022 at 12:19:01AM -0500, Madhavan T. Venkataraman wrote:
>>>>
>>>>
>>>> On 6/23/22 12:32, Will Deacon wrote:
>>>>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
>>>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>>>>
>>>>>> I have synced this patch series to v5.19-rc2.
>>>>>> I have also removed the following patch.
>>>>>>
>>>>>>    [PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
>>>>>>
>>>>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
>>>>>> yet. This patch will be added in the future once Objtool is enhanced to
>>>>>> provide stack validation in some form.
>>>>>
>>>>> Given that it's not at all obvious that we're going to end up using objtool
>>>>> for arm64, does this patch series gain us anything in isolation?
>>>>>
>>>>
>>>> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.
>>>>
>>>> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
>>>> consider reliable stacktrace. These patches reorganize the unwinder code based on
>>>> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
>>>> If Mark Rutland OKes them, we should upstream them.
>>>
>>> Sorry for the delay; I have been rather swamped recently and haven't had the
>>> time to give this the time it needs.
>>>
>>> I'm happy with patches 1 and 2, and I've acked those in case Will wants to pick
>>> them.
>>>
>>> Kalesh (cc'd) is working to share the unwinder code with hyp, and I think that
>>> we need to take a step back and consider how we can make the design work
>>> cleanly with that. I'd had a go at prototyping making the unwinder more data
>>> driven, but I haven't come up with something satisfactory so far.
>>>
>>> It would be good if you could look at / comment on each others series.
>>>
>>
>> I will review Kalesh's unwinder changes.
> 
> Thanks Mark, I'll take a look.
> 
> Madhavan, I'm in the process of preparing a new version. Let me rebase
> on your first 2 patches and resend, so you can look at that version
> instead.
> 

Sure thing.

Thanks.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/26/22 04:18, Mark Rutland wrote:
> On Fri, Jun 24, 2022 at 12:19:01AM -0500, Madhavan T. Venkataraman wrote:
>>
>>
>> On 6/23/22 12:32, Will Deacon wrote:
>>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>>
>>>> I have synced this patch series to v5.19-rc2.
>>>> I have also removed the following patch.
>>>>
>>>> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
>>>>
>>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
>>>> yet. This patch will be added in the future once Objtool is enhanced to
>>>> provide stack validation in some form.
>>>
>>> Given that it's not at all obvious that we're going to end up using objtool
>>> for arm64, does this patch series gain us anything in isolation?
>>>
>>
>> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.
>>
>> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
>> consider reliable stacktrace. These patches reorganize the unwinder code based on
>> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
>> If Mark Rutland OKes them, we should upstream them.
> 
> Sorry for the delay; I have been rather swamped recently and haven't had the
> time to give this the time it needs.
> 
> I'm happy with patches 1 and 2, and I've acked those in case Will wants to pick
> them.
> 

Thanks for the review.

Will,

Are you fine with picking up patches 1 and 2?

For the other patches, I have responded separately.

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Will Deacon]

On Sun, Jun 26, 2022 at 11:48:36PM -0500, Madhavan T. Venkataraman wrote:
> 
> 
> On 6/26/22 04:18, Mark Rutland wrote:
> > On Fri, Jun 24, 2022 at 12:19:01AM -0500, Madhavan T. Venkataraman wrote:
> >>
> >>
> >> On 6/23/22 12:32, Will Deacon wrote:
> >>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
> >>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> >>>>
> >>>> I have synced this patch series to v5.19-rc2.
> >>>> I have also removed the following patch.
> >>>>
> >>>> 	[PATCH v14 7/7] arm64: Select HAVE_RELIABLE_STACKTRACE
> >>>>
> >>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
> >>>> yet. This patch will be added in the future once Objtool is enhanced to
> >>>> provide stack validation in some form.
> >>>
> >>> Given that it's not at all obvious that we're going to end up using objtool
> >>> for arm64, does this patch series gain us anything in isolation?
> >>>
> >>
> >> BTW, I have synced my patchset to 5.19-rc2 and sent it as v15.
> >>
> >> So, to answer your question, patches 1 thru 3 in v15 are still useful even if we don't
> >> consider reliable stacktrace. These patches reorganize the unwinder code based on
> >> comments from both Mark Rutland and Mark Brown. Mark Brown has already OKed them.
> >> If Mark Rutland OKes them, we should upstream them.
> > 
> > Sorry for the delay; I have been rather swamped recently and haven't had the
> > time to give this the time it needs.
> > 
> > I'm happy with patches 1 and 2, and I've acked those in case Will wants to pick
> > them.
> > 
> 
> Thanks for the review.
> 
> Will,
> 
> Are you fine with picking up patches 1 and 2?
> 
> For the other patches, I have responded separately.

Sure thing, I'll do that today. Thanks for persevering with this.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 841 bytes --]

On Thu, Jun 23, 2022 at 06:32:24PM +0100, Will Deacon wrote:
> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:

> > as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
> > yet. This patch will be added in the future once Objtool is enhanced to
> > provide stack validation in some form.

> Given that it's not at all obvious that we're going to end up using objtool
> for arm64, does this patch series gain us anything in isolation?

Having the reliability information seems like it should be useful in
general even without doing live patching - we can use it to annotate
stack traces to warn people about anything that might be suspect in
there.  For live patching it's probably something we'll want regardless
of the use of objtool, it's one more robustness check which always
helps.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v15 0/6] arm64: Reorganize the unwinder and implement stack trace reliability checks

[Madhavan T. Venkataraman]

On 6/24/22 06:42, Mark Brown wrote:
> On Thu, Jun 23, 2022 at 06:32:24PM +0100, Will Deacon wrote:
>> On Fri, Jun 17, 2022 at 04:07:11PM -0500, madvenka@linux.microsoft.com wrote:
> 
>>> as HAVE_RELIABLE_STACKTRACE depends on STACK_VALIDATION which is not present
>>> yet. This patch will be added in the future once Objtool is enhanced to
>>> provide stack validation in some form.
> 
>> Given that it's not at all obvious that we're going to end up using objtool
>> for arm64, does this patch series gain us anything in isolation?
> 
> Having the reliability information seems like it should be useful in
> general even without doing live patching - we can use it to annotate
> stack traces to warn people about anything that might be suspect in
> there.  For live patching it's probably something we'll want regardless
> of the use of objtool, it's one more robustness check which always
> helps.

Hi Mark, Will,

Your comments got me to thinking about the Objtool patch series I have sent earlier.

Since the general feeling is that Objtool is unlikely to be our path to livepatch on ARM64, I think that I can implement what I want in a simpler way as a kernel-only solution. The kernel already has a decoder. I don't need
to provide one. In the kernel-only solution, I don't have to worry about relocations, alternatives, etc, etc.

The number of patches would be about half of the original series with simpler code in many of the patches.

The amount of memory consumed by the CFI entries will most likely be just a fraction of the original series.

I will investigate this. If it works and turns out to be a lot simpler, I will send this as v3 of the livepatch
patch series. Also, if this works, we can replace the various reliability checks with just a single fp validation
check in the unwinder.

Thanks for the input.

Madhavan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel